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

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

On Thu, Jan 25, 2018 at 08:01:42PM -0800, Cong Wang wrote:
> On Thu, Jan 25, 2018 at 7:57 PM, Jason Wang <jasowang@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.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?
> 
> 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