netdev - Re: [net-next PATCH 05/15] net: sched: a dflt qdisc may be used with per cpu stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Aug 2016 10:13:18 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     jhs@...atatu.com, davem@...emloft.net, brouer@...hat.com,
        xiyou.wangcong@...il.com, alexei.starovoitov@...il.com,
        john.r.fastabend@...el.com, netdev@...r.kernel.org
Subject: Re: [net-next PATCH 05/15] net: sched: a dflt qdisc may be used with
 per cpu stats

On 16-08-24 09:41 AM, Eric Dumazet wrote:
> On Tue, 2016-08-23 at 13:24 -0700, John Fastabend wrote:
>> Enable dflt qdisc support for per cpu stats before this patch a
>> dflt qdisc was required to use the global statistics qstats and
>> bstats.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
>> ---
>>  net/sched/sch_generic.c |   24 ++++++++++++++++++++----
>>  1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index 80544c2..910b4d15 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -646,18 +646,34 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
>>  	struct Qdisc *sch;
>>  
>>  	if (!try_module_get(ops->owner))
>> -		goto errout;
>> +		return NULL;
>>  
>>  	sch = qdisc_alloc(dev_queue, ops);
>>  	if (IS_ERR(sch))
>> -		goto errout;
>> +		return NULL;
>>  	sch->parent = parentid;
>>  
>> -	if (!ops->init || ops->init(sch, NULL) == 0)
>> +	if (!ops->init)
>>  		return sch;
>>  
>> -	qdisc_destroy(sch);
>> +	if (ops->init(sch, NULL))
>> +		goto errout;
>> +
>> +	/* init() may have set percpu flags so init data structures */
>> +	if (qdisc_is_percpu_stats(sch)) {
>> +		sch->cpu_bstats =
>> +			netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu);
>> +		if (!sch->cpu_bstats)
>> +			goto errout;
>> +
>> +		sch->cpu_qstats = alloc_percpu(struct gnet_stats_queue);
>> +		if (!sch->cpu_qstats)
>> +			goto errout;
>> +	}
>> +
> 
> Why are you attempting these allocations here instead of qdisc_alloc()
> 
> This looks weird, I would expect base qdisc being fully allocated before
> ops->init() is attempted.
> 
> 
> 

I could fully allocate it in qdisc_alloc() but we don't know if the
qdisc needs per cpu data structures until after the init call. So it
would sit unused in those cases if done from qdisc_alloc(). It seems
best to me at least to just avoid the allocation in qdisc_alloc() and
do it after init like I did here.

Perhaps it would be nice to pull these into a function call
post_init_qdisc_alloc() that does all this allocation?

.John