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
1356–1357

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
4580

Should add here:

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

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

sys/dev/mlx5/mlx5_en/mlx5_en_main.c
3685–3686

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
3685–3686

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
431

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
1356

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
1466–1510

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
2742

ENOTTY?

2781

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
1513

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
3582

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
481

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
1513

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
3582

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
481

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
1513

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