From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CC02543B74; Thu, 22 Feb 2024 12:15:37 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A8E21402CE; Thu, 22 Feb 2024 12:15:37 +0100 (CET) Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by mails.dpdk.org (Postfix) with ESMTP id B7C4A40281 for ; Thu, 22 Feb 2024 12:15:36 +0100 (CET) Received: by mail-ed1-f51.google.com with SMTP id 4fb4d7f45d1cf-563bb51c36eso8649168a12.2 for ; Thu, 22 Feb 2024 03:15:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708600536; x=1709205336; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=KaB3DD/zY3sgJWviSzw3vln3KyD9dxqSUi6ZaMQj9ms=; b=kCISaaWUjh2bolpy9mvPAlLX6QnB0PFvPgKA0vYwBj3J2WFOb4gnotcmrWg1LjqjFQ 7eAw8XYc3FZmseiN4DaZXi56q9xgVYmZHnmch2rhj7YcFQAONEJIRV8jOKgMh890CHB3 N5oxkBuNlKiApMr8xNqF06lEK/qsObr+ieFhYgfi1+w7hzIgz+VF0FmWHwJBCH3iu77P 1akBQHTxDJ8I1oA9zjRKdF38ircPkwhSpTLhSDDT6hwSpYQSSMPgkjGnvf1zJsZIhNCB 715p5sNMw0MQEcT13liA1QIsqdcC4tTjdfejieUmTRb3AdeyKqHBk135mzfMujhFJr0z Pm/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708600536; x=1709205336; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=KaB3DD/zY3sgJWviSzw3vln3KyD9dxqSUi6ZaMQj9ms=; b=vuo/VlLemQ0G0gWdMo1NcjVUdsU8mX7MfPCGbvDoKPtvcA8Q/8ULJmpzeQC9bHZhuv wx0ugXp2ZLWtAScEkDnWaERaWmWFrKbHRktYmbRd41WuXA8gVL7k8mPzx2p0+sjOsNHK xDPSez7V8L4a3fsUs2FJil+4IjWU6PEyKzZKfpymwsU94lfKI1bq/wKUEsRy+bkyh4vY gehxhiKrKZoYb2L8BAilDVGOTRt8jorSGo80vvGy0fW/ND4c3anImYIVLTUaUAmHiLkT /b1Jy5v1zJwQwNgHiv3x3bhQNquuLzTJdvNs/F8E5X37KmCmbhIZaM8Tq7nETolEx+2m 0nig== X-Forwarded-Encrypted: i=1; AJvYcCWgErZ8wXkFN2fY34n72D/JOAMpOP4x/uTc5QEb5LB9FYkdObITe1V66Y1mybTxgW8bHKyJw5zMz3gKr+A= X-Gm-Message-State: AOJu0YzKDVdqbrf66dKbRNqJ3qcx+IrpdNucMlteyG9I5rpvrAios4Ws dAmXBXZ8BhuD+O6vS7YWjkSCsyfaKVHXgTTXGp6v3eZmzKIZffBOOLz1IgKGlCfXLYWmdmVF0ql ZKaurVafpg9MRP4XkpBJCszfX4qg= X-Google-Smtp-Source: AGHT+IF2cNytQXWGGjimVp8gGcWpIZ1tZryNBJUsDW80rhobjRu5FkO7HTSmVBhdd0UHaBsEHqWZKsXDO8BJ+EkIuck= X-Received: by 2002:aa7:c456:0:b0:565:4b37:d2c6 with SMTP id n22-20020aa7c456000000b005654b37d2c6mr333245edr.7.1708600536003; Thu, 22 Feb 2024 03:15:36 -0800 (PST) MIME-Version: 1.0 References: <20240117195228.423261-1-kumaraparamesh92@gmail.com> <20240216034750.47539-1-kumaraparamesh92@gmail.com> <8c3d602b-b83b-4cb6-af04-f51576d72284@amd.com> <5ccd235e-aae7-4ac9-8eef-fa6e0483b1cc@amd.com> In-Reply-To: <5ccd235e-aae7-4ac9-8eef-fa6e0483b1cc@amd.com> From: kumaraparameshwaran rathinavel Date: Thu, 22 Feb 2024 16:45:24 +0530 Message-ID: Subject: Re: [PATCH v8] app/testpmd : fix packets not getting flushed in heavy-weight mode API To: Ferruh Yigit Cc: hujiayu.hu@foxmail.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="00000000000002f7eb0611f69062" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --00000000000002f7eb0611f69062 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Feb 21, 2024 at 11:32=E2=80=AFPM Ferruh Yigit wrote: > On 2/16/2024 1:56 PM, Ferruh Yigit wrote: > > On 2/16/2024 3:47 AM, Kumara Parameshwaran wrote: > >> In heavy-weight mode GRO which is based on timer, the GRO packets > >> will not be flushed in spite of timer expiry if there is no packet > >> in the current poll. If timer mode GRO is enabled the > >> rte_gro_timeout_flush API should be invoked. > >> > >> Fixes: b7091f1dcfbc ("app/testpmd: enable the heavyweight mode TCP/IPv= 4 > GRO") > >> Cc: hujiayu.hu@foxmail.com > >> > >> Signed-off-by: Kumara Parameshwaran > >> --- > >> v1: > >> Changes to make sure that the GRO flush API is invoked if there ar= e > no packets in > >> current poll and timer expiry. > >> > >> v2: > >> Fix code organisation issue > >> > >> v3: > >> Fix warnings > >> > >> v4: > >> Fix error and warnings > >> > >> v5: > >> Fix compilation issue when GRO is not defined > >> > >> v6: > >> Address review comments > >> > >> v7: > >> Address review comments > >> > >> v8: > >> Fix spell check warnings > >> > >> app/test-pmd/csumonly.c | 22 ++++++++++++++++++---- > >> 1 file changed, 18 insertions(+), 4 deletions(-) > >> > >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c > >> index c103e54111..a922160f6d 100644 > >> --- a/app/test-pmd/csumonly.c > >> +++ b/app/test-pmd/csumonly.c > >> @@ -863,16 +863,29 @@ pkt_burst_checksum_forward(struct fwd_stream *fs= ) > >> > >> /* receive a burst of packet */ > >> nb_rx =3D common_fwd_stream_receive(fs, pkts_burst, > nb_pkt_per_burst); > >> - if (unlikely(nb_rx =3D=3D 0)) > >> + if (unlikely(nb_rx =3D=3D 0)) { > >> +#ifndef RTE_LIB_GRO > >> return false; > >> +#else > >> + gro_enable =3D gro_ports[fs->rx_port].enable; > >> + /* > >> + * Make sure that in case of Heavyweight mode GRO the > packets in > >> + * GRO cache should be flushed as the timer could have > expired. > >> + * > >> + * The order of conditions should be the same as gro_ctx > is valid > >> + * only when gro_flush_cycles is not the > GRO_DEFAULT_FLUSH_CYCLES which > >> + * indicates light weight mode GRO > >> + */ > >> > > > > Updated comment as below to make it terse, what do you think: > > /* > > * Check if packets need to be flushed in the GRO context > > * due to a timeout. > > * > > * Continue only in GRO heavyweight mode and if there are > > * packets in the GRO context. > > */ > > > > > >> + if (!gro_enable || (gro_flush_cycles =3D=3D > GRO_DEFAULT_FLUSH_CYCLES) || > >> + > (rte_gro_get_pkt_count(current_fwd_lcore()->gro_ctx) =3D=3D 0)) > >> + return false; > >> +#endif > >> + } > >> > > > > Another issue but also related to your patch, if there is no packet to > > Tx after GRO block, should we add another zero packet check: > > if (unlikely(nb_rx =3D=3D 0)) > > return false; > > > > To prevent executing GSO and Tx path code with zero packet, do you thin= k > > does this make sense? > > > > > > Patch looks good to me, with above comment update, but I am worried > about side impacts of this patch that we might be missing, that is why I > would like it to be in -rc1, so that it can be tested better. Hence, > > > Reviewed-by: Ferruh Yigit > > Applied to dpdk-next-net/main, thanks. > (Updated comment as suggested above while merging.) > > > Lets continue to discuss return on "nb_rx =3D=3D 0" case after GRO block, > incremental to this patch. > > I was not able to get to this. I will also take a look at the code to see > if this can cause any issues. > Thanks. > > --00000000000002f7eb0611f69062 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Feb 21, 2024 at 11:32=E2=80= =AFPM Ferruh Yigit <ferruh.yigit= @amd.com> wrote:
On 2/16/2024 1:56 PM, Ferruh Yigit wrote:
> On 2/16/2024 3:47 AM, Kumara Parameshwaran wrote:
>> In heavy-weight mode GRO which is based on timer, the GRO packets<= br> >> will not be flushed in spite of timer expiry if there is no packet=
>> in the current poll. If timer mode GRO is enabled the
>> rte_gro_timeout_flush API should be invoked.
>>
>> Fixes: b7091f1dcfbc ("app/testpmd: enable the heavyweight mod= e TCP/IPv4 GRO")
>> Cc: hu= jiayu.hu@foxmail.com
>>
>> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com> >> ---
>> v1:
>>=C2=A0 =C2=A0 =C2=A0Changes to make sure that the GRO flush API is = invoked if there are no packets in
>>=C2=A0 =C2=A0 =C2=A0current poll and timer expiry.
>>
>> v2:
>>=C2=A0 =C2=A0 =C2=A0Fix code organisation issue
>>
>> v3:
>>=C2=A0 =C2=A0 =C2=A0Fix warnings
>>
>> v4:
>>=C2=A0 =C2=A0 =C2=A0Fix error and warnings
>>
>> v5:
>>=C2=A0 =C2=A0 =C2=A0Fix compilation issue when GRO is not defined >>
>> v6:
>>=C2=A0 =C2=A0 =C2=A0Address review comments
>>
>> v7:
>>=C2=A0 =C2=A0 =C2=A0Address review comments
>>
>> v8:
>>=C2=A0 =C2=A0 =C2=A0Fix spell check warnings
>>
>>=C2=A0 app/test-pmd/csumonly.c | 22 ++++++++++++++++++----
>>=C2=A0 1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index c103e54111..a922160f6d 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -863,16 +863,29 @@ pkt_burst_checksum_forward(struct fwd_stream= *fs)
>>=C2=A0
>>=C2=A0 =C2=A0 =C2=A0 /* receive a burst of packet */
>>=C2=A0 =C2=A0 =C2=A0 nb_rx =3D common_fwd_stream_receive(fs, pkts_b= urst, nb_pkt_per_burst);
>> -=C2=A0 =C2=A0 if (unlikely(nb_rx =3D=3D 0))
>> +=C2=A0 =C2=A0 if (unlikely(nb_rx =3D=3D 0)) {
>> +#ifndef RTE_LIB_GRO
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return false;
>> +#else
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 gro_enable =3D gro_port= s[fs->rx_port].enable;
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Make sure that = in case of Heavyweight mode GRO the packets in
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* GRO cache shoul= d be flushed as the timer could have expired.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* The order of co= nditions should be the same as gro_ctx is valid
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* only when gro_f= lush_cycles is not the GRO_DEFAULT_FLUSH_CYCLES which
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* indicates light= weight mode GRO
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>>
>
> Updated comment as below to make it terse, what do you think:
>=C2=A0 /*
>=C2=A0 * Check if packets need to be flushed in the GRO context
>=C2=A0 * due to a timeout.
>=C2=A0 *
>=C2=A0 * Continue only in GRO heavyweight mode and if there are
>=C2=A0 * packets in the GRO context.
>=C2=A0 */
>
>
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!gro_enable || (gro= _flush_cycles =3D=3D GRO_DEFAULT_FLUSH_CYCLES) ||
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 (rte_gro_get_pkt_count(current_fwd_lcore()->gro_ctx) =3D=3D 0)) >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 return false;
>> +#endif
>> +=C2=A0 =C2=A0 }
>>
>
> Another issue but also related to your patch, if there is no packet to=
> Tx after GRO block, should we add another zero packet check:
> if (unlikely(nb_rx =3D=3D 0))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return false;
>
> To prevent executing GSO and Tx path code with zero packet, do you thi= nk
> does this make sense?
>
>

Patch looks good to me, with above comment update, but I am worried
about side impacts of this patch that we might be missing, that is why I would like it to be in -rc1, so that it can be tested better. Hence,


Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.
(Updated comment as suggested above while merging.)


Lets continue to discuss return on "nb_rx =3D=3D 0" case after GR= O block,
incremental to this patch.

I was not able to get to this. I wi= ll also take a look at the code to see if this can cause any issues.
Thanks.

--00000000000002f7eb0611f69062--