Fri, Apr 17, 2009 at 05:33:15PM CEST, shemminger@xxxxxxxxxx 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>
_______________________________________________
Bridge mailing list
Bridge@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/bridge
