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 7266A43866; Mon, 8 Jan 2024 17:12:12 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EA64440273; Mon, 8 Jan 2024 17:12:11 +0100 (CET) Received: from mail-yb1-f169.google.com (mail-yb1-f169.google.com [209.85.219.169]) by mails.dpdk.org (Postfix) with ESMTP id 2B90540263 for ; Mon, 8 Jan 2024 17:12:10 +0100 (CET) Received: by mail-yb1-f169.google.com with SMTP id 3f1490d57ef6-dbe69afb431so1276270276.1 for ; Mon, 08 Jan 2024 08:12:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1704730329; x=1705335129; 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=WgC15dphlAh5vdFvRQuDSqy2v9fUhqwxVS+LCH1KeqA=; b=M4mSjZxu2tFJo36+ijKfRzh/NZf7mfoOwt1NU7EYv3aQqEsA2QAvmBcRdpfIci750q CeSkrAotnM64/whBT/1spXq4PFAFhY0vukxFpClq0CwfLNZ26Gt9MeODRZNYBohUaXMj synkKfaM1M769dzwZXgNnVz+oRsTiMJQ7Vwq1xqkmk6MwCuW/e3YG9pCiQNEeSTRqnuQ Fyyx+/FqB5pDT5Io9n3idT8xzkDXNcOqgxqowZrPpRQQZRQSGK4jpFrScWrj6olC5fV2 iXqkdaYGPPacdT6wTepHLqk/xs3Cwy9stAkcxsqe0WJSOrlFwGD4Pb9SpAkD2c/8izdH a0JA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704730329; x=1705335129; 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=WgC15dphlAh5vdFvRQuDSqy2v9fUhqwxVS+LCH1KeqA=; b=SgJfyF6O8yTQZJkoU7Lf+a9C1pI9P2jyeAOs8lL5PRb2xjOMAiUzQegZYmtlk4MRT3 3rpJJXLre2Hx77uIx0gTPKKD2Q+67p1E8U7s9Km9ZuqBPHn8et68xgMJxKRDTTTX/CaT hnE3jIRKXmhZtZ9gS7I9uEz+TmLS6eLFRxip7aYoe3Gl6cJstABAaT6aufq7FyWm6lTm mHcngsQQNF1p1qc0MR31ybe1zyLwEtm2bT1Vx7B2F087Rt6Mj77fmW/IpeaG6m+85lyP +sjswWK050FQr9JrQ/az9gvUeKODBrzmh08p1lGNcNbu/mSkZ6Qg+c6+LOaxb+QfOSLB +zIQ== X-Gm-Message-State: AOJu0Yxk2j9AkYEOkkK687W9YHizsCASXCiGpFub2qs41/d1SlRy85pn WgZphwf4tjlc5N+sCRs/NihMYHzuC+/0fBUS76A= X-Google-Smtp-Source: AGHT+IFgI5v20A/GVYnaS9NDzy2015CdzWC4uU197JpalT4lF4chUGANpKymJ3nzspF4IZ1kNLTzy5xWaUVaogmjyis= X-Received: by 2002:a05:6902:105:b0:db5:4764:acad with SMTP id o5-20020a056902010500b00db54764acadmr1335201ybh.9.1704730329309; Mon, 08 Jan 2024 08:12:09 -0800 (PST) MIME-Version: 1.0 References: <20231208181738.23931-1-kumaraparamesh92@gmail.com> <20240107112920.521184-1-kumaraparamesh92@gmail.com> <20240107092020.371d466d@hermes.local> In-Reply-To: <20240107092020.371d466d@hermes.local> From: kumaraparameshwaran rathinavel Date: Mon, 8 Jan 2024 21:41:58 +0530 Message-ID: Subject: Re: [PATCH v11] gro: fix reordering of packets in GRO layer To: Stephen Hemminger Cc: hujiayu.hu@foxmail.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000b76f94060e717598" 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 --000000000000b76f94060e717598 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sun, Jan 7, 2024 at 10:50=E2=80=AFPM Stephen Hemminger < stephen@networkplumber.org> wrote: > On Sun, 7 Jan 2024 16:59:20 +0530 > Kumara Parameshwaran wrote: > > > + /* Return early if the TCP flags are not handled in GRO layer */ > > + if (tcp_hdr->tcp_flags & (~(VALID_GRO_TCP_FLAGS))) > > Nit, lots of extra paren here. Could be: > if (tcp_hdr->tcp_flags & ~VALID_GRO_TCP_FLAGS) > >> Done. >> > > + if (find =3D=3D 1) { > > + /* > > + * Any packet with additional flags like PSH,FIN should b= e > processed > > + * and flushed immediately. > > + * Hence marking the start time to 0, so that the packets > will be flushed > > + * immediately in timer mode. > > + */ > > + if (tcp_hdr->tcp_flags & (RTE_TCP_ACK_FLAG | > RTE_TCP_PSH_FLAG | RTE_TCP_FIN_FLAG)) { > > + if (tcp_hdr->tcp_flags !=3D RTE_TCP_ACK_FLAG) > > + tbl->items[item_start_idx].start_time =3D= 0; > > + return process_tcp_item(pkt, tcp_hdr, tcp_dl, > tbl->items, > > + tbl->flows[i].start_index= , > > + &tbl->item_num, > tbl->max_item_num, > > + ip_id, is_atomic, > start_time); > > + } else { > > + return -1; > > + } > > + } > > Reordering this conditional would keep code from being so indented. > >> Doing this reordering as suggested by Jiyau since the find =3D=3D 1 woul= d be >> likely in most cases. >> > > - delete_tcp_item(tbl->items, item_idx, > &tbl->item_num, INVALID_ARRAY_INDEX); > > + delete_tcp_item(tbl->items, item_idx, > &tbl->item_num, > > + INVALID_ARRAY_INDEX); > > return -1; > > This change is unnecessary, max line length in DPDK is 100 characters for > readability. > >> Done. >> > > return 0; > > + } else { > > + return -1; > > } > > > > - return process_tcp_item(pkt, tcp_hdr, tcp_dl, tbl->items, > tbl->flows[i].start_index, > > - &tbl->item_num, > tbl->max_item_num, > > - ip_id, is_atomic, > start_time); > > + return -1; > > } > > Since end of else and end of function both return -1, the else clause is > unnecessary. > >> Done. >> > --000000000000b76f94060e717598 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Sun, Jan 7, 2024 at 10:50=E2=80=AF= PM Stephen Hemminger <step= hen@networkplumber.org> wrote:
On Sun,=C2=A0 7 Jan 2024 16:59:20 +0530
Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:

