Re: pluggable pktq_rps_hash()

[Taylor R Campbell <campbell+netbsd-tech-net%mumble.net@localhost>]

> Date: Fri, 8 Oct 2021 14:56:03 +0900
> From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
> 
> On 2021/10/07 18:26, Taylor R Campbell wrote:
> > - In the sysctl definitions, no need for explicit (void *) cast.
> 
> Yes, however, that causes following build failure.
> ====================
> [...]
> /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...

I see -- it's because you defined it with CTLTYPE_STRING, but the
object stored in the kernel and passed to sysctl_pktq_rps_hash_handler
isn't actually a string.  So the (void *) cast is correct after all.

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

OK, thanks.  This sysctl(9) API sure is difficult to understand...

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

I guess so!

> 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!  One tiny nit left: why pktq_rps_hash(&pktq_rps_hash_default,
...) in dev/pci/xmm7360.c while all the others get their own sysctl?
Otherwise, LGTM.

(A legitimate answer is `I don't have the hardware to test changes to
xmm7360.c, so someone else will have to do it.')

(Maybe wg(4) should also have the pktq_rps_hash sysctl treatment too;
for some reason I don't remember I (or maybe Ozaki-san?) just wrote
curcpu()->ci_index with a comment `// pktq_rps_hash(m)'.)