Re: [PATCH 1/3] net: introduce a list of device addressesdev_addr_list (v3)

[Jiri Pirko]

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>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/