[PATCH RFC net-next] net: vlan/macvlan: count packets properly with gso

[PATCH RFC net-next] net: vlan/macvlan: count packets properly with gso

[Debabrata Banerjee]

Fix packet count when using vlan/macvlan drivers with gso. Without this it
is not possible to reconcile packet counts between underlying devices and
these virtual devices. Additionally, the output looks wrong in a standalone
way i.e. device MTU of 1500, 1 packet sent, 31856 bytes sent.

There are many other drivers that likely have a similar problem, although
it is not clear how many of those could be used with gso. Perhaps all
packet counting should be wrapped in a helper fn.

Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
---
 drivers/net/macvlan.c | 2 +-
 net/8021q/vlan_core.c | 2 +-
 net/8021q/vlan_dev.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index fc8d5f1ee1ad..15e67a87f202 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -566,7 +566,7 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
 
 		pcpu_stats = this_cpu_ptr(vlan->pcpu_stats);
 		u64_stats_update_begin(&pcpu_stats->syncp);
-		pcpu_stats->tx_packets++;
+		pcpu_stats->tx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
 		pcpu_stats->tx_bytes += len;
 		u64_stats_update_end(&pcpu_stats->syncp);
 	} else {
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index a313165e7a67..e85f6665d0ed 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -62,7 +62,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
 	rx_stats = this_cpu_ptr(vlan_dev_priv(vlan_dev)->vlan_pcpu_stats);
 
 	u64_stats_update_begin(&rx_stats->syncp);
-	rx_stats->rx_packets++;
+	rx_stats->rx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
 	rx_stats->rx_bytes += skb->len;
 	if (skb->pkt_type == PACKET_MULTICAST)
 		rx_stats->rx_multicast++;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index b2d9c8f27cd7..b28e7535a0b9 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -135,7 +135,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
 
 		stats = this_cpu_ptr(vlan->vlan_pcpu_stats);
 		u64_stats_update_begin(&stats->syncp);
-		stats->tx_packets++;
+		stats->tx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
 		stats->tx_bytes += len;
 		u64_stats_update_end(&stats->syncp);
 	} else {
-- 
2.19.2

Re: [PATCH RFC net-next] net: vlan/macvlan: count packets properly with gso

[Duyck, Alexander H]

[-- Attachment #1: Type: text/plain, Size: 2893 bytes --]

On Thu, 2018-11-29 at 15:58 -0500, Debabrata Banerjee wrote:
> Fix packet count when using vlan/macvlan drivers with gso. Without this it
> is not possible to reconcile packet counts between underlying devices and
> these virtual devices. Additionally, the output looks wrong in a standalone
> way i.e. device MTU of 1500, 1 packet sent, 31856 bytes sent.
> 
> There are many other drivers that likely have a similar problem, although
> it is not clear how many of those could be used with gso. Perhaps all
> packet counting should be wrapped in a helper fn.
> 
> Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
> ---
>  drivers/net/macvlan.c | 2 +-
>  net/8021q/vlan_core.c | 2 +-
>  net/8021q/vlan_dev.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index fc8d5f1ee1ad..15e67a87f202 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -566,7 +566,7 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
>  
>  		pcpu_stats = this_cpu_ptr(vlan->pcpu_stats);
>  		u64_stats_update_begin(&pcpu_stats->syncp);
> -		pcpu_stats->tx_packets++;
> +		pcpu_stats->tx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
>  		pcpu_stats->tx_bytes += len;
>  		u64_stats_update_end(&pcpu_stats->syncp);
>  	} else {
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index a313165e7a67..e85f6665d0ed 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -62,7 +62,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
>  	rx_stats = this_cpu_ptr(vlan_dev_priv(vlan_dev)->vlan_pcpu_stats);
>  
>  	u64_stats_update_begin(&rx_stats->syncp);
> -	rx_stats->rx_packets++;
> +	rx_stats->rx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
>  	rx_stats->rx_bytes += skb->len;
>  	if (skb->pkt_type == PACKET_MULTICAST)
>  		rx_stats->rx_multicast++;
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index b2d9c8f27cd7..b28e7535a0b9 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -135,7 +135,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
>  
>  		stats = this_cpu_ptr(vlan->vlan_pcpu_stats);
>  		u64_stats_update_begin(&stats->syncp);
> -		stats->tx_packets++;
> +		stats->tx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
>  		stats->tx_bytes += len;
>  		u64_stats_update_end(&stats->syncp);
>  	} else {

Instead of just checking for the max it might make more sense to do a
check using skb_is_gso, and then if true use gso_segs, otherwise just
default to 1.

Also your bytes are going to be totally messed up as well since the
headers are trimmed in the GSO frames. It might be worthwhile to just
have a branch based on skb_is_gso that sets the packets and bytes based
on the GSO values, and one that sets it for the default case.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]

Re: [PATCH RFC net-next] net: vlan/macvlan: count packets properly with gso

[Eric Dumazet]

On 11/29/2018 01:13 PM, Duyck, Alexander H wrote:

> Instead of just checking for the max it might make more sense to do a
> check using skb_is_gso, and then if true use gso_segs, otherwise just
> default to 1.
> 
> Also your bytes are going to be totally messed up as well since the
> headers are trimmed in the GSO frames. It might be worthwhile to just
> have a branch based on skb_is_gso that sets the packets and bytes based
> on the GSO values, and one that sets it for the default case.
> 
Note that __dev_queue_xmit() is already doing that, no need
to re-implement in each driver.

It calls qdisc_pkt_len_init(), meaning that drivers can use 
qdisc_skb_cb(skb)->pkt_len instead of skb->len

(Qdisc layers should not modify qdisc_skb_cb(skb)->pkt_len )

RE: [PATCH RFC net-next] net: vlan/macvlan: count packets properly with gso

[Banerjee, Debabrata]

> From: Eric Dumazet <eric.dumazet@gmail.com>
> On 11/29/2018 01:13 PM, Duyck, Alexander H wrote:
> >
> > Also your bytes are going to be totally messed up as well since the
> > headers are trimmed in the GSO frames. It might be worthwhile to just
> > have a branch based on skb_is_gso that sets the packets and bytes
> > based on the GSO values, and one that sets it for the default case.
> >
> Note that __dev_queue_xmit() is already doing that, no need to re-
> implement in each driver.
> 
> It calls qdisc_pkt_len_init(), meaning that drivers can use qdisc_skb_cb(skb)-
> >pkt_len instead of skb->len
> 
> (Qdisc layers should not modify qdisc_skb_cb(skb)->pkt_len )

This works. But rx bytes are still a problem. I haven't spotted anything quick for this.

Re: [PATCH RFC net-next] net: vlan/macvlan: count packets properly with gso

[Cong Wang]

On Thu, Nov 29, 2018 at 1:46 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 11/29/2018 01:13 PM, Duyck, Alexander H wrote:
>
> > Instead of just checking for the max it might make more sense to do a
> > check using skb_is_gso, and then if true use gso_segs, otherwise just
> > default to 1.
> >
> > Also your bytes are going to be totally messed up as well since the
> > headers are trimmed in the GSO frames. It might be worthwhile to just
> > have a branch based on skb_is_gso that sets the packets and bytes based
> > on the GSO values, and one that sets it for the default case.
> >
> Note that __dev_queue_xmit() is already doing that, no need
> to re-implement in each driver.
>
> It calls qdisc_pkt_len_init(), meaning that drivers can use
> qdisc_skb_cb(skb)->pkt_len instead of skb->len
>
> (Qdisc layers should not modify qdisc_skb_cb(skb)->pkt_len )

It should not modify, but, on the other hand, it is neither supposed
to be used for non-qdisc layer. At very least, you have to audit
each ->ndo_start_xmit() to see if it uses its own skb->cb
first.