Repository: cilium/ebpf
Pull request #204 (2021-01-27 04:02:41)

map: Add a method to Unpin maps
This is an inverse of the Pin method. Also, add a method to get Map's pinned path.

Signed-off-by: Aditi Ghag aditi@cilium.io

lmb: The output from `quicktest` seems much more understandable than `stretchr/testify` at least:
```
=== RUN   TestFoo
    /home/lorenz/dev/quicktesttest/main_test.go:10:
        error:
          values are not equal
        got:
          bool(false)
        want:
          bool(true)
        stack:
          /home/lorenz/dev/quicktesttest/main_test.go:18
            qt.Assert(t, false, qt.Equals, true)
          /home/lorenz/dev/quicktesttest/main_test.go:10
            helper(t)
```

It also doesn't pull in a bazillion dependecies. Maybe it's not so bad? It's still an additional dependency, but oh well.
aditighag: >How would this work with a map being pinned in multiple places? Seems like calling Pin repeatedly will overwrite the previous pin state, so leave "stray" pinned maps.

This is an existing issue, no? Anyway I _think_ this should be easy to fix with the help of the new `IsPinned` method. 
aditighag: > The output from `quicktest` seems much more understandable than `stretchr/testify` at least:
> 
> ```
> === RUN   TestFoo
>     /home/lorenz/dev/quicktesttest/main_test.go:10:
>         error:
>           values are not equal
>         got:
>           bool(false)
>         want:
>           bool(true)
>         stack:
>           /home/lorenz/dev/quicktesttest/main_test.go:18
>             qt.Assert(t, false, qt.Equals, true)
>           /home/lorenz/dev/quicktesttest/main_test.go:10
>             helper(t)
> ```
> 
> It also doesn't pull in a bazillion dependecies. Maybe it's not so bad? It's still an additional dependency, but oh well.

Yup, I like the pretty error reporting, and the fact that it exports only a handful of methods (less dependencies), quicktest trumps testify for our current use.
aditighag: >P.S.: What about Program.Unpin?

Haven't really thought about it. How is this related to `Map.Unpin` (mainly the focus of this PR), though?
lmb: > This is an existing issue, no?

```
m.Pin("/foo")
m.Pin("/bar")
m.Unpin()
```

With your PR, only /bar is unlinked. Basically, I don't share your assumption that a map is only ever pinned in one place. `IsPinned` doesn't fix that.

> How is this related to Map.Unpin (mainly the focus of this PR), though?

I'm playing the devils advocate. Program.Pin exists, and is used for the same purpose. Why should we add Map.Unpin but not Program.Unpin?

Maybe we're going at this in the wrong way: why do you need to unpin? From personal experience, a pinned map is never unpinned because it contains valuable state (that's why it's pinned). If you want to re-use a pinned map, you can use [MapOptions.PinPath](https://pkg.go.dev/github.com/cilium/ebpf#MapOptions) together with `LIBBPF_PIN_BY_NAME`. This has the effect that the library does the pinning and reusing of maps for you. If you then really want to get rid of your state that is  just `os.RemoveAll(PinPath)` away.
aditighag: > that a map is only ever pinned in one place.

I'm not aware of any use cases where a map is pinned to multiple paths. Can you elaborate on why we can't make this assumption?
/cc @joestringer 

>Why should we add Map.Unpin but not Program.Unpin?

By all means :) But that's outside the scope of this PR.

>If you then really want to get rid of your state that is just os.RemoveAll(PinPath) away.

It's not just that. With the current API design, we don't require users to pass the pinned path to `Unpin()`. There is a reason why the `Map`'s pinned path is embedded into the struct so that we can avoid error conditions where users pass a wrong path (e.g., another map's path) while calling `Unpin()` for a map. Thoughts?
joestringer: > I'm not aware of any use cases where a map is pinned to multiple paths. Can you elaborate on why we can't make this assumption?

The difficulty with library development is that the aim is to build something generic anyone could use, so it becomes important to the consider use cases outside the one we immediately have in front of us. If we have the foresight at this point to understand that there are two plausible use cases, then ideally we can come up with a sane way to handle both of those use cases.

