[PATCH] net: take care of bonding in build_skb_flow_key (v3)

Subsystems: networking [general], networking [ipv4/ipv6], the rest

STALE3858d

13 messages, 4 authors, 2016-01-21 · open the first message on its own page

[PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: Wengang Wang <hidden>
Date: 2016-01-20 05:59:55

In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the master become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
 net/ipv4/route.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 *fl4, const struct sk_buff *skb,
 			       const struct sock *sk)
 {
 	const struct iphdr *iph = ip_hdr(skb);
-	int oif = skb->dev->ifindex;
+	struct net_device *master = NULL;
 	u8 tos = RT_TOS(iph->tos);
 	u8 prot = iph->protocol;
 	u32 mark = skb->mark;
+	int oif;
+
+	if (skb->dev->flags & IFF_SLAVE) {
+		rtnl_lock();
+		master = netdev_master_upper_dev_get(skb->dev);
+		rtnl_unlock();
+	}
+	if (master)
+		oif = master->ifindex;
+	else
+		oif = skb->dev->ifindex;
 
 	__build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
 }
-- 
2.1.0

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: zhuyj <zyjzyj2000@gmail.com>
Date: 2016-01-20 06:23:39

On 01/20/2016 01:32 PM, Wengang Wang wrote:
quoted hunk
In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the master become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 *fl4, const struct sk_buff *skb,
  			       const struct sock *sk)
  {
  	const struct iphdr *iph = ip_hdr(skb);
-	int oif = skb->dev->ifindex;
+	struct net_device *master = NULL;
  	u8 tos = RT_TOS(iph->tos);
  	u8 prot = iph->protocol;
  	u32 mark = skb->mark;
+	int oif;
+
+	if (skb->dev->flags & IFF_SLAVE) {
+		rtnl_lock();
+		master = netdev_master_upper_dev_get(skb->dev);
+		rtnl_unlock();
update_pmtu is called very frequently. Is it appropriate to use 
rtnl_lock here?
That is, rtnl_lock is called frequently. Maybe other functions have 
little chance to call rtnl_lock.

Best Regards!
Zhu Yanjun
+	}
+	if (master)
+		oif = master->ifindex;
+	else
+		oif = skb->dev->ifindex;
  
  	__build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
  }

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: zhuyj <zyjzyj2000@gmail.com>
Date: 2016-01-20 06:29:13

On 01/20/2016 02:24 PM, zhuyj wrote:
On 01/20/2016 01:32 PM, Wengang Wang wrote:
quoted
In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu 
actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets 
doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the master 
become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 
*fl4, const struct sk_buff *skb,
                     const struct sock *sk)
  {
      const struct iphdr *iph = ip_hdr(skb);
-    int oif = skb->dev->ifindex;
+    struct net_device *master = NULL;
      u8 tos = RT_TOS(iph->tos);
      u8 prot = iph->protocol;
      u32 mark = skb->mark;
+    int oif;
+
+    if (skb->dev->flags & IFF_SLAVE) {
+        rtnl_lock();
+        master = netdev_master_upper_dev_get(skb->dev);
+        rtnl_unlock();
update_pmtu is called very frequently. Is it appropriate to use 
rtnl_lock here?
That is, rtnl_lock is called frequently. Maybe other functions have 
little chance to call rtnl_lock.
Maybe this function netdev_upper_get_next_dev_rcu is better? I am not sure.
Best Regards!
Zhu Yanjun
quoted
+    }
+    if (master)
+        oif = master->ifindex;
+    else
+        oif = skb->dev->ifindex;
        __build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
  }

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: zhuyj <zyjzyj2000@gmail.com>
Date: 2016-01-20 06:31:37

On 01/20/2016 02:29 PM, zhuyj wrote:
On 01/20/2016 02:24 PM, zhuyj wrote:
quoted
On 01/20/2016 01:32 PM, Wengang Wang wrote:
quoted
In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu 
actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets 
doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the master 
become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 
*fl4, const struct sk_buff *skb,
                     const struct sock *sk)
  {
      const struct iphdr *iph = ip_hdr(skb);
-    int oif = skb->dev->ifindex;
+    struct net_device *master = NULL;
      u8 tos = RT_TOS(iph->tos);
      u8 prot = iph->protocol;
      u32 mark = skb->mark;
+    int oif;
+
+    if (skb->dev->flags & IFF_SLAVE) {
+        rtnl_lock();
+        master = netdev_master_upper_dev_get(skb->dev);
+        rtnl_unlock();
update_pmtu is called very frequently. Is it appropriate to use 
rtnl_lock here?
That is, rtnl_lock is called frequently. Maybe other functions have 
little chance to call rtnl_lock.
Maybe this function netdev_master_upper_dev_get_rcu is better? I am 
not sure.
Maybe this function netdev_master_upper_dev_get_rcu is better? I am not 
sure.
quoted
Best Regards!
Zhu Yanjun
quoted
+    }
+    if (master)
+        oif = master->ifindex;
+    else
+        oif = skb->dev->ifindex;
        __build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
  }

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: Wengang Wang <hidden>
Date: 2016-01-20 07:35:19


