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

Jesper Dangaard Brouer
2016-07-14T15:11:53Z

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