From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [Patch net-next v3 3/3] net_sched: implement ->change_tx_queue_len() for pfifo_fast Date: Fri, 26 Jan 2018 15:48:15 +0200 Message-ID: <20180126154618-mutt-send-email-mst@kernel.org> References: <20180126022624.20442-1-xiyou.wangcong@gmail.com> <20180126022624.20442-4-xiyou.wangcong@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Cong Wang , netdev@vger.kernel.org, john.fastabend@gmail.com To: Jason Wang Return-path: Received: from mx1.redhat.com ([209.132.183.28]:44939 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752256AbeAZNsQ (ORCPT ); Fri, 26 Jan 2018 08:48:16 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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 > > Signed-off-by: Cong Wang > > --- > > 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