在 2016年01月20日 14:24, zhuyj 写道:
On 01/20/2016 01:32 PM, Wengang Wang wrote:
quoted
In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu 
actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets 
doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the master 
become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 
*fl4, const struct sk_buff *skb,
                     const struct sock *sk)
  {
      const struct iphdr *iph = ip_hdr(skb);
-    int oif = skb->dev->ifindex;
+    struct net_device *master = NULL;
      u8 tos = RT_TOS(iph->tos);
      u8 prot = iph->protocol;
      u32 mark = skb->mark;
+    int oif;
+
+    if (skb->dev->flags & IFF_SLAVE) {
+        rtnl_lock();
+        master = netdev_master_upper_dev_get(skb->dev);
+        rtnl_unlock();
update_pmtu is called very frequently. Is it appropriate to use 
rtnl_lock here?
By "very frequently", how frequently it is expected? And what situation 
can cause that?
For my case, the update_pmtu is called only once.

thanks,
wengang
That is, rtnl_lock is called frequently. Maybe other functions have 
little chance to call rtnl_lock.

Best Regards!
Zhu Yanjun
quoted
+    }
+    if (master)
+        oif = master->ifindex;
+    else
+        oif = skb->dev->ifindex;
        __build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
  }

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: zhuyj <zyjzyj2000@gmail.com>
Date: 2016-01-20 07:53:34

On 01/20/2016 03:38 PM, Wengang Wang wrote:

在 2016年01月20日 14:24, zhuyj 写道:
quoted
On 01/20/2016 01:32 PM, Wengang Wang wrote:
quoted
In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu 
actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets 
doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the master 
become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 
*fl4, const struct sk_buff *skb,
                     const struct sock *sk)
  {
      const struct iphdr *iph = ip_hdr(skb);
-    int oif = skb->dev->ifindex;
+    struct net_device *master = NULL;
      u8 tos = RT_TOS(iph->tos);
      u8 prot = iph->protocol;
      u32 mark = skb->mark;
+    int oif;
+
+    if (skb->dev->flags & IFF_SLAVE) {
+        rtnl_lock();
+        master = netdev_master_upper_dev_get(skb->dev);
+        rtnl_unlock();
update_pmtu is called very frequently. Is it appropriate to use 
rtnl_lock here?
By "very frequently", how frequently it is expected? And what 
situation can cause that?
For my case, the update_pmtu is called only once.
ip_tunnel_xmit

Zhu Yanjun
thanks,
wengang
quoted
That is, rtnl_lock is called frequently. Maybe other functions have 
little chance to call rtnl_lock.

Best Regards!
Zhu Yanjun
quoted
+    }
+    if (master)
+        oif = master->ifindex;
+    else
+        oif = skb->dev->ifindex;
        __build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
  }

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: Wengang Wang <hidden>
Date: 2016-01-20 09:43:53


在 2016年01月20日 15:54, zhuyj 写道:
On 01/20/2016 03:38 PM, Wengang Wang wrote:
quoted

在 2016年01月20日 14:24, zhuyj 写道:
quoted
On 01/20/2016 01:32 PM, Wengang Wang wrote:
quoted
In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu 
actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets 
doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the 
master become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 
*fl4, const struct sk_buff *skb,
                     const struct sock *sk)
  {
      const struct iphdr *iph = ip_hdr(skb);
-    int oif = skb->dev->ifindex;
+    struct net_device *master = NULL;
      u8 tos = RT_TOS(iph->tos);
      u8 prot = iph->protocol;
      u32 mark = skb->mark;
+    int oif;
+
+    if (skb->dev->flags & IFF_SLAVE) {
+        rtnl_lock();
+        master = netdev_master_upper_dev_get(skb->dev);
+        rtnl_unlock();
update_pmtu is called very frequently. Is it appropriate to use 
rtnl_lock here?
By "very frequently", how frequently it is expected? And what 
situation can cause that?
For my case, the update_pmtu is called only once.
ip_tunnel_xmit
Can you please explain with more details?

thanks,
wengang

Zhu Yanjun
quoted
thanks,
wengang
quoted
That is, rtnl_lock is called frequently. Maybe other functions have 
little chance to call rtnl_lock.

Best Regards!
Zhu Yanjun
quoted
+    }
+    if (master)
+        oif = master->ifindex;
+    else
+        oif = skb->dev->ifindex;
        __build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
  }

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: zhuyj <zyjzyj2000@gmail.com>
Date: 2016-01-20 09:56:12

