On Mon, Jan 15, 2018 at 9:47 PM, Prashant Bhole
<bhole_prashant...@lab.ntt.co.jp> wrote:
>
>
> On 1/16/2018 2:08 PM, Cong Wang wrote:
>>
>> On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
>> <bhole_prashant...@lab.ntt.co.jp> wrote:
>>>
>>> About bug:
>>> During init of clsact/ingress, it links qdisc's cpu_bstats,cpu_qstats
>>> with mini qdisc. TCQ_F_CPUSTATS is set in qdisc->flags during init and
>>> this flag is checked after init to allocate memory for stats.
>>>
>>> Hence mini qdisc points to null per-cpu-stats. The problem isn't caught
>>> while updating stats via mini qdisc because per_cpu_ptr(NULL, cpu_num)
>>> or this_cpu_ptr(NULL) gives a valid pointer.
>>>
>>> About fix:
>>> Currently stats memory is allocated at two places.
>>> - in qdisc_alloc() if TCQ_F_CPUSTATS is set in Qdisc_ops->static_flags
>>> - in qdisc_create() if TCQ_F_CPUSTATS is set in Qdisc->flags
>>>
>>> Qdisc_ops->static_flags is propagated to Qdisc->flags. So to fix this
>>> bug,
>>> we set TCQ_F_CPUSTATS in static flags and additional condition to avoid
>>> allocation after init.
>>
>>
>>
>> Good catch! One nit below.
>>
>>
>>>
>>> Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage
>>> of tp->q for clsact fastpath")
>>> Signed-off-by: Prashant Bhole <bhole_prashant...@lab.ntt.co.jp>
>>> ---
>>> net/sched/sch_api.c | 3 ++-
>>> net/sched/sch_ingress.c | 6 ++----
>>> 2 files changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>>> index 8a04c36e579f..de99a5e80944 100644
>>> --- a/net/sched/sch_api.c
>>> +++ b/net/sched/sch_api.c
>>> @@ -1094,7 +1094,8 @@ static struct Qdisc *qdisc_create(struct net_device
>>> *dev,
>>> goto err_out5;
>>> }
>>>
>>> - if (qdisc_is_percpu_stats(sch)) {
>>> + if (!(ops->static_flags & TCQ_F_CPUSTATS) &&
>>> + qdisc_is_percpu_stats(sch)) {
>>
>>
>> Instead of checking both flags, it is simpler to just check if
>> sch->cpu_bstats
>> and sch->cpu_qstats are NULL here?
>>
>> Also, this patch should go to -net tree, not just net-next, but -net
>> doesn't have ops->static_flags yet. ;) Given this, consider moving
>> the sch->cpu_bstats allocation before ops->init(), which might be
>> simpler.
>
>
> Cong,
> Thanks for reviewing. Based on this patch, Daniel submitted another patch
> for -net:
> http://patchwork.ozlabs.org/patch/861098/
Odd, I don't even see it in my inbox...
>
> Let me know if we still need v2 of my patch on -net-next (with your
> suggested nit picks).
It is okay, but it doesn't have to forward commit d59f5ffa59d8 to
net, reordering allocation before ->init() should be simpler.
