[ofa-general] Re: [PATCH 03/10] dev.c changes.

[Patrick McHardy]

Krishna Kumar2 wrote:
> Hi Patrick,
>
> Thanks for your comments.
>
> Patrick McHardy <kaber at trash.net> wrote on 07/20/2007 03:34:30 PM:
>
>   
>> The queue length can be changed through multiple interfaces, if that
>> really is important you need to catch these cases too.
>>     
>
> I have a TODO comment in net-sysfs.c which is to catch this case.
>   

I noticed that. Still wondering why it is important at all though.

>   
>>> +      } else {
>>> +         dev->skb_blist = kmalloc(sizeof *dev->skb_blist,
>>> +                   GFP_KERNEL);
>>>       
>> Why not simply put the head in struct net_device? It seems to me that
>> this could also be used for gso_skb.
>>     
>
> Without going into GSO, it is wasting some 32 bytes on i386 since most
> drivers don't export this API.
>   

32 bytes? I count 16, - 4 for the pointer, so its 12 bytes of waste.
If you'd use it for gso_skb it would come down to 8 bytes. struct
net_device is a pig already, and there are better ways to reduce this
than starting to allocating single members with a few bytes IMO.

>   
>> Queue purging should be done in dev_deactivate.
>>     
>
> I originally had it in dev_deactivate, but when I did a ifdown eth0, ifup
> eth0,
> the system panic'd. The first solution I thought was to initialize the
> skb_blist
> in dev_change_flags() rather than in register_netdev(), but then felt that
> a
> series of ifup/ifdown will unnecessarily check stuff/malloc/free/initialize
> stuff,
> and so thought of putting it in unregister_netdev (where it is balanced
> with
> register_netdev).
>
> Is there any reason to move this ?
>   

Yes, packets can be holding references to various stuff and
these should be released on device down. As I said above I
don't really like the allocation, but even if you want to
keep it, just do the purging and dev_deactivate and keep the
freeing in unregister_netdev (actually I guess it should be
free_netdev to handle register_netdevice errors).