[PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[Harold Huang]

The gso size is calculated with all of the headers and payload. As a
result, the l2 header should not be included when comparing gso size
with mtu.

Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
Cc: stable@dpdk.org
Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index f1b48cae82..2b561d232c 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -731,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 					mbuf_in->l4_len;
 			tso_segsz = mbuf_in->tso_segsz + hdrs_len;
 			if (unlikely(tso_segsz == hdrs_len) ||
-				tso_segsz > *txq->mtu) {
+				tso_segsz > *txq->mtu + mbuf_in->l2_len) {
 				txq->stats.errs++;
 				break;
 			}
-- 
2.27.0

Re: [PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[Ferruh Yigit]

On 2/28/2022 8:27 AM, Harold Huang wrote:
> The gso size is calculated with all of the headers and payload. As a
> result, the l2 header should not be included when comparing gso size
> with mtu.
> 
> Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
> Cc: stable@dpdk.org
> Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> ---
>   drivers/net/tap/rte_eth_tap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index f1b48cae82..2b561d232c 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -731,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   					mbuf_in->l4_len;
>   			tso_segsz = mbuf_in->tso_segsz + hdrs_len;
>   			if (unlikely(tso_segsz == hdrs_len) ||
> -				tso_segsz > *txq->mtu) {
> +				tso_segsz > *txq->mtu + mbuf_in->l2_len) {
>   				txq->stats.errs++;
>   				break;
>   			}

update emails for Ophir & Raslan.

Hi Ophir, since original code is from you can you please
help on review?

Re: [PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[Harold Huang]

On Mon, Feb 28, 2022 at 4:27 PM Harold Huang <baymaxhuang@gmail.com> wrote:
>
> The gso size is calculated with all of the headers and payload. As a
> result, the l2 header should not be included when comparing gso size
> with mtu.
>
> Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
> Cc: stable@dpdk.org
> Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index f1b48cae82..2b561d232c 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -731,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>                                         mbuf_in->l4_len;
>                         tso_segsz = mbuf_in->tso_segsz + hdrs_len;
>                         if (unlikely(tso_segsz == hdrs_len) ||
> -                               tso_segsz > *txq->mtu) {
> +                               tso_segsz > *txq->mtu + mbuf_in->l2_len) {
>                                 txq->stats.errs++;
>                                 break;
>                         }
> --
> 2.27.0
>

Hi, Jiayu,

This is the only example in the driver to use GSO. I think it is
important for us to calculate a correct GSO size. I see you are the
GSO lib maintainer, could you please help review this patch?

RE: [PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[Ophir Munk]

The effective limit in Tx is the maximum packet size and not the MTU.
On my Ethernet NIC the MTU size is 1500 bytes. As long as I send packets
up to 1514 bytes size (including an Ethernet header of 14 bytes) - TX succeeds.
Conversely, if I add to the packet VLAN, Q-in-Q, VXLAX or any other L2 headers - TX fails
This is because the NIC's maximum packet size is 1514 bytes, regardless of the L2 header size.
(Remark: NIC max size may include 4 bytes CRC as well).
Having said that - I suggest setting up an MTU TAP that considers the maximum supported packet size.
Using mbuf_in-> l2_len in calculating the tso_segsz limit - does not seem right to me.

> -----Original Message-----
> From: Harold Huang <baymaxhuang@gmail.com>
> Sent: Tuesday, March 8, 2022 4:35 PM
> To: dev@dpdk.org
> Cc: Ophir Munk <ophirmu@nvidia.com>; stable@dpdk.org; Keith Wiles
> <keith.wiles@intel.com>; Raslan Darawsheh <rasland@nvidia.com>;
> jiayu.hu@intel.com
> Subject: Re: [PATCH] net/tap: do not include l2 header in gso size when
> compared with mtu
> 
> On Mon, Feb 28, 2022 at 4:27 PM Harold Huang <baymaxhuang@gmail.com>
> wrote:
> >
> > The gso size is calculated with all of the headers and payload. As a
> > result, the l2 header should not be included when comparing gso size
> > with mtu.
> >
> > Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
> > Cc: stable@dpdk.org
> > Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> > b/drivers/net/tap/rte_eth_tap.c index f1b48cae82..2b561d232c 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -731,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> >                                         mbuf_in->l4_len;
> >                         tso_segsz = mbuf_in->tso_segsz + hdrs_len;
> >                         if (unlikely(tso_segsz == hdrs_len) ||
> > -                               tso_segsz > *txq->mtu) {
> > +                               tso_segsz > *txq->mtu +
> > + mbuf_in->l2_len) {
> >                                 txq->stats.errs++;
> >                                 break;
> >                         }
> > --
> > 2.27.0
> >
> 
> Hi, Jiayu,
> 
> This is the only example in the driver to use GSO. I think it is important for us
> to calculate a correct GSO size. I see you are the GSO lib maintainer, could
> you please help review this patch?

Re: [PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[Harold Huang]

Hello,

On Fri, Mar 18, 2022 at 5:22 PM Ophir Munk <ophirmu@nvidia.com> wrote:
>
> The effective limit in Tx is the maximum packet size and not the MTU.
> On my Ethernet NIC the MTU size is 1500 bytes. As long as I send packets
> up to 1514 bytes size (including an Ethernet header of 14 bytes) - TX succeeds.
> Conversely, if I add to the packet VLAN, Q-in-Q, VXLAX or any other L2 headers - TX fails

How did you test the largest packet with vlan tag? IMO, vlan is a l2
header which is not included in MTU.  And in my testbed, if I add a
vlan tag in a Ethernet NIC with  mtu 1500, the largest frame is
1518(1514+4), the tcpdump result  is showed as followed:

10:43:08.847060 6c:92:bf:84:b4:7f > 6c:92:bf:89:9f:77, ethertype
802.1Q (0x8100), length 1518: vlan 8, p 0, ethertype IPv4, (tos 0x0,
ttl 64, id 47606, offset 0, flags [none], proto TCP (6), length 1500)
    172.16.2.2.43754 > 172.16.2.3.5201: Flags [.], cksum 0x61f4
(incorrect -> 0x8ffb), seq 412040:413488, ack 1, win 229, options
[nop,nop,TS val 1557628892 ecr 2075223301], length 1448
10:43:08.847061 6c:92:bf:84:b4:7f > 6c:92:bf:89:9f:77, ethertype
802.1Q (0x8100), length 1518: vlan 8, p 0, ethertype IPv4, (tos 0x0,
ttl 64, id 47607, offset 0, flags [none], proto TCP (6), length 1500)
    172.16.2.2.43754 > 172.16.2.3.5201: Flags [.], cksum 0x61f4
(incorrect -> 0x69cf), seq 413488:414936, ack 1, win 229, options
[nop,nop,TS val 1557628892 ecr 2075223301], length 1448

Furthermore, in the tap driver, I see gso_types is set as
RTE_ETH_TX_OFFLOAD_TCP_TSO, ie. VXLAN is not supported. As a result. I
think gso_size could not consider VXLAN Packet here.

> This is because the NIC's maximum packet size is 1514 bytes, regardless of the L2 header size.
> (Remark: NIC max size may include 4 bytes CRC as well).
> Having said that - I suggest setting up an MTU TAP that considers the maximum supported packet size.
> Using mbuf_in-> l2_len in calculating the tso_segsz limit - does not seem right to me.
>
> > -----Original Message-----
> > From: Harold Huang <baymaxhuang@gmail.com>
> > Sent: Tuesday, March 8, 2022 4:35 PM
> > To: dev@dpdk.org
> > Cc: Ophir Munk <ophirmu@nvidia.com>; stable@dpdk.org; Keith Wiles
> > <keith.wiles@intel.com>; Raslan Darawsheh <rasland@nvidia.com>;
> > jiayu.hu@intel.com
> > Subject: Re: [PATCH] net/tap: do not include l2 header in gso size when
> > compared with mtu
> >
> > On Mon, Feb 28, 2022 at 4:27 PM Harold Huang <baymaxhuang@gmail.com>
> > wrote:
> > >
> > > The gso size is calculated with all of the headers and payload. As a
> > > result, the l2 header should not be included when comparing gso size
> > > with mtu.
> > >
> > > Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
> > > Cc: stable@dpdk.org
> > > Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> > > ---
> > >  drivers/net/tap/rte_eth_tap.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/tap/rte_eth_tap.c
> > > b/drivers/net/tap/rte_eth_tap.c index f1b48cae82..2b561d232c 100644
> > > --- a/drivers/net/tap/rte_eth_tap.c
> > > +++ b/drivers/net/tap/rte_eth_tap.c
> > > @@ -731,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf
> > **bufs, uint16_t nb_pkts)
> > >                                         mbuf_in->l4_len;
> > >                         tso_segsz = mbuf_in->tso_segsz + hdrs_len;
> > >                         if (unlikely(tso_segsz == hdrs_len) ||
> > > -                               tso_segsz > *txq->mtu) {
> > > +                               tso_segsz > *txq->mtu +
> > > + mbuf_in->l2_len) {
> > >                                 txq->stats.errs++;
> > >                                 break;
> > >                         }
> > > --
> > > 2.27.0
> > >
> >
> > Hi, Jiayu,
> >
> > This is the only example in the driver to use GSO. I think it is important for us
> > to calculate a correct GSO size. I see you are the GSO lib maintainer, could
> > you please help review this patch?

Thanks
Harold

Re: [PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[Ferruh Yigit]

On 3/8/2022 2:35 PM, Harold Huang wrote:
> On Mon, Feb 28, 2022 at 4:27 PM Harold Huang <baymaxhuang@gmail.com> wrote:
>>
>> The gso size is calculated with all of the headers and payload. As a
>> result, the l2 header should not be included when comparing gso size
>> with mtu.
>>
>> Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
>> Cc: stable@dpdk.org
>> Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
>> ---
>>   drivers/net/tap/rte_eth_tap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index f1b48cae82..2b561d232c 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -731,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>>                                          mbuf_in->l4_len;
>>                          tso_segsz = mbuf_in->tso_segsz + hdrs_len;
>>                          if (unlikely(tso_segsz == hdrs_len) ||
>> -                               tso_segsz > *txq->mtu) {
>> +                               tso_segsz > *txq->mtu + mbuf_in->l2_len) {
>>                                  txq->stats.errs++;
>>                                  break;
>>                          }
>> --
>> 2.27.0
>>
> 
> Hi, Jiayu,
> 
> This is the only example in the driver to use GSO. I think it is
> important for us to calculate a correct GSO size. I see you are the
> GSO lib maintainer, could you please help review this patch?


Hi Jiayu, Ophir,

Can you please review this patch?

For reference, patchwork link:
https://patches.dpdk.org/project/dpdk/patch/20220228082724.1646930-1-baymaxhuang@gmail.com/

Thanks,
ferruh

RE: [PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[Ophir Munk]

Hi all,

Please note that max size is calculated in the same function (drivers/net/tap/rte_eth_tap.c) as:
 max_size = *txq->mtu + (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + 4);

Since tso_setgsz should not exceed the max packet size - the desired update should be:
                      tso_segsz > max_size

rather than the suggested one:

                      tso_segsz > *txq->mtu + mbuf_in->l2_len)

Regards,
Ophir

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Saturday, May 21, 2022 1:08 AM
> To: Harold Huang <baymaxhuang@gmail.com>; dev@dpdk.org;
> jiayu.hu@intel.com; Ophir Munk <ophirmu@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>
> Cc: stable@dpdk.org; Keith Wiles <keith.wiles@intel.com>
> Subject: Re: [PATCH] net/tap: do not include l2 header in gso size when
> compared with mtu
> 
> On 3/8/2022 2:35 PM, Harold Huang wrote:
> > On Mon, Feb 28, 2022 at 4:27 PM Harold Huang
> <baymaxhuang@gmail.com> wrote:
> >>
> >> The gso size is calculated with all of the headers and payload. As a
> >> result, the l2 header should not be included when comparing gso size
> >> with mtu.
> >>
> >> Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
> >> Cc: stable@dpdk.org
> >> Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> >> ---
> >>   drivers/net/tap/rte_eth_tap.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/tap/rte_eth_tap.c
> >> b/drivers/net/tap/rte_eth_tap.c index f1b48cae82..2b561d232c 100644
> >> --- a/drivers/net/tap/rte_eth_tap.c
> >> +++ b/drivers/net/tap/rte_eth_tap.c
> >> @@ -731,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> >>                                          mbuf_in->l4_len;
> >>                          tso_segsz = mbuf_in->tso_segsz + hdrs_len;
> >>                          if (unlikely(tso_segsz == hdrs_len) ||
> >> -                               tso_segsz > *txq->mtu) {
> >> +                               tso_segsz > *txq->mtu +
> >> + mbuf_in->l2_len) {
> >>                                  txq->stats.errs++;
> >>                                  break;
> >>                          }
> >> --
> >> 2.27.0
> >>
> >
> > Hi, Jiayu,
> >
> > This is the only example in the driver to use GSO. I think it is
> > important for us to calculate a correct GSO size. I see you are the
> > GSO lib maintainer, could you please help review this patch?
> 
> 
> Hi Jiayu, Ophir,
> 
> Can you please review this patch?
> 
> For reference, patchwork link:
> https://patches.dpdk.org/project/dpdk/patch/20220228082724.1646930-1-
> baymaxhuang@gmail.com/
> 
> Thanks,
> ferruh

Re: [PATCH] net/tap: do not include l2 header in gso size when compared with mtu

[Stephen Hemminger]

On Tue, 8 Mar 2022 22:35:18 +0800
Harold Huang <baymaxhuang@gmail.com> wrote:

> On Mon, Feb 28, 2022 at 4:27 PM Harold Huang <baymaxhuang@gmail.com> wrote:
> >
> > The gso size is calculated with all of the headers and payload. As a
> > result, the l2 header should not be included when comparing gso size
> > with mtu.
> >
> > Fixes: 050316a88313 ("net/tap: support TSO (TCP Segment Offload)")
> > Cc: stable@dpdk.org
> > Signed-off-by: Harold Huang <baymaxhuang@gmail.com>
> > ---
> >  drivers/net/tap/rte_eth_tap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> > index f1b48cae82..2b561d232c 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -731,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> >                                         mbuf_in->l4_len;
> >                         tso_segsz = mbuf_in->tso_segsz + hdrs_len;
> >                         if (unlikely(tso_segsz == hdrs_len) ||
> > -                               tso_segsz > *txq->mtu) {
> > +                               tso_segsz > *txq->mtu + mbuf_in->l2_len) {
> >                                 txq->stats.errs++;
> >                                 break;
> >                         }
> > --
> > 2.27.0
> >  
> 
> Hi, Jiayu,
> 
> This is the only example in the driver to use GSO. I think it is
> important for us to calculate a correct GSO size. I see you are the
> GSO lib maintainer, could you please help review this patch?

See Ophir's comment and address it in new version

Please note that max size is calculated in the same function (drivers/net/tap/rte_eth_tap.c) as:
 max_size = *txq->mtu + (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + 4);

Since tso_setgsz should not exceed the max packet size - the desired update should be:
                      tso_segsz > max_size

rather than the suggested one:

                      tso_segsz > *txq->mtu + mbuf_in->l2_len)