DrvAPI: Extend driver KPI with more accessors

[jhibbits]

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

[jhibbits]

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

[melifaro]

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?

2. Any thoughts on locking for setters?

3. 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.

4286

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

4315

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

4442

Does it need to have the return code?

4568

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.

4779

Nit: worth having input_cb signature separately.

[jhibbits]

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?

2. 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.

3. 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?

4286

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.

4315

I like this idea.

4442

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

4779

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

[jhibbits]

sys/net/if.c

4568

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]

Address feedback from @melifaro.

• Switch to if_name() from if_getxname() (makes more sense)
• Add function typedefs for members, to be used in accessors.

[melifaro]

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

2. 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.

3. 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)

?

4286

Nit: should we return const char * ?

4315

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

4568

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

[jhibbits]

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?

4286

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

4315

Consider it done in the next diff.

[jhibbits]

Address more feedback from @melifaro.

[jhibbits]

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

[melifaro]

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)

[jhibbits]

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.