* [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path
@ 2013-09-05 7:48 Ding Tianhong
2013-09-05 13:42 ` Veaceslav Falico
0 siblings, 1 reply; 6+ messages in thread
From: Ding Tianhong @ 2013-09-05 7:48 UTC (permalink / raw)
To: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Veaceslav Falico, Netdev
The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
(bonding: initial RCU conversion) has convert the roundrobin, active-backup,
broadcast and xor xmit path to rcu protection, the performance will be better
for these mode, so this time, convert xmit path for 3ad mode.
Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
Suggested-by: Veaceslav Falico <vfalico@redhat.com>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Nikolay Aleksandrov <nikolay@redhat.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------
drivers/net/bonding/bonding.h | 32 +++++++++++++++++++++++++++++++-
2 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 0d8f427..13f1deb 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
*/
static inline struct port *__get_first_port(struct bonding *bond)
{
- struct slave *first_slave = bond_first_slave(bond);
+ struct slave *first_slave = bond_first_slave_rcu(bond);
return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
}
@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
// If there's no bond for this port, or this is the last slave
if (bond == NULL)
return NULL;
- slave_next = bond_next_slave(bond, slave);
+ slave_next = bond_next_slave_rcu(bond, slave);
if (!slave_next || bond_is_first_slave(bond, slave_next))
return NULL;
@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
{
- struct slave *slave, *start_at;
struct bonding *bond = netdev_priv(dev);
+ struct slave *slave;
int slave_agg_no;
int slaves_in_agg;
int agg_id;
- int i;
struct ad_info ad_info;
int res = 1;
- read_lock(&bond->lock);
if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
dev->name);
@@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
- bond_for_each_slave(bond, slave) {
+ bond_for_each_slave_rcu(bond, slave) {
struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
if (agg && (agg->aggregator_identifier == agg_id)) {
- slave_agg_no--;
- if (slave_agg_no < 0)
- break;
+ if (--slave_agg_no < 0) {
+ if (SLAVE_IS_OK(slave)) {
+ res = bond_dev_queue_xmit(bond,
+ skb, slave->dev);
+ goto out;
+ }
+ }
}
}
@@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
goto out;
}
- start_at = slave;
-
- bond_for_each_slave_from(bond, slave, i, start_at) {
- int slave_agg_id = 0;
+ bond_for_each_slave_rcu(bond, slave) {
struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
- if (agg)
- slave_agg_id = agg->aggregator_identifier;
-
- if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
+ if (SLAVE_IS_OK(slave) && agg &&
+ agg->aggregator_identifier == agg_id) {
res = bond_dev_queue_xmit(bond, skb, slave->dev);
break;
}
}
out:
- read_unlock(&bond->lock);
if (res) {
/* no suitable interface, frame not sent */
kfree_skb(skb);
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index f7ab161..f013b12 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -74,13 +74,34 @@
/* slave list primitives */
#define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
+/* slave list primitives, Caller must hold rcu_read_lock */
+#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
+
+/* bond_is_empty return NULL if slave list is empty*/
+#define bond_is_empty(bond) \
+ (list_empty(&(bond)->slave_list))
+
+/* bond_is_empty_rcu return NULL if slave list is empty*/
+#define bond_is_empty_rcu(bond) \
+ (!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
+
/* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
#define bond_first_slave(bond) \
list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
#define bond_last_slave(bond) \
- (list_empty(&(bond)->slave_list) ? NULL : \
+ (bond_is_empty(bond) ? NULL : \
bond_to_slave((bond)->slave_list.prev))
+/**
+ * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
+ * Caller must hold rcu_read_lock
+ */
+#define bond_first_slave_rcu(bond) \
+ list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
+#define bond_last_slave_rcu(bond) \
+ (bond_is_empty_rcu(bond) ? NULL : \
+ bond_to_slave_rcu((bond)->slave_list.prev))
+
#define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
#define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)
@@ -93,6 +114,15 @@
(bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
bond_to_slave((pos)->list.prev))
+/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
+#define bond_next_slave_rcu(bond, pos) \
+ (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \
+ bond_to_slave_rcu((pos)->list.next))
+
+#define bond_prev_slave_rcu(bond, pos) \
+ (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
+ bond_to_slave_rcu((pos)->list.prev))
+
/**
* bond_for_each_slave_from - iterate the slaves list from a starting point
* @bond: the bond holding this list.
--
1.8.2.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path
2013-09-05 7:48 [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path Ding Tianhong
@ 2013-09-05 13:42 ` Veaceslav Falico
2013-09-05 14:17 ` Ding Tianhong
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Veaceslav Falico @ 2013-09-05 13:42 UTC (permalink / raw)
To: Ding Tianhong
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
On Thu, Sep 05, 2013 at 03:48:44PM +0800, Ding Tianhong wrote:
>The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
>(bonding: initial RCU conversion) has convert the roundrobin, active-backup,
>broadcast and xor xmit path to rcu protection, the performance will be better
>for these mode, so this time, convert xmit path for 3ad mode.
>
>Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
>Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>
>---
> drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------
> drivers/net/bonding/bonding.h | 32 +++++++++++++++++++++++++++++++-
> 2 files changed, 45 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index 0d8f427..13f1deb 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
> */
> static inline struct port *__get_first_port(struct bonding *bond)
> {
>- struct slave *first_slave = bond_first_slave(bond);
>+ struct slave *first_slave = bond_first_slave_rcu(bond);
>
> return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
> }
>@@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
> // If there's no bond for this port, or this is the last slave
> if (bond == NULL)
> return NULL;
>- slave_next = bond_next_slave(bond, slave);
>+ slave_next = bond_next_slave_rcu(bond, slave);
> if (!slave_next || bond_is_first_slave(bond, slave_next))
> return NULL;
>
>@@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
>
> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
> {
>- struct slave *slave, *start_at;
> struct bonding *bond = netdev_priv(dev);
>+ struct slave *slave;
> int slave_agg_no;
> int slaves_in_agg;
> int agg_id;
>- int i;
> struct ad_info ad_info;
> int res = 1;
>
>- read_lock(&bond->lock);
> if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
> pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
> dev->name);
>@@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>
> slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
>
>- bond_for_each_slave(bond, slave) {
>+ bond_for_each_slave_rcu(bond, slave) {
> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>
> if (agg && (agg->aggregator_identifier == agg_id)) {
>- slave_agg_no--;
>- if (slave_agg_no < 0)
>- break;
>+ if (--slave_agg_no < 0) {
>+ if (SLAVE_IS_OK(slave)) {
>+ res = bond_dev_queue_xmit(bond,
>+ skb, slave->dev);
>+ goto out;
>+ }
>+ }
> }
> }
>
>@@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
> goto out;
> }
>
>- start_at = slave;
>-
>- bond_for_each_slave_from(bond, slave, i, start_at) {
>- int slave_agg_id = 0;
>+ bond_for_each_slave_rcu(bond, slave) {
> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>
>- if (agg)
>- slave_agg_id = agg->aggregator_identifier;
>-
>- if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
>+ if (SLAVE_IS_OK(slave) && agg &&
>+ agg->aggregator_identifier == agg_id) {
> res = bond_dev_queue_xmit(bond, skb, slave->dev);
> break;
> }
> }
>
> out:
>- read_unlock(&bond->lock);
> if (res) {
> /* no suitable interface, frame not sent */
> kfree_skb(skb);
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index f7ab161..f013b12 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -74,13 +74,34 @@
> /* slave list primitives */
> #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
>
>+/* slave list primitives, Caller must hold rcu_read_lock */
>+#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
>+
>+/* bond_is_empty return NULL if slave list is empty*/
>+#define bond_is_empty(bond) \
>+ (list_empty(&(bond)->slave_list))
>+
>+/* bond_is_empty_rcu return NULL if slave list is empty*/
>+#define bond_is_empty_rcu(bond) \
>+ (!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
>+
> /* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
> #define bond_first_slave(bond) \
> list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
> #define bond_last_slave(bond) \
>- (list_empty(&(bond)->slave_list) ? NULL : \
>+ (bond_is_empty(bond) ? NULL : \
> bond_to_slave((bond)->slave_list.prev))
>
>+/**
>+ * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
>+ * Caller must hold rcu_read_lock
>+ */
>+#define bond_first_slave_rcu(bond) \
>+ list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
>+#define bond_last_slave_rcu(bond) \
>+ (bond_is_empty_rcu(bond) ? NULL : \
>+ bond_to_slave_rcu((bond)->slave_list.prev))
Really?
No. Again - take a look at list_first_or_null_rcu. And its comments.
I'm sorry that I'm acting that negatively... But if that gets accepted -
I'll have days of nightmares.
Try to understand how RCU works, please, before sending patches using it.
>+
> #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
> #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)
>
>@@ -93,6 +114,15 @@
> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
> bond_to_slave((pos)->list.prev))
>
>+/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
>+#define bond_next_slave_rcu(bond, pos) \
>+ (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \
>+ bond_to_slave_rcu((pos)->list.next))
>+
>+#define bond_prev_slave_rcu(bond, pos) \
>+ (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
>+ bond_to_slave_rcu((pos)->list.prev))
>+
> /**
> * bond_for_each_slave_from - iterate the slaves list from a starting point
> * @bond: the bond holding this list.
>--
>1.8.2.1
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path
2013-09-05 13:42 ` Veaceslav Falico
@ 2013-09-05 14:17 ` Ding Tianhong
2013-09-05 14:42 ` Ding Tianhong
2013-09-05 14:26 ` Eric Dumazet
2013-09-06 1:41 ` Ding Tianhong
2 siblings, 1 reply; 6+ messages in thread
From: Ding Tianhong @ 2013-09-05 14:17 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
于 2013/9/5 21:42, Veaceslav Falico 写道:
> On Thu, Sep 05, 2013 at 03:48:44PM +0800, Ding Tianhong wrote:
>> The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
>> (bonding: initial RCU conversion) has convert the roundrobin,
>> active-backup,
>> broadcast and xor xmit path to rcu protection, the performance will
>> be better
>> for these mode, so this time, convert xmit path for 3ad mode.
>>
>> Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> ---
>> drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------
>> drivers/net/bonding/bonding.h | 32 +++++++++++++++++++++++++++++++-
>> 2 files changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c
>> b/drivers/net/bonding/bond_3ad.c
>> index 0d8f427..13f1deb 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -143,7 +143,7 @@ static inline struct bonding
>> *__get_bond_by_port(struct port *port)
>> */
>> static inline struct port *__get_first_port(struct bonding *bond)
>> {
>> - struct slave *first_slave = bond_first_slave(bond);
>> + struct slave *first_slave = bond_first_slave_rcu(bond);
>>
>> return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
>> }
>> @@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct
>> port *port)
>> // If there's no bond for this port, or this is the last slave
>> if (bond == NULL)
>> return NULL;
>> - slave_next = bond_next_slave(bond, slave);
>> + slave_next = bond_next_slave_rcu(bond, slave);
>> if (!slave_next || bond_is_first_slave(bond, slave_next))
>> return NULL;
>>
>> @@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct
>> bonding *bond, struct ad_info *ad_info)
>>
>> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>> {
>> - struct slave *slave, *start_at;
>> struct bonding *bond = netdev_priv(dev);
>> + struct slave *slave;
>> int slave_agg_no;
>> int slaves_in_agg;
>> int agg_id;
>> - int i;
>> struct ad_info ad_info;
>> int res = 1;
>>
>> - read_lock(&bond->lock);
>> if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>> pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
>> dev->name);
>> @@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb,
>> struct net_device *dev)
>>
>> slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
>>
>> - bond_for_each_slave(bond, slave) {
>> + bond_for_each_slave_rcu(bond, slave) {
>> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>
>> if (agg && (agg->aggregator_identifier == agg_id)) {
>> - slave_agg_no--;
>> - if (slave_agg_no < 0)
>> - break;
>> + if (--slave_agg_no < 0) {
>> + if (SLAVE_IS_OK(slave)) {
>> + res = bond_dev_queue_xmit(bond,
>> + skb, slave->dev);
>> + goto out;
>> + }
>> + }
>> }
>> }
>>
>> @@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb,
>> struct net_device *dev)
>> goto out;
>> }
>>
>> - start_at = slave;
>> -
>> - bond_for_each_slave_from(bond, slave, i, start_at) {
>> - int slave_agg_id = 0;
>> + bond_for_each_slave_rcu(bond, slave) {
>> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>
>> - if (agg)
>> - slave_agg_id = agg->aggregator_identifier;
>> -
>> - if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
>> + if (SLAVE_IS_OK(slave) && agg &&
>> + agg->aggregator_identifier == agg_id) {
>> res = bond_dev_queue_xmit(bond, skb, slave->dev);
>> break;
>> }
>> }
>>
>> out:
>> - read_unlock(&bond->lock);
>> if (res) {
>> /* no suitable interface, frame not sent */
>> kfree_skb(skb);
>> diff --git a/drivers/net/bonding/bonding.h
>> b/drivers/net/bonding/bonding.h
>> index f7ab161..f013b12 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -74,13 +74,34 @@
>> /* slave list primitives */
>> #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
>>
>> +/* slave list primitives, Caller must hold rcu_read_lock */
>> +#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
>> +
>> +/* bond_is_empty return NULL if slave list is empty*/
>> +#define bond_is_empty(bond) \
>> + (list_empty(&(bond)->slave_list))
>> +
>> +/* bond_is_empty_rcu return NULL if slave list is empty*/
>> +#define bond_is_empty_rcu(bond) \
>> + (!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
>> +
>> /* IMPORTANT: bond_first/last_slave can return NULL in case of an
>> empty list */
>> #define bond_first_slave(bond) \
>> list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
>> #define bond_last_slave(bond) \
>> - (list_empty(&(bond)->slave_list) ? NULL : \
>> + (bond_is_empty(bond) ? NULL : \
>> bond_to_slave((bond)->slave_list.prev))
>>
>> +/**
>> + * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of
>> an empty list
>> + * Caller must hold rcu_read_lock
>> + */
>> +#define bond_first_slave_rcu(bond) \
>> + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
>> +#define bond_last_slave_rcu(bond) \
>> + (bond_is_empty_rcu(bond) ? NULL : \
>> + bond_to_slave_rcu((bond)->slave_list.prev))
>
> Really?
>
> No. Again - take a look at list_first_or_null_rcu. And its comments.
>
> I'm sorry that I'm acting that negatively... But if that gets accepted -
> I'll have days of nightmares.
>
> Try to understand how RCU works, please, before sending patches using it.
>
I am sad to here that, I think I had good understand of rcu, but maybe
miss something.
#define bond_first_slave_rcu(bond) \
(bond_is_empty_rcu(bond) ? NULL : \
bond_to_slave_rcu((bond)->slave_list.next))
vs
list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
maybe the first one is confort you, but I think the second is racy and
simple, maybe I still
miss something, I need a coffee to clear up.
>> +
>> #define bond_is_first_slave(bond, pos) ((pos)->list.prev ==
>> &(bond)->slave_list)
>> #define bond_is_last_slave(bond, pos) ((pos)->list.next ==
>> &(bond)->slave_list)
>>
>> @@ -93,6 +114,15 @@
>> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
>> bond_to_slave((pos)->list.prev))
>>
>> +/* Since bond_first/last_slave_rcu can return NULL, these can return
>> NULL too */
>> +#define bond_next_slave_rcu(bond, pos) \
>> + (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \
>> + bond_to_slave_rcu((pos)->list.next))
>> +
>> +#define bond_prev_slave_rcu(bond, pos) \
>> + (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
>> + bond_to_slave_rcu((pos)->list.prev))
>> +
>> /**
>> * bond_for_each_slave_from - iterate the slaves list from a starting
>> point
>> * @bond: the bond holding this list.
>> --
>> 1.8.2.1
>>
>>
>>
> --
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path
2013-09-05 13:42 ` Veaceslav Falico
2013-09-05 14:17 ` Ding Tianhong
@ 2013-09-05 14:26 ` Eric Dumazet
2013-09-06 1:41 ` Ding Tianhong
2 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2013-09-05 14:26 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
On Thu, 2013-09-05 at 15:42 +0200, Veaceslav Falico wrote:
> Really?
>
> No. Again - take a look at list_first_or_null_rcu. And its comments.
>
> I'm sorry that I'm acting that negatively... But if that gets accepted -
> I'll have days of nightmares.
>
> Try to understand how RCU works, please, before sending patches using it.
BTW, RCU is really hard, even for experimented kernel developers.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path
2013-09-05 14:17 ` Ding Tianhong
@ 2013-09-05 14:42 ` Ding Tianhong
0 siblings, 0 replies; 6+ messages in thread
From: Ding Tianhong @ 2013-09-05 14:42 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
于 2013/9/5 22:17, Ding Tianhong 写道:
> 于 2013/9/5 21:42, Veaceslav Falico 写道:
>> On Thu, Sep 05, 2013 at 03:48:44PM +0800, Ding Tianhong wrote:
>>> The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
>>> (bonding: initial RCU conversion) has convert the roundrobin,
>>> active-backup,
>>> broadcast and xor xmit path to rcu protection, the performance will
>>> be better
>>> for these mode, so this time, convert xmit path for 3ad mode.
>>>
>>> Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
>>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>>> Cc: Veaceslav Falico <vfalico@redhat.com>
>>> ---
>>> drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------
>>> drivers/net/bonding/bonding.h | 32 +++++++++++++++++++++++++++++++-
>>> 2 files changed, 45 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_3ad.c
>>> b/drivers/net/bonding/bond_3ad.c
>>> index 0d8f427..13f1deb 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -143,7 +143,7 @@ static inline struct bonding
>>> *__get_bond_by_port(struct port *port)
>>> */
>>> static inline struct port *__get_first_port(struct bonding *bond)
>>> {
>>> - struct slave *first_slave = bond_first_slave(bond);
>>> + struct slave *first_slave = bond_first_slave_rcu(bond);
>>>
>>> return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
>>> }
>>> @@ -163,7 +163,7 @@ static inline struct port
>>> *__get_next_port(struct port *port)
>>> // If there's no bond for this port, or this is the last slave
>>> if (bond == NULL)
>>> return NULL;
>>> - slave_next = bond_next_slave(bond, slave);
>>> + slave_next = bond_next_slave_rcu(bond, slave);
>>> if (!slave_next || bond_is_first_slave(bond, slave_next))
>>> return NULL;
>>>
>>> @@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct
>>> bonding *bond, struct ad_info *ad_info)
>>>
>>> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>>> {
>>> - struct slave *slave, *start_at;
>>> struct bonding *bond = netdev_priv(dev);
>>> + struct slave *slave;
>>> int slave_agg_no;
>>> int slaves_in_agg;
>>> int agg_id;
>>> - int i;
>>> struct ad_info ad_info;
>>> int res = 1;
>>>
>>> - read_lock(&bond->lock);
>>> if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>>> pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
>>> dev->name);
>>> @@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb,
>>> struct net_device *dev)
>>>
>>> slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
>>>
>>> - bond_for_each_slave(bond, slave) {
>>> + bond_for_each_slave_rcu(bond, slave) {
>>> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>>
>>> if (agg && (agg->aggregator_identifier == agg_id)) {
>>> - slave_agg_no--;
>>> - if (slave_agg_no < 0)
>>> - break;
>>> + if (--slave_agg_no < 0) {
>>> + if (SLAVE_IS_OK(slave)) {
>>> + res = bond_dev_queue_xmit(bond,
>>> + skb, slave->dev);
>>> + goto out;
>>> + }
>>> + }
>>> }
>>> }
>>>
>>> @@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb,
>>> struct net_device *dev)
>>> goto out;
>>> }
>>>
>>> - start_at = slave;
>>> -
>>> - bond_for_each_slave_from(bond, slave, i, start_at) {
>>> - int slave_agg_id = 0;
>>> + bond_for_each_slave_rcu(bond, slave) {
>>> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>>
>>> - if (agg)
>>> - slave_agg_id = agg->aggregator_identifier;
>>> -
>>> - if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
>>> + if (SLAVE_IS_OK(slave) && agg &&
>>> + agg->aggregator_identifier == agg_id) {
>>> res = bond_dev_queue_xmit(bond, skb, slave->dev);
>>> break;
>>> }
>>> }
>>>
>>> out:
>>> - read_unlock(&bond->lock);
>>> if (res) {
>>> /* no suitable interface, frame not sent */
>>> kfree_skb(skb);
>>> diff --git a/drivers/net/bonding/bonding.h
>>> b/drivers/net/bonding/bonding.h
>>> index f7ab161..f013b12 100644
>>> --- a/drivers/net/bonding/bonding.h
>>> +++ b/drivers/net/bonding/bonding.h
>>> @@ -74,13 +74,34 @@
>>> /* slave list primitives */
>>> #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
>>>
>>> +/* slave list primitives, Caller must hold rcu_read_lock */
>>> +#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
>>> +
>>> +/* bond_is_empty return NULL if slave list is empty*/
>>> +#define bond_is_empty(bond) \
>>> + (list_empty(&(bond)->slave_list))
>>> +
>>> +/* bond_is_empty_rcu return NULL if slave list is empty*/
>>> +#define bond_is_empty_rcu(bond) \
>>> + (!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
>>> +
>>> /* IMPORTANT: bond_first/last_slave can return NULL in case of an
>>> empty list */
>>> #define bond_first_slave(bond) \
>>> list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
>>> #define bond_last_slave(bond) \
>>> - (list_empty(&(bond)->slave_list) ? NULL : \
>>> + (bond_is_empty(bond) ? NULL : \
>>> bond_to_slave((bond)->slave_list.prev))
>>>
>>> +/**
>>> + * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of
>>> an empty list
>>> + * Caller must hold rcu_read_lock
>>> + */
>>> +#define bond_first_slave_rcu(bond) \
>>> + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
>>> +#define bond_last_slave_rcu(bond) \
>>> + (bond_is_empty_rcu(bond) ? NULL : \
>>> + bond_to_slave_rcu((bond)->slave_list.prev))
>>
>> Really?
>>
>> No. Again - take a look at list_first_or_null_rcu. And its comments.
>>
>> I'm sorry that I'm acting that negatively... But if that gets accepted -
>> I'll have days of nightmares.
>>
>> Try to understand how RCU works, please, before sending patches using
>> it.
>>
> I am sad to here that, I think I had good understand of rcu, but maybe
> miss something.
> #define bond_first_slave_rcu(bond) \
> (bond_is_empty_rcu(bond) ? NULL : \
> bond_to_slave_rcu((bond)->slave_list.next))
>
> vs
> list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
>
> maybe the first one is confort you, but I think the second is racy and
> simple, maybe I still
> miss something, I need a coffee to clear up.
>
fprgot it
best regards
Ding Tianhong
>
>>> +
>>> #define bond_is_first_slave(bond, pos) ((pos)->list.prev ==
>>> &(bond)->slave_list)
>>> #define bond_is_last_slave(bond, pos) ((pos)->list.next ==
>>> &(bond)->slave_list)
>>>
>>> @@ -93,6 +114,15 @@
>>> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
>>> bond_to_slave((pos)->list.prev))
>>>
>>> +/* Since bond_first/last_slave_rcu can return NULL, these can
>>> return NULL too */
>>> +#define bond_next_slave_rcu(bond, pos) \
>>> + (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \
>>> + bond_to_slave_rcu((pos)->list.next))
>>> +
>>> +#define bond_prev_slave_rcu(bond, pos) \
>>> + (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
>>> + bond_to_slave_rcu((pos)->list.prev))
>>> +
>>> /**
>>> * bond_for_each_slave_from - iterate the slaves list from a starting
>>> point
>>> * @bond: the bond holding this list.
>>> --
>>> 1.8.2.1
>>>
>>>
>>>
>> --
>> 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
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path
2013-09-05 13:42 ` Veaceslav Falico
2013-09-05 14:17 ` Ding Tianhong
2013-09-05 14:26 ` Eric Dumazet
@ 2013-09-06 1:41 ` Ding Tianhong
2 siblings, 0 replies; 6+ messages in thread
From: Ding Tianhong @ 2013-09-06 1:41 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
On 2013/9/5 21:42, Veaceslav Falico wrote:
> On Thu, Sep 05, 2013 at 03:48:44PM +0800, Ding Tianhong wrote:
>> The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
>> (bonding: initial RCU conversion) has convert the roundrobin, active-backup,
>> broadcast and xor xmit path to rcu protection, the performance will be better
>> for these mode, so this time, convert xmit path for 3ad mode.
>>
>> Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> ---
>> drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------
>> drivers/net/bonding/bonding.h | 32 +++++++++++++++++++++++++++++++-
>> 2 files changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index 0d8f427..13f1deb 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
>> */
>> static inline struct port *__get_first_port(struct bonding *bond)
>> {
>> - struct slave *first_slave = bond_first_slave(bond);
>> + struct slave *first_slave = bond_first_slave_rcu(bond);
>>
>> return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
>> }
>> @@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
>> // If there's no bond for this port, or this is the last slave
>> if (bond == NULL)
>> return NULL;
>> - slave_next = bond_next_slave(bond, slave);
>> + slave_next = bond_next_slave_rcu(bond, slave);
>> if (!slave_next || bond_is_first_slave(bond, slave_next))
>> return NULL;
>>
>> @@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
>>
>> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>> {
>> - struct slave *slave, *start_at;
>> struct bonding *bond = netdev_priv(dev);
>> + struct slave *slave;
>> int slave_agg_no;
>> int slaves_in_agg;
>> int agg_id;
>> - int i;
>> struct ad_info ad_info;
>> int res = 1;
>>
>> - read_lock(&bond->lock);
>> if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>> pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
>> dev->name);
>> @@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>>
>> slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
>>
>> - bond_for_each_slave(bond, slave) {
>> + bond_for_each_slave_rcu(bond, slave) {
>> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>
>> if (agg && (agg->aggregator_identifier == agg_id)) {
>> - slave_agg_no--;
>> - if (slave_agg_no < 0)
>> - break;
>> + if (--slave_agg_no < 0) {
>> + if (SLAVE_IS_OK(slave)) {
>> + res = bond_dev_queue_xmit(bond,
>> + skb, slave->dev);
>> + goto out;
>> + }
>> + }
>> }
>> }
>>
>> @@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>> goto out;
>> }
>>
>> - start_at = slave;
>> -
>> - bond_for_each_slave_from(bond, slave, i, start_at) {
>> - int slave_agg_id = 0;
>> + bond_for_each_slave_rcu(bond, slave) {
>> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>>
>> - if (agg)
>> - slave_agg_id = agg->aggregator_identifier;
>> -
>> - if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
>> + if (SLAVE_IS_OK(slave) && agg &&
>> + agg->aggregator_identifier == agg_id) {
>> res = bond_dev_queue_xmit(bond, skb, slave->dev);
>> break;
>> }
>> }
>>
>> out:
>> - read_unlock(&bond->lock);
>> if (res) {
>> /* no suitable interface, frame not sent */
>> kfree_skb(skb);
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index f7ab161..f013b12 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -74,13 +74,34 @@
>> /* slave list primitives */
>> #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
>>
>> +/* slave list primitives, Caller must hold rcu_read_lock */
>> +#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
>> +
>> +/* bond_is_empty return NULL if slave list is empty*/
>> +#define bond_is_empty(bond) \
>> + (list_empty(&(bond)->slave_list))
>> +
>> +/* bond_is_empty_rcu return NULL if slave list is empty*/
>> +#define bond_is_empty_rcu(bond) \
>> + (!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
>> +
>> /* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
>> #define bond_first_slave(bond) \
>> list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
>> #define bond_last_slave(bond) \
>> - (list_empty(&(bond)->slave_list) ? NULL : \
>> + (bond_is_empty(bond) ? NULL : \
>> bond_to_slave((bond)->slave_list.prev))
>>
>> +/**
>> + * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
>> + * Caller must hold rcu_read_lock
>> + */
>> +#define bond_first_slave_rcu(bond) \
>> + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
>> +#define bond_last_slave_rcu(bond) \
>> + (bond_is_empty_rcu(bond) ? NULL : \
>> + bond_to_slave_rcu((bond)->slave_list.prev))
>
> Really?
>
> No. Again - take a look at list_first_or_null_rcu. And its comments.
>
> I'm sorry that I'm acting that negatively... But if that gets accepted -
> I'll have days of nightmares.
>
> Try to understand how RCU works, please, before sending patches using it.
>
After careful consideration and review the code, I find my problem and agree you,
thanks for your patient guidance, I will fix it as soon as possible.
>> +
>> #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
>> #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)
>>
>> @@ -93,6 +114,15 @@
>> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
>> bond_to_slave((pos)->list.prev))
>>
>> +/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
>> +#define bond_next_slave_rcu(bond, pos) \
>> + (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \
>> + bond_to_slave_rcu((pos)->list.next))
>> +
>> +#define bond_prev_slave_rcu(bond, pos) \
>> + (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
>> + bond_to_slave_rcu((pos)->list.prev))
>> +
>> /**
>> * bond_for_each_slave_from - iterate the slaves list from a starting point
>> * @bond: the bond holding this list.
>> --
>> 1.8.2.1
>>
>>
>>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-06 1:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-05 7:48 [PATCH net-next v3 1/6] bonding: simplify and use RCU protection for 3ad xmit path Ding Tianhong
2013-09-05 13:42 ` Veaceslav Falico
2013-09-05 14:17 ` Ding Tianhong
2013-09-05 14:42 ` Ding Tianhong
2013-09-05 14:26 ` Eric Dumazet
2013-09-06 1:41 ` Ding Tianhong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).