On Thu, Jan 25, 2018 at 08:01:42PM -0800, Cong Wang wrote:
> On Thu, Jan 25, 2018 at 7:57 PM, Jason Wang <jasow...@redhat.com> 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.fastab...@gmail.com>
> >> Signed-off-by: Cong Wang <xiyou.wangc...@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?
>
> 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
