| Message ID | 20180126022624.20442-4-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net_sched: reflect tx_queue_len change for pfifo_fast | expand |
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
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.
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.
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.
On Fri, Jan 26, 2018 at 6:10 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > This part? Yes, dev_deactivate() as you quote. > > + 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. I really to hate to point it out again we have RTNL here. You missed my previous response to John. ;) Please read v1 and v2 when you response to v3.
On 2018年01月26日 12:01, Cong Wang wrote: >> 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. Aha, ok, I think I get the synchronize_net() trick in dev_deactivate_many(). Thanks
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, };
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(+)