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

[John Fastabend <john.fastabend@gmail.com>]

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@intel.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