Re: [PATCH V3 net-next 3/5] xen-netfront: Factor queue-specific data into queue struct.

[Andrew Bennieston <andrew.bennieston@citrix.com>]

On 14/02/14 18:04, David Vrabel wrote:
> On 14/02/14 17:35, Andrew J. Bennieston wrote:
>> From: "Andrew J. Bennieston" <andrew.bennieston@citrix.com>
>>
>> In preparation for multi-queue support in xen-netfront, move the
>> queue-specific data from struct netfront_info to struct netfront_queue,
>> and update the rest of the code to use this.
>>
>> Also adds loops over queues where appropriate, even though only one is
>> configured at this point, and uses alloc_etherdev_mq() and the
>> corresponding multi-queue netif wake/start/stop functions in preparation
>> for multiple active queues.
>>
>> Finally, implements a trivial queue selection function suitable for
>> ndo_select_queue, which simply returns 0, selecting the first (and
>> only) queue.
> [...]
>> --- a/drivers/net/xen-netfront.c
>> +++ b/drivers/net/xen-netfront.c
> [...]
>> @@ -2048,17 +2196,27 @@ static const struct xenbus_device_id netfront_ids[] = {
> [...]
>> +	for (i = 0; i < info->num_queues; ++i) {
>> +		queue = &info->queues[i];
>> +		del_timer_sync(&queue->rx_refill_timer);
>> +	}
>> +
>> +	if (info->num_queues) {
>> +		kfree(info->queues);
>> +		info->queues = NULL;
>> +	}
>> +
>>   	xennet_sysfs_delif(info->netdev);
>>
>>   	unregister_netdev(info->netdev);
>>
>> -	del_timer_sync(&info->rx_refill_timer);
>> -
>
> This has reordered the del_timer_sync() to before the
> unregister_netdev() call.
>
> Can you be sure that the timer cannot be restarted after deleting it?
>
> David
>
Looking at the code, mod_timer() is called from 
xennet_alloc_rx_buffers(), only. This, in turn, is called from 
xennet_poll, which is the registered NAPI handler function. This should 
not be called after a napi_disable(), which is done in xennet_close(). 
xennet_close() is called to stop the interface, which should be done 
before the module is removed (unless I'm mistaken here). So this should 
be safe.

That said, there is no reason that the queue cleanup has to happen 
before the unregister_netdev() call. I'll move it to after that point, 
just to be safe.

-Andrew