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 16:10:02 +0200 Message-ID: <20180126160850-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: Jason Wang , Linux Kernel Network Developers , John Fastabend To: Cong Wang Return-path: Received: from mx1.redhat.com ([209.132.183.28]:49978 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751324AbeAZOKK (ORCPT ); Fri, 26 Jan 2018 09:10:10 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Jan 25, 2018 at 08:01:42PM -0800, Cong Wang wrote: > On Thu, Jan 25, 2018 at 7:57 PM, 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? > > Yes, we sync with dequeue path before calling ->change_tx_queue_len(). > I already mentioned this in patch 2/3. This part? + bool up = dev->flags & IFF_UP; + unsigned int i; + int ret = 0; + + if (up) + dev_deactivate(dev); + + for (i = 0; i < dev->num_tx_queues; i++) { + ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i]); + + /* TODO: revert changes on a partial failure */ + if (ret) + break; + } + + if (up) + dev_activate(dev); I wonder whether it really is safe to read dev->flags like that without any locks. -- MST