Re: [Patch net-next v3 3/3] net_sched: implement ->change_tx_queue_len() for pfifo_fast

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jan 26, 2018 at 11:57:59AM +0800, Jason Wang wrote:
> 
> 
> On 2018年01月26日 10:26, Cong Wang wrote:
> > pfifo_fast used to drop based on qdisc_dev(qdisc)->tx_queue_len,
> > so we have to resize skb array when we change tx_queue_len.
> > 
> > Other qdiscs which read tx_queue_len are fine because they
> > all save it to sch->limit or somewhere else in qdisc during init.
> > They don't have to implement this, it is nicer if they do so
> > that users don't have to re-configure qdisc after changing
> > tx_queue_len.
> > 
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
> >   net/sched/sch_generic.c | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> > 
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index 08f9fa27e06e..190570f21b20 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -763,6 +763,23 @@ static void pfifo_fast_destroy(struct Qdisc *sch)
> >   	}
> >   }
> > +static int pfifo_fast_change_tx_queue_len(struct Qdisc *sch,
> > +					  unsigned int new_len)
> > +{
> > +	struct pfifo_fast_priv *priv = qdisc_priv(sch);
> > +	struct skb_array *bands[PFIFO_FAST_BANDS];
> > +	int prio;
> > +
> > +	for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
> > +		struct skb_array *q = band2list(priv, prio);
> > +
> > +		bands[prio] = q;
> > +	}
> > +
> > +	return skb_array_resize_multiple(bands, PFIFO_FAST_BANDS, new_len,
> > +					 GFP_KERNEL);
> > +}
> > +
> >   struct Qdisc_ops pfifo_fast_ops __read_mostly = {
> >   	.id		=	"pfifo_fast",
> >   	.priv_size	=	sizeof(struct pfifo_fast_priv),
> > @@ -773,6 +790,7 @@ struct Qdisc_ops pfifo_fast_ops __read_mostly = {
> >   	.destroy	=	pfifo_fast_destroy,
> >   	.reset		=	pfifo_fast_reset,
> >   	.dump		=	pfifo_fast_dump,
> > +	.change_tx_queue_len =  pfifo_fast_change_tx_queue_len,
> >   	.owner		=	THIS_MODULE,
> >   	.static_flags	=	TCQ_F_NOLOCK | TCQ_F_CPUSTATS,
> >   };
> 
> Is __skb_array_empty() in pfifo_fast_dequeue() still safe after this change?
> 
> Thanks

I think it isn't.  If you want to use resize *and* use unlocked variants,
you must lock all producers and consumers when resizing yourself.

-- 
MST