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 064674566C; Mon, 22 Jul 2024 06:28:23 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4469F427C8; Mon, 22 Jul 2024 06:28:22 +0200 (CEST) Received: from AM0PR83CU005.outbound.protection.outlook.com (mail-westeuropeazon11010039.outbound.protection.outlook.com [52.101.69.39]) by mails.dpdk.org (Postfix) with ESMTP id AA831402D9; Mon, 22 Jul 2024 06:27:43 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=xhWWb2UG6PC3n1a3BpMjSR9whw6qXD8esvApLB3TVVZ1481bFxdrix23B/EOs1VHNxxCm7g83XV2+IhcuYCoKrcuL9WPDKrDZemWibI1Zvk4z4RSgMbk3OjOd4JKAdeM2awXiMVwezMh8rCwPMtyyxoGYkyMqElc1EVohCyk7pz8zpz6WSs1gtKAZS2lK5MpuJQoSwlay7KacZONNNzdH7PjB3jXUivifNUgX5Tn4qNxOyXzQ7kMK2ItY1mNHEop9FXb24RnjG+STe8e1u5n+0oTr7O4CC44/Ej38yWjJf0jH30ghyhEr5ANi5gOOnny6HQVPIHD+A3iz0jLMgT70g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=yZ2rWcB9H6MmVAJ41beNPuBkvRaSuRmzFLCII/f7tiw=; b=RwToTg9AWYTF5Pk1bghqT06O8S2FmjXD1g2xx81mba7Pzp73WFS2FXCjmGaXZJW+UP6RTsbFqiO1k5RnVyMlp9Zxy4/mVQqF6mT8oDeZ+pAW1NjFoN1ZNMckGP6vcSeVDnmT8GMUBZVP2fAkYCAbbUxC4nlPZnjtCdsUi+L9uHwnW0/nFADNr64X7HluDLqLsMp0RNO3SN8UBS6UXJANpqZlZpxN6EXGsCfveYPx96PRRsOdkvAVzcZKqSazrYmAHWQos+Qa4/+1RF8ZrMFqGQ8DrYMe7K2le9DOm/QQXf4ODIOCXJlwjuVrA6pQTNIzNI1WFCkNjvtuCzO45AwX+g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com; dkim=pass header.d=nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=yZ2rWcB9H6MmVAJ41beNPuBkvRaSuRmzFLCII/f7tiw=; b=KcmyRBIk1mhRRC9aJlegDSKGONq787wwLDtpOzkzCtC3xIlkgs2tKIdXpuRachvYTTMWW0EQmvzZ4dKbwbKwRVE8rHl9eYKBz3XmOa8gxHA5oNK7yi3/8M3GR7wHMaANVWEUWDcvtLhaboG6ho1EpLYI0Y7/OxJE+QZar9DrvcQ= Received: from AS8SPR01MB0024.eurprd04.prod.outlook.com (2603:10a6:20b:3d0::24) by DU2PR04MB9083.eurprd04.prod.outlook.com (2603:10a6:10:2f2::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7784.20; Mon, 22 Jul 2024 04:27:42 +0000 Received: from AS8SPR01MB0024.eurprd04.prod.outlook.com ([fe80::c634:479e:8f8a:a325]) by AS8SPR01MB0024.eurprd04.prod.outlook.com ([fe80::c634:479e:8f8a:a325%7]) with mapi id 15.20.7784.017; Mon, 22 Jul 2024 04:27:42 +0000 From: Gagandeep Singh To: Konstantin Ananyev , "dev@dpdk.org" , Konstantin Ananyev , Sean Morrissey CC: "stable@dpdk.org" Subject: RE: [PATCH 3/3] examples/l3fwd: fix maximum acceptable port ID in routes Thread-Topic: [PATCH 3/3] examples/l3fwd: fix maximum acceptable port ID in routes Thread-Index: AQHa1p/wVRBmr3j9f0St0JrbTClx37H6t9YAgAFKzOCAAEMlgIAF2LrQgAARmwA= Date: Mon, 22 Jul 2024 04:27:41 +0000 Message-ID: References: <20240715101458.645014-1-g.singh@nxp.com> <20240715101458.645014-3-g.singh@nxp.com> <370cf325ad02427cbca5a37756da4c35@huawei.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=nxp.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: AS8SPR01MB0024:EE_|DU2PR04MB9083:EE_ x-ms-office365-filtering-correlation-id: 9ef53eb2-348a-49ca-9f40-08dcaa06a08d x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; ARA:13230040|366016|376014|1800799024|38070700018; x-microsoft-antispam-message-info: =?us-ascii?Q?RyB7M6FbOPmBm4NL+3+o91+r6HJkTuGxCdgCYuiMSO88e3BxMGu3B5bZsDci?= =?us-ascii?Q?4TS5zfMCUSBk1vL1wPRx1mksdmfF+0y8fbJLqftMESZ1TehaIjPcMlA243FV?= =?us-ascii?Q?NynbeXqBz7KffMFy+v7enFtmkRb0GSNI1UZdvPT24LciG6lzbGFfPiFM95Kv?= =?us-ascii?Q?1qScitKEwlh7Yvz2nTjRlA1gABC+jN4kKTxEJPkwDzsY0z9k5kfRLbgp77eA?= =?us-ascii?Q?e0i40r70KbmK8PxWrM8Y92K4R4h5iiEeWPRrbX48IzpfMRx0F96jmrSC7Gvx?= =?us-ascii?Q?dpEj6LmbAj+Hajx4jW5343DyvuNa+RN07s8QXAe3KAL+EfV3yyzhDZUmse5Q?= =?us-ascii?Q?GMaz5il4yvwXjmx2vq6YoK77JxXOxYMYJ9tP0oKOSB0e/dP5dbx4S2Al1iQl?= =?us-ascii?Q?jM1cRpd4hkD3dr5wISAnja4tf/pMBoraD1FeUpqkbUZNkYjJpJjDv4mqORIZ?= =?us-ascii?Q?+dTJB5+ZuRn8hDqTs6fYreCrX6BTRRijF1GNmVKdrK4JaZlrChDSIOTiMITN?= =?us-ascii?Q?aaw94HDmbSjXReHmZaRVdeo9zg/SAgenH6WxS19r8NaMzMmuQ/eUcR3tn88E?= =?us-ascii?Q?WiKEeg9WUKsKPjrI0G8BeigpUwh/nhyGhk6ecTcGo4Xq3bTiqk9/XNDOjiDd?= =?us-ascii?Q?YPD/HDTEjCwC5bibhrwvk0H6+lfCZhD6RnhBybUoYuSvRJWG/kxkFOzWKyqu?= =?us-ascii?Q?mnWbnRRmzHcKqZO3loI4DQUP7n0STnV98+lk+bqQJ60vg6JphtHdCfv5SbsG?= =?us-ascii?Q?IoRryeCcojz2Dj17uoVkopPKJgYir17oNtjQnrE7bBMf3e635F7+aX2jJdkj?= =?us-ascii?Q?VvNtJ+MXcg+kmKmEL90DO4sq0licPlHax6dolmDzgeBe6x3ECPANN1ANhQPR?= =?us-ascii?Q?Z+QUJrAAIEaW00gThbxQm0QxfakJ1729Wcya70ZWNXpol6n3iSYnWzLhvJMq?= =?us-ascii?Q?+OVUqiIFyoFiOd18b41XzygGLGHmkapwiszYtwC3asyxDxUjXzrKRlMC7oLu?= =?us-ascii?Q?9dw4fSDjeHgaX25iWM/qxeK0Dzy19M8SwSYuu7a+YrW/H9XcozFW9EMQy/or?= =?us-ascii?Q?Ic9d/aLvOEj2oeGApKe3aigoBbtf0eiqKyq8H/iBaOHrqcVfaEVLFdvNnu+w?= =?us-ascii?Q?xx4nlLzayJdHf/dsz8fZsGCTj0LiHvpEwZd5FcAf6EOeKzdzsFisZMHMyHL0?= =?us-ascii?Q?xizp3MtV04J+W+iBXucWWVw+8TdbjttxrwnwPBxKut51Nde5++RJ4WyWYBKe?= =?us-ascii?Q?scuBWehzFsBARVqb7HtQPOmAE4mXV/Dlsma6+j/BbKO4Z3VgBZQskTPu8vhV?= =?us-ascii?Q?b88w/TFi9apJxXhHppDfm+NLW4toLs25kMqpBHCAcQajAYDUMbh+9fm/MHn/?= =?us-ascii?Q?LQ7sSvxvwZCEv7QSx/HcS5ACyAFGhkrAUZsIEjNnMY/53qvReA=3D=3D?= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:AS8SPR01MB0024.eurprd04.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230040)(366016)(376014)(1800799024)(38070700018); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?5IsqMisXaf0ZecZ6z4AlpRB4roQ4kj6pj2MLByV0ebVlJaMmu1RxEQ98zFwL?= =?us-ascii?Q?q8hombfDimE1q1wdi4xJm8/qlesas7mjP0SdKExLJUr/xNkHbNviB84dEzqh?= =?us-ascii?Q?737HOjHRafvEnq2mBzBkOu+Zmu2URP3q5m0VT83BA5JrEaOl7Yr6RVk4p7Tc?= =?us-ascii?Q?GeHlDN0bSXPblyBRyjM3QC6D0yVFFKU9twCIbXRxhKZt1CTYoXvEFMFJpDl0?= =?us-ascii?Q?20lJ8q5HbDMffpze5yP8V/1ttz6Aaqurc0ez94y2bBwYKbmWfDdX/NMFYqS6?= =?us-ascii?Q?DbTZImQYV0dahZPioFCScLSxAVQmZGMgaVlvoSHJ2LfVPU4c1MSZ/nHbqlND?= =?us-ascii?Q?Ewkj3JKskV0ZV/Fnwd1CS720UhLyrmukCkDFpp7WxTuxjHnbcfi0XB59kNzr?= =?us-ascii?Q?bKUAsXQXegNRCNuWfYuqBNVHbCwqYrqj/Q12CnfqgEte4VeS8t7Vwq9HqiCi?= =?us-ascii?Q?mr78/ue69ZHfQXcbxmA2x7YnFv5lD7kFY4+j5QFpbMnciFZeqGr5A4rWEdxe?= =?us-ascii?Q?yz6CqczPKBL8kOVOxALmgYyCx4NAhobHFTSPyG9sMM3rlKJrFkh5SufU+EwJ?= =?us-ascii?Q?jaUgpMeVyL0gFuu5BBbgkIFKxe1xorNdV0mJvRs7aRATmwj4bmUyeFvt1bks?= =?us-ascii?Q?GVK9hyubVSFwAiTwKRLgliquMkEQ069ddy4MRKzRab/w5r+2U9p17kOmNr+I?= =?us-ascii?Q?zrivuhL45UqfLefgcg/oaX1aSDlAxK1H9dvyDhcesN8FbW6wniXl8nROx/AK?= =?us-ascii?Q?KrQcZ8R8cFYTolowaglptE03oWY3HgCpX+helv9UxTOu1N8/47iAtRk8cen/?= =?us-ascii?Q?zUSjeS+Rq9RVb6w/81O84eD89TpHLBLlRiWi70nRFhy3lR4DU6y3NaRMJHdM?= =?us-ascii?Q?5xfLbWdaIohFwrwqaKGRmsq+a/gVgKCPHHM68LcIPpQ+oJU5P4pDr+W6XF8B?= =?us-ascii?Q?XdXO5eva6LjaLp/wL0eKYc6scAVOQuAKy0deikK6nadZ7WUKpAefLnXlpTeX?= =?us-ascii?Q?xHTzzLrknLE3NmW7KK+WrieZ0Hq6A8y+U9K7m72Iuy5TXaWvTKK/RkHbeQSb?= =?us-ascii?Q?l90cP9unrxYOHpJv4J/MI8GsVBzTh6ReFi4tccr9PGOzg/0qEml54UzEfVdC?= =?us-ascii?Q?QqWWEomIDPkz2LW6cmtEit9L2i/PW7d0ogKcO/d1IDolReG/GdfesxS4fJqm?= =?us-ascii?Q?IkA0acsng1tEOSjLrlMDqSCLhntCUf9SG7XoE0yn/XgBg5GQOPtiIYGHf/Qi?= =?us-ascii?Q?FooluBsxoOK3uw8VbDRHFRgBjGr3KvbwRruxlDAaSPClNm3Z37X/JQDCJUVV?= =?us-ascii?Q?yxrwdD3RzY3uo3KCaRJ7nVaJ5rLVdQFUaBEK4Y8PetPZdR5xj3MhiMPehYem?= =?us-ascii?Q?KQQTsw/wMisx/T+iMqLUk0kQroNCduicn8t9xWoIaTPsFheX9dxzmcG27Kpk?= =?us-ascii?Q?W6wQoCGmsMtq8R24ttg/S7mNoBNE4UJ0WBhuxXugy/B1aw96+/gCBfipzvff?= =?us-ascii?Q?G4BlqltWIteFMPXe2ucx3lWMggpdiy1fm7QY9kgosP+WTGVVqG/xZQg8ZuWm?= =?us-ascii?Q?50uniMybJVC1T1Omqs0=3D?= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: AS8SPR01MB0024.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 9ef53eb2-348a-49ca-9f40-08dcaa06a08d X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Jul 2024 04:27:41.9913 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: iKgZvGNyNdhozq6zdnFZFrVj3Aq759IsPALBbMoNsK3nXqI2q+3c60Qpv+3+IEIR X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB9083 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 Hi, > > > > > Application is accepting routes for port ID up to UINT8_MAX for > > > > > LPM amd EM routes on parsing the given rule file, but only up to > > > > > 32 ports can be enabled as per the variable enabled_port_mask > > > > > which is defined as uint32_t. > > > > > > > > > > This patch restricts the rules parsing code to accept routes for > > > > > port ID up to 31 only to avoid any unnecessary maintenance of > > > > > rules which will never be used. > > > > > > > > If we want to add this extra check, probably better to do it in set= up_lpm(). > > > > Where we already check that port is enabled, and If not, then this > > > > route rule will be skipped: > > > > > > > > /* populate the LPM table */ > > > > for (i =3D 0; i < route_num_v4; i++) { > > > > struct in_addr in; > > > > > > > > /* skip unused ports */ > > > > if ((1 << route_base_v4[i].if_out & > > > > enabled_port_mask) =3D=3D 0) > > > > continue; > > > > > > > > Same for EM. > > > I am trying to update the check for MAX if_out value in rules config > > > file parsing > > which will be before setup_lpm(). > > > The reason is, restricting and adding only those rules which can be > > > used by the application while populating the route_base_v4/v6 at > > > first step and avoid unnecessary memory allocation for local > > > variables to store more > > not required rules. > > > > Hmm... but why it is a problem? > Not really a problem, Just trying to optimize wherever it Is possible. >=20 > > > > > > > > > ((1 << route_base_v4[i].if_out & > > > > enabled_port_mask) > > > By looking into this check, it seems restriction to maximum 31 port > > > ID while parsing rule file becomes more valid as this check can pass > > > due to overflow in case value of route_base_v4[i].if_out Is 31+. > > > > Agree, I think we need both, and it probably need to be in setup_lpm(). > > Something like: > > > > if (route_base_v4[i].if_out >=3D sizeof(enabled_port_mask) * CHAR_BIT |= | > > ((1 << route_base_v4[i].if_out & enabled_port_mask) =3D=3D 0) { > > /* print some error message here*/ > > rte_exiit(...); /* or return an error */ } > > > Yes, I can change it to this. I re-checked the code, IMO we should restrict the rules in " read_config_fi= les" May be we can move this check to read_config_files. As having this check in the setup can result in rte_exit() call when no use= r rule file Is given and application is using the default rules. In that case route_bas= e_v4 will Have 16 rules for 16 ports (default rules).=20 So this check will fails always unless user enable all the 16 ports with -p= option. >=20 > > > > > > > Another question here - why we just silently skip the rule with inv= alid port? > > > In read_config_files_lpm() we are calling the rte_exit in case port I= D is 31+. > > > In setup_lpm, skipping the rules for the ports which are not enabled > > > and not giving error, I guess probably because of ease of use. > > > e.g. user has only single ipv4_routes config file with route rules > > > for port ID 0,1,2,3,4 and want to use same file for multiple test > > > cases like 1. when only port 0 enabled 2. when only port 0 and 1 > > > enabled and so on. > > > In this case, user can avoid to have separate route files for each of= the test > case. > > > > The problem as I see it - we are not consistent here. > > In some cases we just silently skip rules with invalid (or disabled) > > port numbers, in other cases we generate an error and exit. > > For me it would be better, if we follow one simple policy (abort with > > error) here for all cases. > Ok, I will add the rte_exit if route port is invalid or not enabled. > With this change onwards It will be assumed user will add only those rout= es With > port IDs which are valid and enabled in the application. >=20 > > > > > > > > > Probably need to fail with error... that what ACL code-path does. > > > > > > > > > Fixes: e7e6dd643092 ("examples/l3fwd: support config file for > > > > > EM") > > > > > Fixes: 52def963fc1c ("examples/l3fwd: support config file for > > > > > LPM/FIB") > > > > > Cc: sean.morrissey@intel.com > > > > > Cc: stable@dpdk.org > > > > > > > > > > Signed-off-by: Gagandeep Singh > > > > > --- > > > > > examples/l3fwd/em_route_parse.c | 6 ++++-- > > > > > examples/l3fwd/lpm_route_parse.c | 6 ++++-- > > > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/examples/l3fwd/em_route_parse.c > > > > > b/examples/l3fwd/em_route_parse.c index 8b534de5f1..65c71cd1ba > > > > > 100644 > > > > > --- a/examples/l3fwd/em_route_parse.c > > > > > +++ b/examples/l3fwd/em_route_parse.c > > > > > @@ -65,7 +65,8 @@ em_parse_v6_rule(char *str, struct em_rule *v) > > > > > /* protocol. */ > > > > > GET_CB_FIELD(in[CB_FLD_PROTO], v->v6_key.proto, 0, UINT8_MAX, 0= ); > > > > > /* out interface. */ > > > > > - GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0); > > > > > + GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, > > > > > + (sizeof(enabled_port_mask) * CHAR_BIT) - 1, 0); > > > > > > > > > > return 0; > > > > > } > > > > > @@ -102,7 +103,8 @@ em_parse_v4_rule(char *str, struct em_rule *v= ) > > > > > /* protocol. */ > > > > > GET_CB_FIELD(in[CB_FLD_PROTO], v->v4_key.proto, 0, UINT8_MAX, 0= ); > > > > > /* out interface. */ > > > > > - GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0); > > > > > + GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, > > > > > + (sizeof(enabled_port_mask) * CHAR_BIT) - 1, 0); > > > > > > > > > > return 0; > > > > > } > > > > > diff --git a/examples/l3fwd/lpm_route_parse.c > > > > > b/examples/l3fwd/lpm_route_parse.c > > > > > index f27b66e838..357c12d9fe 100644 > > > > > --- a/examples/l3fwd/lpm_route_parse.c > > > > > +++ b/examples/l3fwd/lpm_route_parse.c > > > > > @@ -110,7 +110,8 @@ lpm_parse_v6_rule(char *str, struct > > > > > lpm_route_rule > > > > > *v) > > > > > > > > > > rc =3D lpm_parse_v6_net(in[CB_FLD_DST_ADDR], v->ip_32, > > > > > &v->depth); > > > > > > > > > > - GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0); > > > > > + GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, > > > > > + (sizeof(enabled_port_mask) * CHAR_BIT) - 1, 0); > > > > > > > > > > return rc; > > > > > } > > > > > @@ -132,7 +133,8 @@ lpm_parse_v4_rule(char *str, struct > > > > > lpm_route_rule > > > > > *v) > > > > > > > > > > rc =3D parse_ipv4_addr_mask(in[CB_FLD_DST_ADDR], &v->ip, > > > > > &v->depth); > > > > > > > > > > - GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0); > > > > > + GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, > > > > > + (sizeof(enabled_port_mask) * CHAR_BIT) - 1, 0); > > > > > > > > > > return rc; > > > > > } > > > > > -- > > > > > 2.25.1 > > > > > > Gagan