lkml.org 
[lkml]   [2009]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] net: introduce a list of device addresses dev_addr_list (v3)
Fri, Apr 17, 2009 at 05:33:15PM CEST, shemminger@vyatta.com wrote:

<snip>

>> +struct netdev_hw_addr {
>> + struct list_head list;
>> + unsigned char addr[MAX_ADDR_LEN];
>> + int refcount;
>> + struct rcu_head rcu_head;
>> +};
>
>Minor nit, the ordering of elements cause holes that might not be
>needed.

Agree that ordering might be done better. Will do.
>
>Space saving? is rcu_head needed or would using synchronize_net
>make code cleaner and save space.
>

Well I originaly had this done by synchronize_rcu(). Eric argued that it might
cause especially __hw_addr_del_multiple_ii() to run long and suggested to use
call_rcu() instead. I plan to switch this to kfree_rcu() (or whatever it's
called) once it hits the tree.

<snip>

>> + ha = kzalloc(max(sizeof(*ha), L1_CACHE_BYTES), GFP_ATOMIC);
>> + if (!ha)
>> + return -ENOMEM;
>Since you are initializing all fields, kzalloc isn't really needed

Noted.
>
>> + memcpy(ha->addr, addr, addr_len);
>> + ha->refcount = 1;
>> + list_add_tail_rcu(&ha->list, list);
>> + return 0;
>> +}

<snip>

>> +static void dev_addr_flush(struct net_device *dev)
>> +{
>> + ASSERT_RTNL();
>> +
>Since this is local you should be able to audit all
>the callers and remove this ASSERT.

Okay. I will at least put a comment instead of this.
>
>> + __hw_addr_flush(&dev->dev_addr_list);
>> + dev->dev_addr = NULL;
>> +}
>> +
>> +static int dev_addr_init(struct net_device *dev)
>> +{
>> + unsigned char addr[MAX_ADDR_LEN];
>> + struct netdev_hw_addr *ha;
>> + int err;
>> +
>> + ASSERT_RTNL();
>Ditto, ASSERT_RTNL makes sense for exposed kernel API and
>initial testing.
>
>> + INIT_LIST_HEAD(&dev->dev_addr_list);
>> + memset(addr, 0, sizeof(*addr));
>> + err = __hw_addr_add(&dev->dev_addr_list, addr, sizeof(*addr));
>> + if (!err) {
>> + /*
>> + * Get the first (previously created) address from the list
>> + * and set dev_addr pointer to this location.
>> + */
>> + ha = list_first_entry(&dev->dev_addr_list,
>> + struct netdev_hw_addr, list);
>> + dev->dev_addr = ha->addr;
>> + }
>> + return err;
>> +}

<snip>



\
 
 \ /
  Last update: 2009-04-18 09:07    [W:0.186 / U:1.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site