Page MenuHomeFreeBSD

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

Authored by kib on Oct 18 2021, 3:18 PM.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Address comment by @jhb .

Keep existing capabilities as legacy capabilities. Consider moving the TLSTX bits out of there, because it is new and few nics support it currently.

Updated mlx5en driver to support TLS RX caps.

Fix locking issue in mlx5en(4).

Fix conflict between libnvpair and libnv when building rescue binaries.

make buildworld && make buildkernel

  1. passed
hselasky added a reviewer: network.

I only noticed one thing in the mdoc syntax.

share/man/man9/ifnet.9
1355–1356

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

Be sure to check with mandoc and igor.

hselasky marked an inline comment as done.

Update ifnet.9 as suggested by @debdrup .

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

This revision is now accepted and ready to land.Mar 15 2022, 7:06 PM
kib edited reviewers, added: hselasky; removed: 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.

This revision now requires review to proceed.Apr 15 2022, 2:28 PM
sys/dev/mlx5/mlx5_en/mlx5_en_main.c
4558–4559

Should add here:

ifp->if_capenable2 = ifp->if_capabilities2;
sys/net/if.h
435

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

sys/dev/mlx5/mlx5_en/mlx5_en_main.c
3532

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

kib marked 4 inline comments as done.Apr 16 2022, 1:14 AM
kib added inline comments.
sys/dev/mlx5/mlx5_en/mlx5_en_main.c
3532

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
435

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 marked 2 inline comments as done.

Properly handle if_capenable2 for mce(4).

This revision is now accepted and ready to land.Apr 16 2022, 7:08 AM

.Dd also needs to be bumped.

share/man/man9/ifnet.9
1355

Still missing a space here between NULL and the comma.

kib marked an inline comment as done.

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

This revision now requires review to proceed.May 7 2022, 11:23 PM

mdoc looks good to me.

This revision is now accepted and ready to land.May 8 2022, 4:05 PM
sbin/ifconfig/ifconfig.c
1478–1524

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
569

s/JNV/NV/

sys/net/if.c
2704

ENOTTY?

2743

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 marked 4 inline comments as done.May 18 2022, 10:17 PM

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

This revision now requires review to proceed.May 18 2022, 11:52 PM
This revision is now accepted and ready to land.May 20 2022, 5:32 PM

Changes look good to me. Some minor comments added.

sbin/ifconfig/ifconfig.c
1528

Whitespace changes here could be skipped.

share/man/man9/ifnet.9
31

Fix date before commit?

1341

.Xr ioctl 2 .

1342

Spelling?

Caller must provide the pointer to
Caller must provide a pointer to
1353

can skip the word "then"

sys/dev/mlx5/mlx5_en/mlx5_en_main.c
3437

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

Upper case "nv" -> "NV" ?

kib marked 7 inline comments as done.May 23 2022, 9:58 PM
kib added inline comments.
sbin/ifconfig/ifconfig.c
1528

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

share/man/man9/ifnet.9
1341

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

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

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

kib marked 4 inline comments as done.

Minor tweaks suggested by Hans:

  • man page grammar fixes
  • initialize fake ifcapnv parameter structure in mlx5en ioctl handler
This revision now requires review to proceed.May 23 2022, 10:09 PM

I'm good with this.

sbin/ifconfig/ifconfig.c
1528

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

This revision is now accepted and ready to land.May 24 2022, 6:03 AM