Re: [PATCH net-next 1/3] net: provide macros for commonly copied lockless queue stop/wake code

[Jakub Kicinski <kuba@kernel.org>]

On Sat, 1 Apr 2023 17:04:17 +0200 Heiner Kallweit wrote:
> > +#define netif_tx_queue_try_stop(txq, get_desc, start_thrs)		\
> > +	({								\
> > +		int _res;						\
> > +									\
> > +		netif_tx_stop_queue(txq);				\
> > +									\
> > +		smp_mb();						\  
> 
> Wouldn't a smp_mb__after_atomic() be sufficient here, because netif_tx_stop_queue()
> includes a set_bit()? At least on X86 this would result in a no-op.

Yup, good point, __after_atomic() should be perfectly fine. I didn't
think too much about the smp_mb() 'cause it's what existing code was
using and it's a slow path. I'll fix in v3.

> > +									\
> > +		/* We need to check again in a case another		\
> > +		 * CPU has just made room available.			\
> > +		 */							\
> > +		_res = 0;						\
> > +		if (unlikely(get_desc >= start_thrs)) {			\
> > +			netif_tx_start_queue(txq);			\
> > +			_res = -1;					\
> > +		}							\
> > +		_res;							\
> > +	})								\
> > +
> > +/**
> > + * netif_tx_queue_maybe_stop() - locklessly stop a Tx queue, if needed
> > + * @txq:	struct netdev_queue to stop/start
> > + * @get_desc:	get current number of free descriptors (see requirements below!)
> > + * @stop_thrs:	minimal number of available descriptors for queue to be left
> > + *		enabled
> > + * @start_thrs:	minimal number of descriptors to re-enable the queue, can be
> > + *		equal to @stop_thrs or higher to avoid frequent waking
> > + *
> > + * All arguments may be evaluated multiple times, beware of side effects.
> > + * @get_desc must be a formula or a function call, it must always
> > + * return up-to-date information when evaluated!
> > + * Expected to be used from ndo_start_xmit, see the comment on top of the file.
> > + *
> > + * Returns:
> > + *	 0 if the queue was stopped
> > + *	 1 if the queue was left enabled
> > + *	-1 if the queue was re-enabled (raced with waking)
> > + */
> > +#define netif_tx_queue_maybe_stop(txq, get_desc, stop_thrs, start_thrs)	\
> > +	({								\
> > +		int _res;						\
> > +									\
> > +		_res = 1;						\
> > +		if (unlikely(get_desc < stop_thrs))			\
> > +			_res = netif_tx_queue_try_stop(txq, get_desc,	\
> > +						       start_thrs);	\
> > +		_res;							\
> > +	})								\
> > +
> > +#define __netif_tx_queue_try_wake(txq, get_desc, start_thrs, down_cond) \  
> 
> Maybe I miss something, but: Why the get_desc and start_thrs parameters
> if they aren't used?

Copy'n'paste fail, will fix :(