Re: pluggable pktq_rps_hash()

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

Hi,

Thank you for your comments.

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

Exactly.  Moreover, I am not sure which sysctl entry should be used
"net.wwan.rps_hash", "hw.xmm7360.rps_hash", or something else.


> (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)'.)

Ozaki-san says he don't know the comment and the subsequent pktq_enqueue().
Hmm, I think wg(4) may require more than one pktq_rps_hash sysctl.


Thanks,

On 2021/10/08 18:33, Taylor R Campbell wrote:
> > 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)'.)

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

Device Engineering Section,
Product Division,
Technology Unit

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