Re: [PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
On Thu, Dec 24, 2015 at 1:14 AM, Nicolas Dichtel
wrote:
> Le 24/12/2015 00:52, Pravin B Shelar a écrit :
> [snip]
>>
>> @@ -83,22 +84,12 @@ int ip6_tnl_get_iflink(const struct net_device *dev);
>> static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
>> struct net_device *dev)
>> {
>> - struct net_device_stats *stats = &dev->stats;
>> int pkt_len, err;
>>
>> pkt_len = skb->len - skb_inner_network_offset(skb);
>> err = ip6_local_out(dev_net(skb_dst(skb)->dev), sk, skb);
>> -
>> - if (net_xmit_eval(err) == 0) {
>> - struct pcpu_sw_netstats *tstats =
>> get_cpu_ptr(dev->tstats);
>> - u64_stats_update_begin(&tstats->syncp);
>> - tstats->tx_bytes += pkt_len;
>> - tstats->tx_packets++;
>> - u64_stats_update_end(&tstats->syncp);
>> - put_cpu_ptr(tstats);
>> - } else {
>> - stats->tx_errors++;
>> - stats->tx_aborted_errors++;
>> - }
>> + if (likely(!net_xmit_eval(err)))
>> + err = pkt_len;
>> + iptunnel_xmit_stats(dev, err);
>
> I don't think this is an equivalent change.
> For example, if err == NET_XMIT_DROP, then '!net_xmit_eval(err)' is false
> and
> iptunnel_xmit_stats() is called with err set to a positive value
> (NET_XMIT_DROP
> value is 0x01), ie not an error.
I have sent out updated patch handle it correctly.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
Le 24/12/2015 13:46, Thadeu Lima de Souza Cascardo a écrit :
On Thu, Dec 24, 2015 at 10:21:27AM +0100, Nicolas Dichtel wrote:
Le 24/12/2015 00:52, Pravin B Shelar a écrit :
[snip]
diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 6af78c6..d63a911 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -182,15 +182,9 @@ static int tipc_udp_send_msg(struct net *net, struct
sk_buff *skb,
goto tx_error;
}
ttl = ip4_dst_hoplimit(&rt->dst);
- err = udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb,
- src->ipv4.s_addr,
- dst->ipv4.s_addr, 0, ttl, 0,
- src->udp_port, dst->udp_port,
- false, true);
- if (err < 0) {
- ip_rt_put(rt);
- goto tx_error;
- }
+ udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb, src->ipv4.s_addr,
+ dst->ipv4.s_addr, 0, ttl, 0, src->udp_port,
+ dst->udp_port, false, true);
I don't know how tipc works, but this change is clearly suspect. What make the
error path not needed anymore after your patch?
I looked into it as well. As far as I see, err could only be positive or 0, so
if there is a tipc bug here, Pravin's patch introduces no regression. Or did I
fail to see how udp_tunnel_xmit_skb could return a negative value?
You're probably right. But I think it needs a separate patch in that case.
Regards,
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
On Thu, Dec 24, 2015 at 10:21:27AM +0100, Nicolas Dichtel wrote:
> Le 24/12/2015 00:52, Pravin B Shelar a écrit :
> [snip]
> >diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
> >index 6af78c6..d63a911 100644
> >--- a/net/tipc/udp_media.c
> >+++ b/net/tipc/udp_media.c
> >@@ -182,15 +182,9 @@ static int tipc_udp_send_msg(struct net *net, struct
> >sk_buff *skb,
> > goto tx_error;
> > }
> > ttl = ip4_dst_hoplimit(&rt->dst);
> >-err = udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb,
> >- src->ipv4.s_addr,
> >- dst->ipv4.s_addr, 0, ttl, 0,
> >- src->udp_port, dst->udp_port,
> >- false, true);
> >-if (err < 0) {
> >-ip_rt_put(rt);
> >-goto tx_error;
> >-}
> >+udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb, src->ipv4.s_addr,
> >+dst->ipv4.s_addr, 0, ttl, 0, src->udp_port,
> >+dst->udp_port, false, true);
> I don't know how tipc works, but this change is clearly suspect. What make the
> error path not needed anymore after your patch?
I looked into it as well. As far as I see, err could only be positive or 0, so
if there is a tipc bug here, Pravin's patch introduces no regression. Or did I
fail to see how udp_tunnel_xmit_skb could return a negative value?
Cascardo.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
Le 24/12/2015 00:52, Pravin B Shelar a écrit :
[snip]
diff --git a/net/tipc/udp_media.c b/net/tipc/udp_media.c
index 6af78c6..d63a911 100644
--- a/net/tipc/udp_media.c
+++ b/net/tipc/udp_media.c
@@ -182,15 +182,9 @@ static int tipc_udp_send_msg(struct net *net, struct
sk_buff *skb,
goto tx_error;
}
ttl = ip4_dst_hoplimit(&rt->dst);
- err = udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb,
- src->ipv4.s_addr,
- dst->ipv4.s_addr, 0, ttl, 0,
- src->udp_port, dst->udp_port,
- false, true);
- if (err < 0) {
- ip_rt_put(rt);
- goto tx_error;
- }
+ udp_tunnel_xmit_skb(rt, ub->ubsock->sk, skb, src->ipv4.s_addr,
+ dst->ipv4.s_addr, 0, ttl, 0, src->udp_port,
+ dst->udp_port, false, true);
I don't know how tipc works, but this change is clearly suspect. What make the
error path not needed anymore after your patch?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
Le 24/12/2015 00:52, Pravin B Shelar a écrit :
[snip]
@@ -83,22 +84,12 @@ int ip6_tnl_get_iflink(const struct net_device *dev);
static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
struct net_device *dev)
{
- struct net_device_stats *stats = &dev->stats;
int pkt_len, err;
pkt_len = skb->len - skb_inner_network_offset(skb);
err = ip6_local_out(dev_net(skb_dst(skb)->dev), sk, skb);
-
- if (net_xmit_eval(err) == 0) {
- struct pcpu_sw_netstats *tstats = get_cpu_ptr(dev->tstats);
- u64_stats_update_begin(&tstats->syncp);
- tstats->tx_bytes += pkt_len;
- tstats->tx_packets++;
- u64_stats_update_end(&tstats->syncp);
- put_cpu_ptr(tstats);
- } else {
- stats->tx_errors++;
- stats->tx_aborted_errors++;
- }
+ if (likely(!net_xmit_eval(err)))
+ err = pkt_len;
+ iptunnel_xmit_stats(dev, err);
I don't think this is an equivalent change.
For example, if err == NET_XMIT_DROP, then '!net_xmit_eval(err)' is false and
iptunnel_xmit_stats() is called with err set to a positive value (NET_XMIT_DROP
value is 0x01), ie not an error.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
Le 24/12/2015 05:37, Pravin Shelar a écrit :
On Wed, Dec 23, 2015 at 6:57 PM, David Miller wrote:
From: Pravin B Shelar
Date: Wed, 23 Dec 2015 15:52:03 -0800
} else {
- err_stats->tx_dropped++;
+ struct net_device_stats *err_stats = &dev->stats;
+
+ if (err < 0) {
+ err_stats->tx_errors++;
+ err_stats->tx_aborted_errors++;
+ } else {
+ err_stats->tx_dropped++;
+ }
The original code did not have this "tx_dropped" code path
and you aren't explaining in your commit message why you
are adding this new behavior.
There is "tx_dropped" code path in existing iptunnel_xmit_stats(). I
have only moved err_stats variable definition to local block.
There is no new behavior in this patch.
Yes, I think it's ok now. There is no functional change. The prototype
of the function has changed: err_stats is not provided anymore in the arguments.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
On Wed, Dec 23, 2015 at 6:57 PM, David Miller wrote:
> From: Pravin B Shelar
> Date: Wed, 23 Dec 2015 15:52:03 -0800
>
>> } else {
>> - err_stats->tx_dropped++;
>> + struct net_device_stats *err_stats = &dev->stats;
>> +
>> + if (err < 0) {
>> + err_stats->tx_errors++;
>> + err_stats->tx_aborted_errors++;
>> + } else {
>> + err_stats->tx_dropped++;
>> + }
>
> The original code did not have this "tx_dropped" code path
> and you aren't explaining in your commit message why you
> are adding this new behavior.
There is "tx_dropped" code path in existing iptunnel_xmit_stats(). I
have only moved err_stats variable definition to local block.
There is no new behavior in this patch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
From: Pravin B Shelar
Date: Wed, 23 Dec 2015 15:52:03 -0800
> } else {
> - err_stats->tx_dropped++;
> + struct net_device_stats *err_stats = &dev->stats;
> +
> + if (err < 0) {
> + err_stats->tx_errors++;
> + err_stats->tx_aborted_errors++;
> + } else {
> + err_stats->tx_dropped++;
> + }
The original code did not have this "tx_dropped" code path
and you aren't explaining in your commit message why you
are adding this new behavior.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next v2] ip_tunnel: Move stats update to iptunnel_xmit()
By moving stats update into iptunnel_xmit(), we can simplify
iptunnel_xmit() usage. With this change there is no need to
call another function (iptunnel_xmit_stats()) to update stats
in tunnel xmit code path.
Signed-off-by: Pravin B Shelar
---
v1-v2:
- keep iptunnel_xmit_stats() stats update same.
---
drivers/net/geneve.c | 17 -
drivers/net/vxlan.c | 9 -
include/net/ip6_tunnel.h | 17 -
include/net/ip_tunnels.h | 24 +---
include/net/udp_tunnel.h | 8
net/ipv4/ip_gre.c | 7 +++
net/ipv4/ip_tunnel.c | 7 ++-
net/ipv4/ip_tunnel_core.c | 9 +
net/ipv4/ip_vti.c | 2 +-
net/ipv4/udp_tunnel.c | 11 +--
net/ipv6/sit.c| 7 ++-
net/tipc/udp_media.c | 12 +++-
12 files changed, 54 insertions(+), 76 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index e6e0092..20dd664 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -918,12 +918,11 @@ static netdev_tx_t geneve_xmit_skb(struct sk_buff *skb,
struct net_device *dev,
ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);
df = 0;
}
- err = udp_tunnel_xmit_skb(rt, gs4->sock->sk, skb, fl4.saddr, fl4.daddr,
- tos, ttl, df, sport, geneve->dst_port,
- !net_eq(geneve->net, dev_net(geneve->dev)),
- !(flags & GENEVE_F_UDP_CSUM));
+ udp_tunnel_xmit_skb(rt, gs4->sock->sk, skb, fl4.saddr, fl4.daddr,
+ tos, ttl, df, sport, geneve->dst_port,
+ !net_eq(geneve->net, dev_net(geneve->dev)),
+ !(flags & GENEVE_F_UDP_CSUM));
- iptunnel_xmit_stats(err, &dev->stats, dev->tstats);
return NETDEV_TX_OK;
tx_error:
@@ -1005,10 +1004,10 @@ static netdev_tx_t geneve6_xmit_skb(struct sk_buff
*skb, struct net_device *dev,
ttl = 1;
ttl = ttl ? : ip6_dst_hoplimit(dst);
}
- err = udp_tunnel6_xmit_skb(dst, gs6->sock->sk, skb, dev,
- &fl6.saddr, &fl6.daddr, prio, ttl,
- sport, geneve->dst_port,
- !!(flags & GENEVE_F_UDP_ZERO_CSUM6_TX));
+ udp_tunnel6_xmit_skb(dst, gs6->sock->sk, skb, dev,
+&fl6.saddr, &fl6.daddr, prio, ttl,
+sport, geneve->dst_port,
+!!(flags & GENEVE_F_UDP_ZERO_CSUM6_TX));
return NETDEV_TX_OK;
tx_error:
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index ba363ce..fecf7b6 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1841,9 +1841,10 @@ static int vxlan_xmit_skb(struct rtable *rt, struct sock
*sk, struct sk_buff *sk
skb_set_inner_protocol(skb, htons(ETH_P_TEB));
- return udp_tunnel_xmit_skb(rt, sk, skb, src, dst, tos,
- ttl, df, src_port, dst_port, xnet,
- !(vxflags & VXLAN_F_UDP_CSUM));
+ udp_tunnel_xmit_skb(rt, sk, skb, src, dst, tos, ttl, df,
+ src_port, dst_port, xnet,
+ !(vxflags & VXLAN_F_UDP_CSUM));
+ return 0;
}
#if IS_ENABLED(CONFIG_IPV6)
@@ -2056,8 +2057,6 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct
net_device *dev,
skb = NULL;
goto rt_tx_error;
}
-
- iptunnel_xmit_stats(err, &dev->stats, dev->tstats);
#if IS_ENABLED(CONFIG_IPV6)
} else {
struct dst_entry *ndst;
diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
index ff788b6..2ef06cd 100644
--- a/include/net/ip6_tunnel.h
+++ b/include/net/ip6_tunnel.h
@@ -5,6 +5,7 @@
#include
#include
#include
+#include
#define IP6TUNNEL_ERR_TIMEO (30*HZ)
@@ -83,22 +84,12 @@ int ip6_tnl_get_iflink(const struct net_device *dev);
static inline void ip6tunnel_xmit(struct sock *sk, struct sk_buff *skb,
struct net_device *dev)
{
- struct net_device_stats *stats = &dev->stats;
int pkt_len, err;
pkt_len = skb->len - skb_inner_network_offset(skb);
err = ip6_local_out(dev_net(skb_dst(skb)->dev), sk, skb);
-
- if (net_xmit_eval(err) == 0) {
- struct pcpu_sw_netstats *tstats = get_cpu_ptr(dev->tstats);
- u64_stats_update_begin(&tstats->syncp);
- tstats->tx_bytes += pkt_len;
- tstats->tx_packets++;
- u64_stats_update_end(&tstats->syncp);
- put_cpu_ptr(tstats);
- } else {
- stats->tx_errors++;
- stats->tx_aborted_errors++;
- }
+ if (likely(!net_xmit_eval(err)))
+ err = pkt_len;
+ iptunnel_xmit_stats(dev, err);
}
#endif
diff --git a/in