On 01/20/2016 05:47 PM, Wengang Wang wrote:

在 2016年01月20日 15:54, zhuyj 写道:
quoted
On 01/20/2016 03:38 PM, Wengang Wang wrote:
quoted

在 2016年01月20日 14:24, zhuyj 写道:
quoted
On 01/20/2016 01:32 PM, Wengang Wang wrote:
quoted
In a bonding setting, we determines fragment size according to MTU 
and
PMTU associated to the bonding master. If the slave finds the 
fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu 
actually
tries to update is the slave (skb->dev), not the master. Thus 
since no
PMTU change happens on master, the fragment size for later packets 
doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the 
transition of
device index from bonding slave to the master. That makes the 
master become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 
*fl4, const struct sk_buff *skb,
                     const struct sock *sk)
  {
      const struct iphdr *iph = ip_hdr(skb);
-    int oif = skb->dev->ifindex;
+    struct net_device *master = NULL;
      u8 tos = RT_TOS(iph->tos);
      u8 prot = iph->protocol;
      u32 mark = skb->mark;
+    int oif;
+
+    if (skb->dev->flags & IFF_SLAVE) {
+        rtnl_lock();
+        master = netdev_master_upper_dev_get(skb->dev);
+        rtnl_unlock();
update_pmtu is called very frequently. Is it appropriate to use 
rtnl_lock here?
By "very frequently", how frequently it is expected? And what 
situation can cause that?
For my case, the update_pmtu is called only once.
ip_tunnel_xmit
Can you please explain with more details?
dev_queue_xmit->ipip_tunnel_xmit->ip_tunnel_xmit->tnl_update_pmtu-> 
skb_dst(skb)->ops->update_pmtu
thanks,
wengang

quoted
Zhu Yanjun
quoted
thanks,
wengang
quoted
That is, rtnl_lock is called frequently. Maybe other functions have 
little chance to call rtnl_lock.

Best Regards!
Zhu Yanjun
quoted
+    }
+    if (master)
+        oif = master->ifindex;
+    else
+        oif = skb->dev->ifindex;
        __build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
  }

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: Sabrina Dubroca <sd@queasysnail.net>
Date: 2016-01-20 15:18:23

