Page MenuHomeFreeBSD

ifconfig: Add option to report VF status info
Needs ReviewPublic

Authored by erj on Mar 19 2019, 6:25 PM.

Details

Reviewers
shurd
marius
Group Reviewers
iflib
Summary
  • Adds new options to ifconfig for this: "-s"
  • Adds ioctl command for reporting VF status info from drivers
  • Adds support to iflib for drivers to handle this new ioctl
  • Add support for ioctl in ixl(4)

Signed-off-by: Eric Joyner <erj@freebsd.org>

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23686
Build 22660: arc lint + arc unit

Event Timeline

erj created this revision.Mar 19 2019, 6:25 PM
erj planned changes to this revision.Mar 19 2019, 6:25 PM
Harbormaster completed remote builds in B23180: Diff 55243.
marius requested changes to this revision.Mar 27 2019, 7:51 PM
marius added a subscriber: marius.
marius added inline comments.
sbin/ifconfig/ifconfig.c
391

I don't particularly like the idea of adding "-s" (no real association with the VF status in particular) or actually adding any additional option for this; what's wrong with just adding the VF status to the verbose ("-v") information akin to the SFP data?

sbin/ifconfig/ifvfstatus.c
2

This file needs a license boilerplate and a "$FreeBSD$".

sys/dev/ixl/if_ixl.c
1664

This should either use sizeof(ent.mac_addr) instead of ETHER_ADDR_LEN or the declaration of mac_addr should use ETHER_ADDR_LEN instead of 6, too, but you should not hardcode the assumption that ETHER_ADDR_LEN equals 6.

sys/net/if.h
591

Some spares probably would be a good idea and yield better alignment.

sys/net/iflib.c
4222

"ifvfs" should be declared at the top of iflib_if_ioctl(), just like its other variables.

sys/sys/sockio.h
146

This should be "define<tab>" rather than "define<space>".

erj added inline comments.Apr 11 2019, 6:18 PM
sbin/ifconfig/ifconfig.c
391

My thought was that verbose might get too verbose with both SFP data and this info in it -- especially if you decide to add 128 VFs to a single port, which is currently possible on ixl.

That said, I did this because I was experimenting to see what I could get away with in terms of editing ifconfig. I don't mind removing the option if this patch ends up being something that people seriously want.

sbin/ifconfig/ifvfstatus.c
2

Yeah -- will do.

sys/dev/ixl/if_ixl.c
57

this should get removed...

sys/net/if.h
591

Then what should I do here? How much space space would be good? Something like another 8 or 16 bytes for spares?

sys/sys/sockio.h
146

This is already a tab. It's just one column wide?

erj updated this revision to Diff 56102.Apr 11 2019, 6:38 PM
erj marked an inline comment as not done.
  • ifconfig: Add option to report VF status info
  • ixl: Add a license to ifvfstatus.c
  • ixl: Updates to if_ixl.c
  • Add spare data fields to struct ifvfstatus
marius requested changes to this revision.Apr 15 2019, 2:41 PM
marius added inline comments.
sbin/ifconfig/ifconfig.c
391

Well, with 128 VFs on a single port, the 4 lines of SFP status at the verbosity level of 1 hardly should make it too verbose and at higher verbosity levels, the user really is asking for more output. But okay, let's see what others think.

sys/net/if.h
591

Yes, using 128 bytes in total looks good to me.

sys/sys/sockio.h
146

Hum, it still is "define<space>" according to the raw version of the diff.

This revision now requires changes to proceed.Apr 15 2019, 2:41 PM
erj updated this revision to Diff 56233.Apr 16 2019, 12:40 AM
erj marked an inline comment as done.
  • Replace a space after #define with tab
erj marked an inline comment as done.Apr 16 2019, 12:41 AM