[next-net] ipv6: Use hlist_for_each_entry_rcu_bh() in ipv6_chk_same_addr()

From: RongQing.Li <roy.qing.li@gmail.com>

Replace hlist_for_each_entry and hlist_for_each_entry_rcu with
hlist_for_each_entry_rcu_bh() in ipv6_chk_same_addr and
ipv6_chk_addr to keep that all dereference methods for addr_list
are same, and take advantage of _rcu_bh() critical section
checking and prevention from compiler merging or refetching.

Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
---
 net/ipv6/addrconf.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

From: roy.qing.li@gmail.com
Date: Tue,  1 Nov 2011 13:30:55 +0800

> From: RongQing.Li <roy.qing.li@gmail.com>
> 
> Replace hlist_for_each_entry and hlist_for_each_entry_rcu with
> hlist_for_each_entry_rcu_bh() in ipv6_chk_same_addr and
> ipv6_chk_addr to keep that all dereference methods for addr_list
> are same, and take advantage of _rcu_bh() critical section
> checking and prevention from compiler merging or refetching.
> 
> Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>

Callers are already in an RCU section with BH disabled when these
functions are invoked.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2011/11/1 David Miller <davem@davemloft.net>:
> From: roy.qing.li@gmail.com
> Date: Tue,  1 Nov 2011 13:30:55 +0800
>
>> From: RongQing.Li <roy.qing.li@gmail.com>
>>
>> Replace hlist_for_each_entry and hlist_for_each_entry_rcu with
>> hlist_for_each_entry_rcu_bh() in ipv6_chk_same_addr and
>> ipv6_chk_addr to keep that all dereference methods for addr_list
>> are same, and take advantage of _rcu_bh() critical section
>> checking and prevention from compiler merging or refetching.
>>
>> Signed-off-by: RongQing.Li <roy.qing.li@gmail.com>
>
> Callers are already in an RCU section with BH disabled when these
> functions are invoked.
>

Yes, But I think the code readable is not good,
sometime, call hlist_for_each_entry, sometime
hlist_for_each_entry_rcu, sometime hlist_for_each_entry_rcu_bh.

I think the RCU pair should be :

rcu_read_lock_bh
 hlist_for_each_entry_rcu_bh(...)
rcu_read_unlock_bh

at the same time,   hlist_for_each_entry can not prevent compiler from
merging or refetching.

-Roy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: RongQing Li <roy.qing.li@gmail.com>
Date: Tue, 1 Nov 2011 16:33:49 +0800

> Yes, But I think the code readable is not good,

It is wasteful to add multiple BH disables and RCU memory
barriers in code paths where it is not necessary.

Your patch fixes no real bug, and adds a regression.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2011/11/1 David Miller <davem@davemloft.net>:
> From: RongQing Li <roy.qing.li@gmail.com>
> Date: Tue, 1 Nov 2011 16:33:49 +0800
>
>> Yes, But I think the code readable is not good,
>
> It is wasteful to add multiple BH disables and RCU memory
> barriers in code paths where it is not necessary.
>
> Your patch fixes no real bug, and adds a regression.
>

hlist_for_each_entry_rcu_bh() does not disable BH,
it only check If BH is disabled.

These codes is different from normal RCU convention.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: RongQing Li <roy.qing.li@gmail.com>
Date: Tue, 1 Nov 2011 17:05:19 +0800

> 2011/11/1 David Miller <davem@davemloft.net>:
>> From: RongQing Li <roy.qing.li@gmail.com>
>> Date: Tue, 1 Nov 2011 16:33:49 +0800
>>
>>> Yes, But I think the code readable is not good,
>>
>> It is wasteful to add multiple BH disables and RCU memory
>> barriers in code paths where it is not necessary.
>>
>> Your patch fixes no real bug, and adds a regression.
>>
> 
> hlist_for_each_entry_rcu_bh() does not disable BH,
> it only check If BH is disabled.
> 
> These codes is different from normal RCU convention.

But adding _rcu does add memory barriers.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2011/11/1 David Miller <davem@davemloft.net>:
> From: RongQing Li <roy.qing.li@gmail.com>
> Date: Tue, 1 Nov 2011 17:05:19 +0800
>
>> 2011/11/1 David Miller <davem@davemloft.net>:
>>> From: RongQing Li <roy.qing.li@gmail.com>
>>> Date: Tue, 1 Nov 2011 16:33:49 +0800
>>>
>>>> Yes, But I think the code readable is not good,
>>>
>>> It is wasteful to add multiple BH disables and RCU memory
>>> barriers in code paths where it is not necessary.
>>>
>>> Your patch fixes no real bug, and adds a regression.
>>>
>>
>> hlist_for_each_entry_rcu_bh() does not disable BH,
>> it only check If BH is disabled.
>>
>> These codes is different from normal RCU convention.
>
> But adding _rcu does add memory barriers.
>

Yes,  But I hope we can keep this convention.

Like the below similar codes, which exists in everywhere,
If we  replace rcu_dereference_bh with dereference ..
it is not good solution, though we can reduce  memory barriers

	rcu_read_lock_bh();

	txq = dev_pick_tx(dev, skb);
	q = rcu_dereference_bh(txq->qdisc);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html