2016-01-20, 13:32:13 +0800, Wengang Wang wrote:
quoted hunk
In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the master become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
 net/ipv4/route.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 *fl4, const struct sk_buff *skb,
 			       const struct sock *sk)
 {
 	const struct iphdr *iph = ip_hdr(skb);
-	int oif = skb->dev->ifindex;
+	struct net_device *master = NULL;
 	u8 tos = RT_TOS(iph->tos);
 	u8 prot = iph->protocol;
 	u32 mark = skb->mark;
+	int oif;
+
+	if (skb->dev->flags & IFF_SLAVE) {
Maybe use netif_is_bond_slave here instead, since you have this
problem with bonding slaves?

+		rtnl_lock();
+		master = netdev_master_upper_dev_get(skb->dev);
+		rtnl_unlock();
+	}
As zhuyj said, this is called from dev_queue_xmit, so you cannot take
rtnl_lock here.
+	if (master)
+		oif = master->ifindex;
You cannot dereference master after you release the rtnl lock.

So it would probably be best to use netdev_master_upper_dev_get_rcu,
as zhuyj suggested earlier, and make sure that you only use the result
between rcu_read_lock()/rcu_read_unlock():

    rcu_read_lock();
    master = netdev_master_upper_dev_get_rcu(skb->dev);
    if (master)
        oif = master->ifindex;
    rcu_read_unlock();


Thanks,

-- 
Sabrina

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: Wengang Wang <hidden>
Date: 2016-01-21 02:37:16


在 2016年01月20日 17:56, zhuyj 写道:
On 01/20/2016 05:47 PM, Wengang Wang wrote:
quoted

在 2016年01月20日 15:54, zhuyj 写道:
quoted
On 01/20/2016 03:38 PM, Wengang Wang wrote:
quoted

在 2016年01月20日 14:24, zhuyj 写道:
quoted
On 01/20/2016 01:32 PM, Wengang Wang wrote:
quoted
In a bonding setting, we determines fragment size according to 
MTU and
PMTU associated to the bonding master. If the slave finds the 
fragment
size is too big, it drops the fragment and calls 
ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu 
actually
tries to update is the slave (skb->dev), not the master. Thus 
since no
PMTU change happens on master, the fragment size for later 
packets doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the 
transition of
device index from bonding slave to the master. That makes the 
master become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct 
flowi4 *fl4, const struct sk_buff *skb,
                     const struct sock *sk)
  {
      const struct iphdr *iph = ip_hdr(skb);
-    int oif = skb->dev->ifindex;
+    struct net_device *master = NULL;
      u8 tos = RT_TOS(iph->tos);
      u8 prot = iph->protocol;
      u32 mark = skb->mark;
+    int oif;
+
+    if (skb->dev->flags & IFF_SLAVE) {
+        rtnl_lock();
+        master = netdev_master_upper_dev_get(skb->dev);
+        rtnl_unlock();
update_pmtu is called very frequently. Is it appropriate to use 
rtnl_lock here?
By "very frequently", how frequently it is expected? And what 
situation can cause that?
For my case, the update_pmtu is called only once.
ip_tunnel_xmit
Can you please explain with more details?
dev_queue_xmit->ipip_tunnel_xmit->ip_tunnel_xmit->tnl_update_pmtu-> 
skb_dst(skb)->ops->update_pmtu
For ipip,  yes seems update_pmtu is called in line for each call of 
queue_xmit.  Do you know if it's a good configuration for ipip + bonding?
Other's comment and suggestion?

thanks,
wengang

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: Jay Vosburgh <hidden>
Date: 2016-01-21 04:05:33

Wengang Wang [off-list ref] wrote:
[...]
For ipip,  yes seems update_pmtu is called in line for each call of
queue_xmit.  Do you know if it's a good configuration for ipip + bonding?
	Yes, it is.
Other's comment and suggestion?
	I agree with Sabrina Dubroca [off-list ref]'s suggestions
from yesterday.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: Wengang Wang <hidden>
Date: 2016-01-21 05:12:27


在 2016年01月20日 23:18, Sabrina Dubroca 写道:
2016-01-20, 13:32:13 +0800, Wengang Wang wrote:
quoted
In a bonding setting, we determines fragment size according to MTU and
PMTU associated to the bonding master. If the slave finds the fragment
size is too big, it drops the fragment and calls ip_rt_update_pmtu(),
passing _skb_ and _pmtu_, trying to update the path MTU.
Problem is that the target device that function ip_rt_update_pmtu actually
tries to update is the slave (skb->dev), not the master. Thus since no
PMTU change happens on master, the fragment size for later packets doesn't
change so all later fragments/packets are dropped too.

The fix is letting build_skb_flow_key() take care of the transition of
device index from bonding slave to the master. That makes the master become
the target device that ip_rt_update_pmtu tries to update PMTU to.

Signed-off-by: Wengang Wang <redacted>
---
  net/ipv4/route.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 85f184e..c59fb0d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -523,10 +523,21 @@ static void build_skb_flow_key(struct flowi4 *fl4, const struct sk_buff *skb,
  			       const struct sock *sk)
  {
  	const struct iphdr *iph = ip_hdr(skb);
-	int oif = skb->dev->ifindex;
+	struct net_device *master = NULL;
  	u8 tos = RT_TOS(iph->tos);
  	u8 prot = iph->protocol;
  	u32 mark = skb->mark;
+	int oif;
+
+	if (skb->dev->flags & IFF_SLAVE) {
Maybe use netif_is_bond_slave here instead, since you have this
problem with bonding slaves?

quoted
+		rtnl_lock();
+		master = netdev_master_upper_dev_get(skb->dev);
+		rtnl_unlock();
+	}
As zhuyj said, this is called from dev_queue_xmit, so you cannot take
rtnl_lock here.
quoted
+	if (master)
+		oif = master->ifindex;
You cannot dereference master after you release the rtnl lock.

So it would probably be best to use netdev_master_upper_dev_get_rcu,
as zhuyj suggested earlier, and make sure that you only use the result
between rcu_read_lock()/rcu_read_unlock():

     rcu_read_lock();
     master = netdev_master_upper_dev_get_rcu(skb->dev);
     if (master)
         oif = master->ifindex;
     rcu_read_unlock();
OK, thanks for advising.

thanks,
wengang
Thanks,

Re: [PATCH] net: take care of bonding in build_skb_flow_key (v3)

From: Wengang Wang <hidden>
Date: 2016-01-21 05:13:54


在 2016年01月21日 12:05, Jay Vosburgh 写道:
Wengang Wang [off-list ref] wrote:
[...]
quoted
For ipip,  yes seems update_pmtu is called in line for each call of
queue_xmit.  Do you know if it's a good configuration for ipip + bonding?
	Yes, it is.
quoted
Other's comment and suggestion?
	I agree with Sabrina Dubroca [off-list ref]'s suggestions
from yesterday.
Thank you! I will follow.

thanks,
wengang
	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help