> +=C2=A0 =C2=A0 =C2=A0/* Return early if the TCP flags are not handled = in GRO layer */
> +=C2=A0 =C2=A0 =C2=A0if (tcp_hdr->tcp_flags & (~(VALID_GRO_TCP_= FLAGS)))

Nit, lots of extra paren here. Could be:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (tcp_hdr->tcp_flags & ~VALID_GRO_TCP_= FLAGS)
Done.
> +=C2=A0 =C2=A0 =C2=A0if (find =3D=3D 1) {
> +=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 * Any packet with ad= ditional flags like PSH,FIN should be processed
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * and flushed immedi= ately.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * Hence marking the = start time to 0, so that the packets will be flushed
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * immediately in tim= er mode.
> +=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=A0if (tcp_hdr->tcp_f= lags & (RTE_TCP_ACK_FLAG | RTE_TCP_PSH_FLAG | RTE_TCP_FIN_FLAG)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (tcp_hdr->tcp_flags !=3D RTE_TCP_ACK_FLAG)
> +=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 =C2=A0tbl->items[item_start_idx].start_time= =3D 0;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return process_tcp_item(pkt, tcp_hdr, tcp_dl, tbl->items,
> +=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 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0tbl->flows[i].start_index,
> +=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 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0&tbl->item_num, tbl->max_item_num,
> +=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 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0ip_id, is_atomic, start_time);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return -1;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0}

Reordering this conditional would keep code from being so indented.
Doing this reordering as sugg= ested by Jiyau since the find =3D=3D 1 would be likely in most cases.
<= /blockquote> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0delete_tcp_item(tbl->items, item_idx, &tbl->item_num, INVA= LID_ARRAY_INDEX);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0delete_tcp_item(tbl->items, item_idx, &tbl->item_num,
> +=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 =C2=A0INVALID_ARRAY_INDEX);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0return -1;

This change is unnecessary, max line length in DPDK is 100 characters for r= eadability.
Done.
=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
> +=C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0return process_tcp_item(pkt, tcp_hdr, tcp_dl, tbl= ->items, tbl->flows[i].start_index,
> -=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 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0&tbl->item_num, tbl->max_item_num,
> -=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 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0ip_id, is_atomic, start_time);
> +=C2=A0 =C2=A0 =C2=A0return -1;
>=C2=A0 }

Since end of else and end of function both return -1, the else clause is un= necessary.
Done.
<= /blockquote>
--000000000000b76f94060e717598--