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

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

[roy.qing.li]

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

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index e39239e..5ad7d5f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1279,7 +1279,7 @@ int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 	unsigned int hash = ipv6_addr_hash(addr);
 
 	rcu_read_lock_bh();
-	hlist_for_each_entry_rcu(ifp, node, &inet6_addr_lst[hash], addr_lst) {
+	hlist_for_each_entry_rcu_bh(ifp, node, &inet6_addr_lst[hash], addr_lst) {
 		if (!net_eq(dev_net(ifp->idev->dev), net))
 			continue;
 		if (ipv6_addr_equal(&ifp->addr, addr) &&
@@ -1303,7 +1303,7 @@ static bool ipv6_chk_same_addr(struct net *net, const struct in6_addr *addr,
 	struct inet6_ifaddr *ifp;
 	struct hlist_node *node;
 
-	hlist_for_each_entry(ifp, node, &inet6_addr_lst[hash], addr_lst) {
+	hlist_for_each_entry_rcu_bh(ifp, node, &inet6_addr_lst[hash], addr_lst) {
 		if (!net_eq(dev_net(ifp->idev->dev), net))
 			continue;
 		if (ipv6_addr_equal(&ifp->addr, addr)) {
-- 
1.7.1

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

[David Miller]

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.

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

[RongQing Li]

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

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

[David Miller]

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.

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

[RongQing Li]

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.

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

[David Miller]

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.

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

[RongQing Li]

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