diff mbox series

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

Message ID 20220228082724.1646930-1-baymaxhuang@gmail.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series net/tap: do not include l2 header in gso size when compared with mtu |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Harold Huang Feb. 28, 2022, 8:27 a.m. UTC 
  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(-)

Comments

Ferruh Yigit March 4, 2022, 4:30 p.m. UTC | #1 
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?
Harold Huang March 8, 2022, 2:35 p.m. UTC | #2 
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?
Ferruh Yigit May 20, 2022, 10:08 p.m. UTC | #3 
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
Ophir Munk May 24, 2022, 2:01 p.m. UTC | #4
Unaddressed 
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
Stephen Hemminger Oct. 17, 2023, 4:47 p.m. UTC | #5 
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)
diff mbox series

Patch

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;
 			}