Page MenuHomeFreeBSD
Differential D37664

DrvAPI: Extend driver KPI with more accessors
ClosedPublic
Actions

Authored by jhibbits on Dec 9 2022, 8:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 10, 7:21 AM
Unknown Object (File)
Wed, Dec 4, 5:56 PM
Unknown Object (File)
Thu, Nov 28, 10:08 PM
Unknown Object (File)
Thu, Nov 21, 4:19 PM
Unknown Object (File)
Wed, Nov 20, 9:02 AM
Unknown Object (File)
Tue, Nov 19, 4:36 PM
Unknown Object (File)
Mon, Nov 18, 6:24 AM
Unknown Object (File)
Mon, Nov 18, 5:35 AM
View All 84 Files
Subscribers
ae
freebsd_igalic.co
imp
melifaro

Details

Reviewers
glebius
melifaro
Group Reviewers
transport
network
Commits
rGeb1da3e52582: DrvAPI: Extend driver KPI with more accessors
Summary

Add the following accessors to hide some more netstack details:

  • if_get/setcapabilities2 and *bits analogue
  • if_setdname
  • if_getxname
  • if_transmit - wrapper for call to ifp->if_transmit()
  • This required changing the existing if_transmit to

if_transmit_default, since that's its purpose.

  • if_getalloctype
  • if_getindex
  • if_foreach_addr_type - Like if_foreach_lladdr() but for any address family type. Used by some drivers to iterate over all AF_INET addresses.
  • if_init() - wrapper for ifp->if_init() call
  • if_setinputfn
  • if_setsndtagallocfn
  • if_togglehwassist

Diff Detail

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

Event Timeline

jhibbits created this revision.Dec 9 2022, 8:57 PM
Herald added subscribers: melifaro, ae, imp. · View Herald TranscriptDec 9 2022, 8:57 PM
jhibbits requested review of this revision.Dec 9 2022, 8:57 PM
Harbormaster completed remote builds in B48687: Diff 114021.Dec 9 2022, 8:57 PM
jhibbits added a comment.Dec 9 2022, 9:00 PM

Next step would be to move all the accessors into net/if.h, and make if_var.h private to sys/net (netstack).

melifaro added a comment.Dec 10 2022, 5:47 PM

First of all, I fully support the ifnet KPI approach and love to see this (and the other relevant) reviews landed. Thank you for doing this!
Some questions -

  1. When some action happens (like, a user sets the MTU), then (a) driver-specific internal structures and HW state are updated, (b) actual ifp datastructed gets updated, and (c) network-stack-specific staff is triggered (eventhandler etc).

In the current KPI most of the if_set() functions do just (b), but there are examples of functions doing all 3 items (if_setlladdr()). Any thoughts on differentiating between these two classes?

  1. Any thoughts on locking for setters?
  1. Any plans to update ifnet(9)? :-)
sys/net/if.c
4245

Do we really want to expose these two capabilities fields as is?

Could we do per-bit-index interface instead, w/o exposing any underlying storage details?

It would have a nice property of not dealing with setcapabilities3, 4 and so on.

4257

There is already a widely-used if_name() macro. I'd vote for having either if_name() or if_getname(), preferring the former.

4286

It is probably worth considering having const if_t in all getters.

4413

Does it need to have the return code?

4539

Do we want to enter the epoch instead of enforcing the epoch?
To call this function safely, one has to either be in the epoch already or hold an ifp reference.
Personally, I'd prefer to reduce the number of cases when one needs to acquire/free references for the objects.

4750

Nit: worth having input_cb signature separately.

jhibbits added a comment.Dec 11 2022, 4:35 PM
In D37664#856421, @melifaro wrote:

First of all, I fully support the ifnet KPI approach and love to see this (and the other relevant) reviews landed. Thank you for doing this!

Thanks for the review!

Some questions -

  1. When some action happens (like, a user sets the MTU), then (a) driver-specific internal structures and HW state are updated, (b) actual ifp datastructed gets updated, and (c) network-stack-specific staff is triggered (eventhandler etc).

In the current KPI most of the if_set() functions do just (b), but there are examples of functions doing all 3 items (if_setlladdr()). Any thoughts on differentiating between these two classes?

I'm mixed on this. I think it requires further thinking of how drivers and the stack interact, and how other things interact with the stack. if_setlladdr() for instance looks like it needs to be hardware and stack oriented, to keep everything in sync. Unless we make the KPI a "set" and "commit" style. Thoughts on that direction?

  1. Any thoughts on locking for setters?

Maybe for the next iteration of things? Right now just doing the naive approach to get the KPI in place and get drivers ported over.

  1. Any plans to update ifnet(9)? :-)

Definitely need to do that. Thanks for reminding me.

sys/net/if.c
4245

I agree with you, but there are many places that already do if_setcapabilitiesbit(), so expect a bitmask word. Maybe make it accept a bitset instead?

4257

Not sure how I missed it, but yeah if_name() is better than if_getname(). if_getname() would imply that you're getting an immutable object, which is obviously not the case.

4286

I like this idea.

4413

Probably not. All the other setters do have return values, probably for other stacks that could return an error (hypothetically).

4750

Good idea, I was following existing practice, but those existing should be fixed as well.

jhibbits added inline comments.Dec 12 2022, 7:15 PM
sys/net/if.c
4539

Maybe? I'm not sure here. This is analogous to the if_foreach_lladdr() and if_foreach_llmaddr() functions. Should those be updated as well?