I see @lmb's point here, currently there is no `Unpin()`, so assuming you have a use case, you can pin wherever you like and as many times as you like. It could be something as simple as "I have a program-friendly map naming scheme that can be generated from hashes/uuids, plus a user-friendly map naming scheme which makes it easier for users to interact with, and I want both maps to point to the same underlying object".  The "official" Unpin approach today would be, the user should simply remove the file from the filesystem through something like `os.Remove()`. Proper filesystem cleanup is completely up to the library user.

At a philosophical level though, if the library provides a `Pin()` function, then we are inherently saying that the library is responsible for handling filesystem association. That makes me wonder, why should `Unpin()` be different, just because under the hood it uses the `unlink()` syscall rather than `bpf()`? It's "nice" to have properly balanced "Add()"/"Remove()", "Create"/"Delete", or in this case "Pin()"/"Unpin()".

As a (slight) aside, actually looking at the `os.Remove()` [implementation](https://golang.org/src/os/file_unix.go?s=8292:8322#L265), if you pass a directory in instead of a path then it will attempt to delete the directory which seems like it could potentially have the wrong behaviour, hiding this behind a bpf library abstraction might make that kind of mistake more difficult to trigger (because we could ensure that the filepath is *not* a directory during the `Pin()`, and/or implement the `Unpin()` directly with `Unlink()` (though we probably don't want to get into os abstractions like that in this library, `os.Remove()` already does those bits for us). I think that's the kind of potholes that @aditighag suggests we could help library users to avoid.

Looking at the code proposal, one natural way to handle multiple mappings would be:

```
m1 := NewMap(...)
// Optional; could be part of Spec or could be manually done
m2.Pin("/first/path")
m2 := m.Clone()
m2.Pin("/second/path")
// ... use the maps
m.Unpin()
// ... keep m2 around for a while if you want
m2.Unpin()
```

Then, if you really care about separately handling the paths because your use case demands that the paths have different lifecycles, you keep two structures around and interact with them independently. To make this more clean, one might argue to take it one step further and force the `Pin()` to tie to a single path, ie if you `Pin()` an already-pinned map, it either fails with "ErrorAlreadyPinned" or moves the pinning to the new location. Then it would be difficult for the library user to shoot themselves in the foot by leaving old pinnings on the filesystem. Downside is a little extra memory usage, but for the rich representation of the map, maybe that's not so terrible?


> Maybe we're going at this in the wrong way: why do you need to unpin? From personal experience, a pinned map is never unpinned because it contains valuable state (that's why it's pinned). If you want to re-use a pinned map, you can use MapOptions.PinPath together with LIBBPF_PIN_BY_NAME. This has the effect that the library does the pinning and reusing of maps for you. If you then really want to get rid of your state that is just os.RemoveAll(PinPath) away.

The Cilium use case is primarily around upgrade or reconfiguration. One version of Cilium defines the map in one way (with certain attributes like size for instance), then the new version defines the map in a different way. We want to reuse the path but redefine the attributes. I think the following workflow is fairly reasonable (though I didn't spend too long looking into it):

```
fd := os.Open(...)
m := NewMapFromFD(fd)
// Check the attributes match what we want them to be, if they're good we use `m`, if not we want to delete/recreate it (and potentially run some special logic to safely migrate map contents).
m.Unpin()
```

A slight variation on this case is that we make sure there is a backup of the currently-active map, then we create the new map and populate it, then we switch the BPF program over to use the new map, then once everything is successful, we unpin the old one. If some of those steps were unsuccessful (eg prog load fails), then we can roll back all the steps. `cilium/cilium/pkg/bpf/map_linux.go:OpenParallel()` is one example, there's also `pkg/datapath/ipcache/listener.go:garbageCollect()` with the `shuffleMaps()` for another example. Some of that code could improve from more reuse of this library, but either way they'll need to actually handle the `Unpin()` of the old map. These are not required to be during upgrade either, the `garbageCollect()` case is recreating the same map in-place to work around a kernel behaviour on older kernels.
aditighag: Thanks for the detailed answer, Joe. 

Sure, it makes sense to not make the assumption with respect to how many times a map can be pinned. 

> 
> Looking at the code proposal, one natural way to handle multiple mappings would be:
> 
> ```
> m1 := NewMap(...)
> // Optional; could be part of Spec or could be manually done
> m2.Pin("/first/path")
> m2 := m.Clone()
> m2.Pin("/second/path")
> // ... use the maps
> m.Unpin()
> // ... keep m2 around for a while if you want
> m2.Unpin()
> ```
> 
> Then, if you really care about separately handling the paths because your use case demands that the paths have different lifecycles, you keep two structures around and interact with them independently. To make this more clean, one might argue to take it one step further and force the `Pin()` to tie to a single path, ie if you `Pin()` an already-pinned map, it either fails with "ErrorAlreadyPinned" or moves the pinning to the new location. Then it would be difficult for the library user to shoot themselves in the foot by leaving old pinnings on the filesystem. Downside is a little extra memory usage, but for the rich representation of the map, maybe that's not so terrible?

Yes, the current design of the `Clone` API can be used to support multiple pinned paths. Either we could document that users can use `Clone` to pin a map to multiple paths, or we can facilitate that behind the scenes. If it's the latter, I'm inclined to keep the current `Pin` method as-is (aligned with the most common use case), and introduce a new method `PinWithClone` to make the intent more clear.
@joestringer @lmb Do you prefer one to the other?  


joestringer: I don't think we need extra functions like `PinWithClone()`. If there's a simple, straightforward way to achieve this for users who care by doing `m2 := m.Clone()` + `m2.Pin()` then that would satisfy the use case.
aditighag: > I don't think we need extra functions like `PinWithClone()`. If there's a simple, straightforward way to achieve this for users who care by doing `m2 := m.Clone()` + `m2.Pin()` then that would satisfy the use case.

Yup, that was my preference too. I've documented the behavior. PTAL.
joestringer: > Let's imagine this uses the new API:

> func shuffleMaps(a, b Map) error {
    os.Rename(a.Path(), "backup")
    os.Rename(b.Path(), a.Path())
    // not sure what happens to "backup"?
}

> Since we've tied a path to a map, both a and b are inconsistent. a.Unpin will remove b, and b.Unpin produces an error. I don't see how to make this safe.

Right, @aditighag this is an interesting point on the design. By adding `Unpin()` and holding path state, we *force* library users to interact with the library to change the pinning on the filesystem. If library users otherwise interact with the underlying filesystem like the above, then the `Unpin()` will fail.

To implement 1:1, the above would need to become something like:

```
path := oldMap.Path()
oldMap.Unpin()
newMap.Pin(path)
```

(plus the rollback logic to re-pin `oldMap` upon failure).


I think that the idea would be to replace the whole `shuffleMaps` stuff instead with two in-memory `*ebpf.Map`, each of which has an open fd, and you basically unpin
joestringer: > Let's imagine this uses the new API:

> func shuffleMaps(a, b Map) error {
    os.Rename(a.Path(), "backup")
    os.Rename(b.Path(), a.Path())
    // not sure what happens to "backup"?
}

> Since we've tied a path to a map, both a and b are inconsistent. a.Unpin will remove b, and b.Unpin produces an error. I don't see how to make this safe.

Right, @aditighag this is an interesting point on the design. By adding `Unpin()` and holding path state, we *force* library users to interact with the library to change the pinning on the filesystem. If library users otherwise interact with the underlying filesystem like the above, then the `Unpin()` will fail.

To implement 1:1, the above would need to become something like:

```
path := oldMap.Path()
// Depending on whether we define that `Pin()` has move semantics or create semantics (ie fail if it is already pinned), unpin the old path
oldMap.Unpin()
oldMap.Pin("backup")
newMap.Pin(path)
```

Strictly speaking for the actual use case above, we could probably get away with just an `oldMap.Unpin()` + `newMap.Pin()`, I think it's just trying to ensure that there are traces on the filesystem of the ongoing operation, in case anything goes wrong (kill the daemon in between operations, perhaps?)
aditighag: I've pushed a new update to accommodate the test cases involving `LoadPinnedMap`. The latest changes ensure that users won't get inconsistent view of a map's pinned state as long as they have used the internal APIs of the library. 

> Let's imagine this uses the new API:
> 
> func shuffleMaps(a, b Map) error {
>     os.Rename(a.Path(), "backup")
>     os.Rename(b.Path(), a.Path())
>     // not sure what happens to "backup"?
> }
With the new APIs, the function calls will translate to realized.Unpin(), realized.Pin("backup"), pending.Unpin(), pending.Pin("realized").  

>  If library users otherwise interact with the underlying filesystem like the above, then the Unpin() will fail.

I've changed the behavior such that it won't. 
aditighag: Re: `Unpin` for `prog`, I went over prog.go, looks like the logic is likely to be same as `Map.Unpin`. Once we finalize the API design, I'll implement a common construct for both `Map` and `Prog`. 
ti-mo: > Since Unpin so far is idempotent, maybe it makes sense to treat ENOENT from os.Remove as no error? Then the behaviour of (2) and (3) is the same.

Agreed, if ENOENT is received, that means `Unpin()`'s goal is already accomplished, although as a user I might want to check the status of the unpin operation. Not sure if this would come in handy.

> Note that skipping a.Unpin because we always want there to be a pinned map is not safe, because then a.Unpin would remove b. In general, playing games with .Path() is a hazard, so I wonder if there is a way to get away without it?

IMO the safer way would be calling `os.Rename()` under the hood and keeping the map's internal pin path state in sync. Would be cleaner for the caller, too. Instead of juggling `Unpin()` and `Pin()`s to recreate a map, simply:

```
old.Pin("backup")
new.Pin(old_path)
```

Error handling becomes less involved for the caller because there are less moving parts. If `old.Pin("backup")` succeeds, there's a guarantee that the fs was updated and that the new path was committed to `old`'s struct.

Re: `prog.Unpin()`, why not tackle that in a separate PR?
ti-mo: ```
old.Pin("backup")
// crash here makes old not discoverable so a restart will fail
```
Assuming `old.Pin("backup")` was successful, `backup` maintains a ref to the map, so the map will outlive the process in case of a crash. If the process comes back up, it can/should discover an existing map by opening `backup` first, or it can clean up and start from scratch depending on the use case.

`Pin()` using an atomic fs operation removes all risk of the process terminating while it's holding the only map ref, so there shouldn't be any data loss. 
lmb: @aditighag ping, are you still interested in this?
aditighag: @lmb Yes, I'm still working on it. I'll follow-up shortly. 
aditighag: Pushed a new commit with the following main changes - 

- `Pin` method allows overriding previously pinned path. This was done to accommodate the Cilium like use cases where map paths are updated for backup purposes. 
- `IsPinned` now only checks if a map has a non-empty path or not.

Thanks!
aditighag: @lmb Thanks for your detailed review. Can we get this merged? I'll send another PR with prog pinning support. 

Patch:
diff --git a/go.mod b/go.mod
index 35ab732..df81396 100644
--- a/go.mod
+++ b/go.mod
@@ -3,6 +3,7 @@ module github.com/cilium/ebpf
 go 1.15
 
 require (
+	github.com/frankban/quicktest v1.11.3
 	github.com/google/go-cmp v0.5.4
 	golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c
 )
diff --git a/go.sum b/go.sum
index d7787fc..7f94146 100644
--- a/go.sum
+++ b/go.sum
@@ -1,5 +1,12 @@
+github.com/frankban/quicktest v1.11.3 h1:8sXhOn0uLys67V8EsXLc6eszDs8VXWxL3iRvebPhedY=
+github.com/frankban/quicktest v1.11.3/go.mod h1:wRf/ReqHper53s+kmmSZizM8NamnL3IM0I9ntUbOk+k=
 github.com/google/go-cmp v0.5.4 h1:L8R9j+yAqZuZjsqh/z+F1NCffTKKLShY6zXTItVIZ8M=
 github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
+github.com/kr/pretty v0.2.1 h1:Fmg33tUaq4/8ym9TJN1x7sLJnHVwhP33CNkpYV/7rwI=
+github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
+github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
+github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
+github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 golang.org/x/sys v0.0.0-20200124204421-9fbb57f87de9 h1:1/DFK4b7JH8DmkqhUk48onnSfrPzImPoVxuomtbT2nk=
 golang.org/x/sys v0.0.0-20200124204421-9fbb57f87de9/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c h1:VwygUrnw9jn88c4u8GD3rZQbqrP/tgas88tPUbBxQrk=
diff --git a/map.go b/map.go
index 23beb8d..658be27 100644
--- a/map.go
+++ b/map.go
@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"os"
 	"path/filepath"
 	"strings"
 
@@ -125,6 +126,7 @@ type Map struct {
 	valueSize  uint32
 	maxEntries uint32
 	flags      uint32
+	pinnedPath string
 	// Per CPU maps return values larger than the size in the spec
 	fullValueSize int
 }
@@ -354,6 +356,7 @@ func newMap(fd *internal.FD, name string, typ MapType, keySize, valueSize, maxEn
 		valueSize,
 		maxEntries,
 		flags,
+		"",
 		int(valueSize),
 	}
 
@@ -618,6 +621,7 @@ func (m *Map) FD() int {
 //
 // Closing the duplicate does not affect the original, and vice versa.
 // Changes made to the map are reflected by both instances however.
+// If the original map was pinned, the cloned map will not be pinned by default.
 //
 // Cloning a nil Map returns nil.
 func (m *Map) Clone() (*Map, error) {
@@ -638,15 +642,63 @@ func (m *Map) Clone() (*Map, error) {
 		m.valueSize,
 		m.maxEntries,
 		m.flags,
+		"",
 		m.fullValueSize,
 	}, nil
 }
 
 // Pin persists the map past the lifetime of the process that created it.
 //
+// Calling Pin on a previously pinned map will override the path.
+// You can Clone a map to pin it to a different path.
+//
 // This requires bpffs to be mounted above fileName. See https://docs.cilium.io/en/k8s-doc/admin/#admin-mount-bpffs
 func (m *Map) Pin(fileName string) error {
-	return internal.BPFObjPin(fileName, m.fd)
+	if fileName == "" {
+		return fmt.Errorf("pinned path cannot be empty")
+	}
+	if m.IsPinned() {
+		path := m.pinnedPath
+		if path == fileName {
+			return nil
+		}
+		if err := os.Rename(m.pinnedPath, fileName); err != nil {
+			if !os.IsNotExist(err) {
+				return fmt.Errorf("unable to pin the map at new path %v: %w", fileName, err)
+			}
+		} else {
+			m.pinnedPath = fileName
+			return nil
+		}
+	}
+	err := internal.BPFObjPin(fileName, m.fd)
+	if err == nil {
+		m.pinnedPath = fileName
+	}
+	return err
+}
+
+// Unpin removes the persisted state for the map.
+//
+// Unpinning an un-pinned Map returns nil.
+func (m *Map) Unpin() error {
+	if m.pinnedPath == "" {
+		return nil
+	}
+	err := os.Remove(m.pinnedPath)
+	if err == nil || os.IsNotExist(err) {
+		m.pinnedPath = ""
+		return nil
+	}
+	return err
+}
+
+// IsPinned returns true if the map has non-empty pinned path.
+func (m *Map) IsPinned() bool {
+	if m.pinnedPath == "" {
+		return false
+	}
+	return true
 }
 
 // Freeze prevents a map to be modified from user space.
@@ -789,7 +841,12 @@ func LoadPinnedMap(fileName string) (*Map, error) {
 		return nil, err
 	}
 
-	return newMapFromFD(fd)
+	m, err := newMapFromFD(fd)
+	if err == nil {
+		m.pinnedPath = fileName
+	}
+
+	return m, err
 }
 
 // unmarshalMap creates a map from a map ID encoded in host endianness.


Test patch:
diff --git a/map_test.go b/map_test.go
index 893616b..bf4d0f0 100644
--- a/map_test.go
+++ b/map_test.go
@@ -16,6 +16,19 @@ import (
 	"github.com/cilium/ebpf/internal/btf"
 	"github.com/cilium/ebpf/internal/testutils"
 	"github.com/cilium/ebpf/internal/unix"
+
+	qt "github.com/frankban/quicktest"
+)
+
+var (
+	spec1 = &MapSpec{
+		Name:       "foo",
+		Type:       Hash,
+		KeySize:    4,
+		ValueSize:  4,
+		MaxEntries: 1,
+		Pinning:    PinByName,
+	}
 )
 
 func TestMain(m *testing.M) {
@@ -97,6 +110,7 @@ func TestMapCloneNil(t *testing.T) {
 
 func TestMapPin(t *testing.T) {
 	m := createArray(t)
+	c := qt.New(t)
 	defer m.Close()
 
 	if err := m.Put(uint32(0), uint32(42)); err != nil {
@@ -113,6 +127,10 @@ func TestMapPin(t *testing.T) {
 	if err := m.Pin(path); err != nil {
 		t.Fatal(err)
 	}
+
+	pinned := m.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
+
 	m.Close()
 
 	m, err := LoadPinnedMap(path)
@@ -168,6 +186,110 @@ func TestNestedMapPin(t *testing.T) {
 	defer m.Close()
 }
 
+func TestMapPinMultiple(t *testing.T) {
+	tmp := tempBPFFS(t)
+	c := qt.New(t)
+
+	spec := spec1.Copy()
+
+	m1, err := NewMapWithOptions(spec, MapOptions{PinPath: tmp})
+	if err != nil {
+		t.Fatal("Can't create map:", err)
+	}
+	defer m1.Close()
+	pinned := m1.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
+
+	newPath := filepath.Join(tmp, "bar")
+	err = m1.Pin(newPath)
+	c.Assert(err, qt.IsNil)
+	oldPath := filepath.Join(tmp, spec.Name)
+	if _, err := os.Stat(oldPath); err == nil {
+		t.Fatal("Previous pinned map path still exists:", err)
+	}
+	m2, err := LoadPinnedMap(newPath)
+	c.Assert(err, qt.IsNil)
+	defer m2.Close()
+}
+
+func TestMapPinWithEmptyPath(t *testing.T) {
+	m := createArray(t)
+	c := qt.New(t)
+	defer m.Close()
+
+	err := m.Pin("")
+
+	c.Assert(err, qt.Not(qt.IsNil))
+}
+
+func TestMapUnpin(t *testing.T) {
+	tmp := tempBPFFS(t)
+	c := qt.New(t)
+	spec := spec1.Copy()
+
+	m, err := NewMapWithOptions(spec, MapOptions{PinPath: tmp})
+	if err != nil {
+		t.Fatal("Failed to create map:", err)
+	}
+	defer m.Close()
+
+	pinned := m.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
+	path := filepath.Join(tmp, spec.Name)
+	m2, err := LoadPinnedMap(path)
+	c.Assert(err, qt.IsNil)
+	defer m2.Close()
+
+	if err = m.Unpin(); err != nil {
+		t.Fatal("Failed to unpin map:", err)
+	}
+	if _, err := os.Stat(path); err == nil {
+		t.Fatal("Pinned map path still exists after unpinning:", err)
+	}
+}
+
+func TestMapLoadPinned(t *testing.T) {
+	tmp := tempBPFFS(t)
+	c := qt.New(t)
+
+	spec := spec1.Copy()
+
+	m1, err := NewMapWithOptions(spec, MapOptions{PinPath: tmp})
+	c.Assert(err, qt.IsNil)
+	defer m1.Close()
+	pinned := m1.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
+
+	path := filepath.Join(tmp, spec.Name)
+	m2, err := LoadPinnedMap(path)
+	c.Assert(err, qt.IsNil)
+	defer m2.Close()
+	pinned = m2.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
+}
+
+func TestMapLoadPinnedUnpin(t *testing.T) {
+	tmp := tempBPFFS(t)
+	c := qt.New(t)
+
+	spec := spec1.Copy()
+
+	m1, err := NewMapWithOptions(spec, MapOptions{PinPath: tmp})
+	c.Assert(err, qt.IsNil)
+	defer m1.Close()
+	pinned := m1.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
+
+	path := filepath.Join(tmp, spec.Name)
+	m2, err := LoadPinnedMap(path)
+	c.Assert(err, qt.IsNil)
+	defer m2.Close()
+	err = m1.Unpin()
+	c.Assert(err, qt.IsNil)
+	err = m2.Unpin()
+	c.Assert(err, qt.IsNil)
+}
+
 func createArray(t *testing.T) *Map {
 	t.Helper()
 
@@ -794,6 +916,7 @@ func TestNewMapFromID(t *testing.T) {
 
 func TestMapPinning(t *testing.T) {
 	tmp := tempBPFFS(t)
+	c := qt.New(t)
 
 	spec := &MapSpec{
 		Name:       "test",
@@ -809,6 +932,8 @@ func TestMapPinning(t *testing.T) {
 		t.Fatal("Can't create map:", err)
 	}
 	defer m1.Close()
+	pinned := m1.IsPinned()
+	c.Assert(pinned, qt.Equals, true)
 
 	if err := m1.Put(uint32(0), uint32(42)); err != nil {
 		t.Fatal("Can't write value:", err)