if(9): Implement support for nvlist-based set- and get- network interface capabilities.

[kib]

Summary

The existing 31 capabilities supported are now called legacy capabilities while
new capabilities will be added and supported by the use of SIOCGIFCAPNV and
SIOCSIFCAPNV. Refer to the if(9) manual page for a detailed description.

MFC after: 1 week
Sponsored by: NVIDIA Networking

Test Plan

The mlx5en changes will be committed separately, and are just there as an example.

[• hselasky]

Fix locking issue in mlx5en(4).

[• hselasky]

Fix conflict between libnvpair and libnv when building rescue binaries.

[• hselasky]

Fix order of variables.

[• hselasky]

make buildworld && make buildkernel

1. passed

[debdrup]

I only noticed one thing in the mdoc syntax.

share/man/man9/ifnet.9

1357–1358 ↗

(On Diff #103831)

The syntax here needs work; I think my suggestion will work.

Be sure to check with mandoc and igor.

[• hselasky]

Update ifnet.9 as suggested by @debdrup .

[debdrup]

Manual page looks good, I'll let others review the code.

[kib]

Directly add if_capenable2 and if_capabilities2. Pre-parse them from nvlist, same as if_capenable, to avoid repeated code in the drivers.

I might add some note about SIOCSIFCAPNV_DRIVER socket option in the man page some time later.

[• hselasky]

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

4558 ↗

(On Diff #105039)

Should add here:

ifp->if_capenable2 = ifp->if_capabilities2;

sys/net/if.h

436 ↗

(On Diff #105039)

Maybe 64 Kbytes would be good enough. 2MBytes seems large.

[• hselasky]

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

3532 ↗

(On Diff #105039)

Do we need to update "ifp->if_capenable2" here?

[kib]

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

3532 ↗

(On Diff #105039)

Yes, we do. But this should be done on the cap-by-cap basis, same as it is done for if_capenable. We need to use e.g. mask2 or reuse mask and do similar manipulations.

This is why I did not touched this aspect. Or, do you mean something different?

sys/net/if.h

436 ↗

(On Diff #105039)

This value is arbitrary. I do not see a problem with 'too large value' at the moment. For one-off ioctls, it should not matter.

[kib]

Properly handle if_capenable2 for mce(4).

[kib]

ping?

[debdrup]

.Dd also needs to be bumped.

share/man/man9/ifnet.9

1356 ↗

(On Diff #105072)

Still missing a space here between NULL and the comma.

[kib]

Space in .Dv NULL ,
Bump man page date in advance.

[debdrup]

mdoc looks good to me.

[jhb]

sbin/ifconfig/ifconfig.c

1478 ↗

(On Diff #100332)

Hmm, this is still not addressed. That is, you still won't show "new" capabilities in the capabilities line. capabilities corresponds to if_capabilities whereas options corresponds to if_capenable and both are extended. I think what you want instead is to do something like:

if (ioctl(s, SIOCGIFCAP, ...) == 0) {
    if ((ifr.ifr_curcap & IFCAP_NV != 0) {
      ...
      ioctl(s, SIOCGIFCAPNV, ...);
      ...
      printf("\toptions");
      cookie = NULL;
      for (first = true; ;; first = false) {
          nvname = nvlist_next(...);
          ...
      }
      if (supmedia) {
          printf("\tcapabilities");
          cookie = NULL;
          for (first = true; first = false) {
              nvame = nvlist_next(...);
              if (nvame == NULL) {
                  ...
              }
              if (type == NV_TYPE_BOOL) {
                  printf("%c%s", first ? ' ' : ',', nvname);
              }
          }
          nvlist_destroy(nvcap);
          free(buf);
      }
    } else {
      if (ifr.ifr_curcap != 0) {
          printb("\toptions", ifr.ifr_curcap, IFCAPBITS);
          putchar('\n');
      }
      if (supmedia && ifr.ifr_reqcap != 0) {
          printb("\tcapabilties", ifr.ifr_reqcap, IFCAPBITS);
          putchar('\n');
      }
}

share/mk/src.libnames.mk

570 ↗

(On Diff #105775)

s/JNV/NV/

sys/net/if.c

2705 ↗

(On Diff #105775)

ENOTTY?

2744 ↗

(On Diff #105775)

Rather than adding a new ioctl, I would just reuse SIOCSIFCAPNV here and the driver contract would be that you are getting the helper struct in the arg in the driver rather than the user pointer. I think we do similar things in some other network ioctls already, for example ADDMULTI and DELMULTI.

[kib]

Remove SIOCSIFCAPNV_DRIVER.
Properly print capabilities from nv list for ifconfig -m.
Fix typo in the libnv variable dir name.

[jhb]

Thanks

[• hselasky]

Changes look good to me. Some minor comments added.

sbin/ifconfig/ifconfig.c

1528 ↗

(On Diff #106151)

Whitespace changes here could be skipped.

share/man/man9/ifnet.9

31 ↗

(On Diff #106151)

Fix date before commit?

1341 ↗

(On Diff #106151)

.Xr ioctl 2 .

1342 ↗

(On Diff #106151)

Spelling?

Caller must provide the pointer to

Caller must provide a pointer to

1353 ↗

(On Diff #106151)

can skip the word "then"

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

3437 ↗

(On Diff #106151)

I think it would be clever to zero-init drv_ioctl_data_d:

struct siocsifcapnv_driver_data *drv_ioctl_data, drv_ioctl_data_d;

struct siocsifcapnv_driver_data *drv_ioctl_data:
struct siocsifcapnv_driver_data drv_ioctl_data_d = {};

sys/net/if.h

486 ↗

(On Diff #106151)

Upper case "nv" -> "NV" ?

[kib]

sbin/ifconfig/ifconfig.c

1528 ↗

(On Diff #106151)

How so? Patch moves the block under the nested 'else'.

share/man/man9/ifnet.9

1341 ↗

(On Diff #106151)

IMO there is no point in cross-reference there. We talk not about syscall, but about some feature implemented as part of the described framework.

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

3437 ↗

(On Diff #106151)

It is not needed, in the sense that driver code which fills the structure, uses it as well, but this is not a critical path.

sys/net/if.h

486 ↗

(On Diff #106151)

Why? Other abbreviations, like 'pcp', 'vlan', or 'fib' are not upper-cased.

[kib]

Minor tweaks suggested by Hans:

• man page grammar fixes
• initialize fake ifcapnv parameter structure in mlx5en ioctl handler

[• hselasky]

I'm good with this.

sbin/ifconfig/ifconfig.c

1528 ↗

(On Diff #106151)

You wrapped a long line, but it is not important.