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.
Tags
None
Referenced Files
Unknown Object (File)
Tue, May 14, 9:24 AM
Unknown Object (File)
Mon, May 13, 6:24 PM
Unknown Object (File)
Sun, May 12, 9:20 PM
Unknown Object (File)
Sat, May 11, 11:06 AM
Unknown Object (File)
Sat, May 11, 1:29 AM
Unknown Object (File)
Fri, May 10, 12:22 PM
Unknown Object (File)
Sun, May 5, 2:51 AM
Unknown Object (File)
Sat, May 4, 8:26 AM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

Fix locking issue in mlx5en(4).

Fix conflict between libnvpair and libnv when building rescue binaries.

make buildworld && make buildkernel

  1. passed

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 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 ↗(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.

sys/dev/mlx5/mlx5_en/mlx5_en_main.c
3532 ↗(On Diff #105039)

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 ↗(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 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 ↗(On Diff #105072)

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 ↗(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 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 ↗(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 marked 7 inline comments as done.May 23 2022, 9:58 PM
kib added inline comments.
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 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 ↗(On Diff #106151)

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