Re: pluggable pktq_rps_hash()

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

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

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

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

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

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

- 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);

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

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