jhibbits updated this revision to Diff 114079.Dec 12 2022, 8:42 PM

Address feedback from @melifaro.

  • Switch to if_name() from if_getxname() (makes more sense)
  • Add function typedefs for members, to be used in accessors.
Harbormaster completed remote builds in B48701: Diff 114079.Dec 12 2022, 8:42 PM
melifaro added a comment.Dec 16 2022, 1:29 PM

>> Some questions -

  1. When some action happens (like, a user sets the MTU), then (a) driver-specific internal structures and HW state are updated, (b) actual ifp datastructed gets updated, and (c) network-stack-specific staff is triggered (eventhandler etc).

In the current KPI most of the if_set() functions do just (b), but there are examples of functions doing all 3 items (if_setlladdr()). Any thoughts on differentiating between these two classes?

I'm mixed on this. I think it requires further thinking of how drivers and the stack interact, and how other things interact with the stack. if_setlladdr() for instance looks like it needs to be hardware and stack oriented, to keep everything in sync. Unless we make the KPI a "set" and "commit" style. Thoughts on that direction?

It certanly requires thinking. Also, there are also interaction inside the current stack that should be considered w.r.t KPI changes. Specifically, I mean the change notifications that needs to be propagated to the other parts of kernel and/or userland programs.
Set&commit may bring some benefits. I don't see any way that wouldn' add significant complexity both to the underlying implementation and the KPI users. I don't think it's worth the effort.

  1. Any thoughts on locking for setters?

Maybe for the next iteration of things? Right now just doing the naive approach to get the KPI in place and get drivers ported over.

Sure, I don't expect it to be the scope of this change - more curious abouth the thoughts on the general approach.

  1. Any plans to update ifnet(9)? :-)

Definitely need to do that. Thanks for reminding me.

That's great!

sys/net/if.c
424

Nit: if_init_idxtable() ?

4245

I'd try to have per-bit manipulation as the core KPI, with maybe the compat wrapper handling lower 32/64 bits.
Alternatively, maybe we can have a macro that accepts a static array of bit indexes ?
e.g.

static const uint8_t cap_bits[] = { IFCAP_RXCSUM_BIT, IFCAP_TOE6, IFCAP_TOE4 };
IF_SETCAPABILITIES(ifp, cap_bits)

?

4257

Nit: should we return const char * ?

4286

Awesome! If we're on the same page, probably worth doing it from day 1? What do you think?

4539

We can probably leave it as is for now and discuss epoch stuff separately.

jhibbits marked 6 inline comments as done.Dec 17 2022, 12:37 AM
jhibbits added inline comments.
sys/net/if.c
4245

kib in D32551 stated capabilities2 is for the 'nvlist' caps, so maybe it's better to leave setcapabilities/setcapabilitiesbit as-is, and add the new KPI for all new capabilities, to be used by the nvlist caps?

4257

Sure. Needed to finesse some other code to make it work.

4286

Consider it done in the next diff.

jhibbits updated this revision to Diff 114210.Dec 17 2022, 12:57 AM
jhibbits marked 2 inline comments as done.

Address more feedback from @melifaro.

Harbormaster completed remote builds in B48736: Diff 114210.Dec 17 2022, 12:57 AM
freebsd_igalic.co added a subscriber: freebsd_igalic.co.Dec 17 2022, 8:27 AM
jhibbits updated this revision to Diff 114287.Dec 19 2022, 6:13 PM

Change if_setcapababilities2, etc to if_setcapabilities_list(), with a single-capability option of if_setcapability().

Harbormaster completed remote builds in B48761: Diff 114287.Dec 19 2022, 6:13 PM
melifaro accepted this revision as: network, melifaro.Dec 20 2022, 10:36 AM

I apologise for the belated response.
Thank you for addressing the comments!
The only remaining thing I'd prefer to discuss a bit more is the setcaps2 thing. I'd rather prefer to move these bits to a dedicated review. It would be easier to iron out KPIs around that (I can come up with the one if you're okay with it).
The rest I'm totally fine with - and I'd love to see that landed.

sys/net/if.c
4245

Sorry for being picky on that.
I'm going to export all capabilities to userland via netlink interface info, in a bitarray parameter,

Netlink userland interface allows to get, set, append or remove certain bits from the capabilties.
Personally, from the stack standpoint the good KPI can be the following:

bool if_checkcap(ifp, cap_bit)
int if_setcapbit(ifp, cap_bit)
int if_clearcapbit(ifp, cap_bit)
void if_getcaparray(ifp, array_p, array_sz)
int if_setcaps(ifp, array_p, array_sz, bool clear)
This revision is now accepted and ready to land.Dec 20 2022, 10:36 AM
jhibbits added inline comments.Dec 20 2022, 4:13 PM
sys/net/if.c
4245

Only mlx5 currently uses if_capabilities2 from what I can tell, so it's not a big sticking point yet. I'll rip it out for further discussion and review.

Closed by commit rGeb1da3e52582: DrvAPI: Extend driver KPI with more accessors (authored by jhibbits). · Explain WhyDec 20 2022, 4:23 PM
This revision was automatically updated to reflect the committed changes.
jhibbits added a commit: rGeb1da3e52582: DrvAPI: Extend driver KPI with more accessors.

Revision Contents
Changeset List

PathSize
sys/
net/
5 lines
147 lines
64 lines

Diff 114326

View Options

sys/net/if.h

Loading...
View Options

sys/net/if.c

Loading...
View Options

sys/net/if_var.h

Loading...