Re: pluggable pktq_rps_hash()

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

Thank you for your reviewing!

On 2021/10/07 18:26, Taylor R Campbell wrote:
> > Date: Thu, 7 Oct 2021 12:06:08 +0900
> > From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
> >
> > On 2021/09/24 13:27, Kengo NAKAHARA wrote:
> > > Hi,
> > >
> > > I implement some of pktq_rps_hash() and make pluggable it for each
> > > interface type such as ether, pppoe, gif, and ipsecif.
> > > Here is the patch
> > >       https://github.com/knakahara/netbsd-src/commit/68a43105b541aec79c6f067e81dbe34ad2282dec
> > >
> > > Of course, the default behavior is not changed.  We can change RPS
> > > behavior by "sysctl -w net.{ether,pppoe,gif,ipsecif}.rps_hash=${value}".
> > > Currently, the values are
> > >       - zero : always return 0
> > >       - curcpu : return current cpuid
> > >       - toeplitz : return toeplitz hash value which calculated from mbuf
> > >
> > > I think it is easy to add new rps hash functions to the above patch
> > > and to apply the pluggable rps hash function to other interfaces.
> > >
> > > Could you comment the patch?
>
> Looks reasonable!  Some minor comments:
>
> - Where you do atomic_swap_ptr(func, ...) and discard the result, you
>    should just do atomic_store_relaxed(func, ...) instead.  Cheaper way
>    to get the same effect.

I see.  I fix it.


> - pktqueue.c, pktq_rps_hash_toeplitz, line 266:
> 	}
> 	else if (...) {
>    should be
> 	} else if (...) {

Oh, I fix it.


> - In the sysctl definitions, no need for explicit (void *) cast.

Yes, however, that causes following build failure.
====================
if_ethersubr.c:1772:43: error: passing argument 1 of '__sysctl_verify_CTLTYPE_STRING_arg' from incompatible pointer type [-Werror=incompatible-pointer-types]
 1772 |          sysctl_pktq_rps_hash_handler, 0, &ether_pktq_rps_hash_p,
      |                                           ^~~~~~~~~~~~~~~~~~~~~~
      |                                           |
      |                                           uint32_t (**)(const struct mbuf *) {aka unsigned int (**)(const struct mbuf *)}
/disk2/home/k-nakahara/repos/netbsd-src/sys/sys/sysctl.h:1168:35: note: in definition of macro 'sysctl_createv'
 1168 |      __sysctl_verify_##type##_arg(newp), __VA_ARGS__)
      |                                   ^~~~
/disk2/home/k-nakahara/repos/netbsd-src/sys/sys/sysctl.h:1152:42: note: expected 'char *' but argument is of type 'uint32_t (**)(const struct mbuf *)' {aka 'unsigned int (**)(const struct mbuf *)'}
 1152 | __sysctl_verify_##ctl_type##_arg(c_type *arg) \
====================

https://nxr.netbsd.org/xref/src/sys/sys/sysctl.h#1159
I think the above verifying macro is wrong, hmm, it should be fixed
another time...


> - Indentation in the sysctl_createv calls is a little confusing -- I
>    suggest putting `CTL_CREATE, CTL_EOL' on its own line to make it
>    clear that `PKTQ_RPS_HASH_NAME_LEN' is not actually part of the
>    sysctl mib.

I see.


>    (Is it necessary to pass PKTQ_RPS_HASH_NAME_LEN, anyway?  A lot of
>    sysctl_createv calls just pass zero here.  I can't tell from the man
>    page what it does.)

Yes, because the "flags" of sysctl_createv is CTLFLAG_READWRITE.  "newlen"
argument is used to set buffer size from userland.  A lot of sysctl which
calls pass zero would have CTLFLAG_READONLY "flags".


> - pktqueue.c, sysctl_pktq_rps_hash_handler, lines 320 and 325: no need
>    for a stack buffer to copy the type name into; can just do
>
> 	const char *type;
> 	...
> 	type = pktq_get_rps_hash_type(*func);

I think that is required by sysctl_lookup() to pass old value to userland.
Sorry if I am wrong.


> - Instead of copying & pasting the conditional default everywhere,
>
> 	static pktq_rps_hash_func_t xyzif_pktq_rps_hash_p =
> 	#ifdef NET_MPSAFE
> 		pktq_rps_hash_curcpu;
> 	#else
> 		pktq_rps_hash_zero;
> 	#endif
>
>    how about defining a default in pktqueue.c/h and using that
>    everywhere?
>
> 	static pktq_rps_hash_func_t xyzif_pktq_rps_hash_p =
> 	    pktq_rps_hash_default;
>
>    Or is there a reason to make the default subsystem-specific?

No.  I update as you suggest.


> - Could avoid writing lots of subsystem-specific wrapper functions
>    like ether_pktq_rps_hash by defining a function to call the
>    subsystem-specific pktq rps hash function:
>
> 	/* pktqueue.c */
> 	uint32_t
> 	pktq_rps_hash(pktq_rps_hash_func_t *funcp, struct mbuf *m)
> 	{
> 		pktq_rps_hash_func_t func = atomic_load_relaxed(funcp);
>
> 		KASSERT(func != NULL);
> 	
> 		return (*func)(m);
> 	}
>
> 	/* if_ethersubr.c */
> 		const uint32_t h = pktq_rps_hash(&ether_pktq_rps_hash_p, m);
>
>    This would also avoid triggering sanitizers which object to loading
>    ether_pktq_rps_hash_p with a non-atomic load.  (Not actually a
>    problem on any CPUs we care about, but helps reduce false alarms
>    from sanitizers.)
>
>    This would also give us more flexibility if the pktq rps hash had
>    more state, e.g. maybe a random seed alongside it.  (Not sure that's
>    an important avenue of flexibility, but it keeps the API details
>    more neatly confined to pktqueue.c.)

I see. I apply it.

I rebase and update patch, here is the update diff,
    https://github.com/knakahara/netbsd-src/commit/7ce7c9293782916c8a2e486ca6605911f4b4fea1
and here is unified diff with a little update,
    https://github.com/knakahara/netbsd-src/pull/8


Thanks,

--
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>