Re: [RFC PATCH v2 08/10] net: sched: pfifo_fast use alf_queue

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Wed, 13 Jul 2016 23:23:12 -0700
John Fastabend <john.fastabend@gmail.com> wrote:

> This converts the pfifo_fast qdisc to use the alf_queue enqueue and
> dequeue routines then sets the NOLOCK bit.

Should have said skb_array, not alf_queue ;-)

 > This also removes the logic used to pick the next band to dequeue from
> and instead just checks each alf_queue for packets from top priority
> to lowest. This might need to be a bit more clever but seems to work
> for now.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>  net/sched/sch_generic.c |  131 +++++++++++++++++++++++++++--------------------
>  1 file changed, 75 insertions(+), 56 deletions(-)
> 
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 7dcd066..2ac3eb9 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -26,6 +26,7 @@
>  #include <linux/list.h>
>  #include <linux/slab.h>
>  #include <linux/if_vlan.h>
> +#include <linux/skb_array.h>
>  #include <net/sch_generic.h>
>  #include <net/pkt_sched.h>
>  #include <net/dst.h>
> @@ -555,88 +556,79 @@ static const u8 prio2band[TC_PRIO_MAX + 1] = {
>  
>  /*
>   * Private data for a pfifo_fast scheduler containing:
> - * 	- queues for the three band
> - * 	- bitmap indicating which of the bands contain skbs
> + *	- rings for priority bands
>   */
>  struct pfifo_fast_priv {
> -	u32 bitmap;
> -	struct sk_buff_head q[PFIFO_FAST_BANDS];
> +	struct skb_array q[PFIFO_FAST_BANDS];
>  };
>  
> -/*
> - * Convert a bitmap to the first band number where an skb is queued, where:
> - * 	bitmap=0 means there are no skbs on any band.
> - * 	bitmap=1 means there is an skb on band 0.
> - *	bitmap=7 means there are skbs on all 3 bands, etc.
> - */
> -static const int bitmap2band[] = {-1, 0, 1, 0, 2, 0, 1, 0};
> -
> -static inline struct sk_buff_head *band2list(struct pfifo_fast_priv *priv,
> -					     int band)
> +static inline struct skb_array *band2list(struct pfifo_fast_priv *priv,
> +					  int band)
>  {
> -	return priv->q + band;
> +	return &priv->q[band];
>  }
>  
>  static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc *qdisc,
>  			      struct sk_buff **to_free)
>  {
> -	if (skb_queue_len(&qdisc->q) < qdisc_dev(qdisc)->tx_queue_len) {
> -		int band = prio2band[skb->priority & TC_PRIO_MAX];
> -		struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
> -		struct sk_buff_head *list = band2list(priv, band);
> -
> -		priv->bitmap |= (1 << band);
> -		qdisc->q.qlen++;
> -		return __qdisc_enqueue_tail(skb, qdisc, list);
> -	}
> +	int band = prio2band[skb->priority & TC_PRIO_MAX];
> +	struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
> +	struct skb_array *q = band2list(priv, band);
> +	int err;
>  
> -	return qdisc_drop(skb, qdisc, to_free);
> +	err = skb_array_produce_bh(q, skb);

Do you need the _bh variant here?  (Doesn't the qdisc run with BH disabled?)


> +
> +	if (unlikely(err))
> +		return qdisc_drop_cpu(skb, qdisc, to_free);
> +
> +	qdisc_qstats_cpu_qlen_inc(qdisc);
> +	qdisc_qstats_cpu_backlog_inc(qdisc, skb);
> +	return NET_XMIT_SUCCESS;
>  }
>  
>  static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
>  {
>  	struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
> -	int band = bitmap2band[priv->bitmap];
> +	struct sk_buff *skb = NULL;
> +	int band;
>  
> -	if (likely(band >= 0)) {
> -		struct sk_buff_head *list = band2list(priv, band);
> -		struct sk_buff *skb = __qdisc_dequeue_head(qdisc, list);
> +	for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) {
> +		struct skb_array *q = band2list(priv, band);
>  
> -		qdisc->q.qlen--;
> -		if (skb_queue_empty(list))
> -			priv->bitmap &= ~(1 << band);
> +		if (__skb_array_empty(q))
> +			continue;
>  
> -		return skb;
> +		skb = skb_array_consume_bh(q);

Also _bh variant.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer