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

Krishna Kumar2 wrote:

Hi Patrick,

Thanks for your comments.

Patrick McHardy [off-list ref] wrote on 07/20/2007 03:34:30 PM:

  

quoted

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.

quoted

quoted

+      } 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.

quoted

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).