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

[John Fastabend <john.fastabend@gmail.com>]

On 16-07-14 08:11 AM, Jesper Dangaard Brouer wrote:
> 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 ;-)

I think I'll get numbers for the alf_queue to so we can compare them
as well. On the todo list for tomorrow.

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

Yep its inside rcu_read_lock_bh().