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 E8F7E46463; Mon, 24 Mar 2025 08:44:45 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D1BDC40677; Mon, 24 Mar 2025 08:44:45 +0100 (CET) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 3B2A040674 for ; Mon, 24 Mar 2025 08:44:44 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 1C380204FE; Mon, 24 Mar 2025 08:44:44 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----_=_NextPart_001_01DB9C90.9BBB4E36" Subject: RE: [PATCH] app/testpmd: fix VLAN header parsing X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Mon, 24 Mar 2025 08:44:42 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9FB42@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH] app/testpmd: fix VLAN header parsing Thread-Index: AQHbm+8qBy++gaakq0m8DcV6iO248LOAszuAgAEyu+qAAAERkA== References: <20250323122822.90407-1-rasland@nvidia.com> <98CBD80474FA8B44BF855DF32C47DC35E9FB3D@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Raslan Darawsheh" , "NBU-Contact-Thomas Monjalon (EXTERNAL)" , "Stephen Hemminger" Cc: , 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 This is a multi-part message in MIME format. ------_=_NextPart_001_01DB9C90.9BBB4E36 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Comment inline below. =20 From: Raslan Darawsheh [mailto:rasland@nvidia.com]=20 Sent: Monday, 24 March 2025 08.35 =20 >> From: Raslan Darawsheh [mailto:rasland@nvidia.com] >> Sent: Sunday, 23 March 2025 13.28 >>=20 >> Updated the `get_ethertype_by_ptype` function to correctly parse VLAN >> headers. >>=20 >> Fixes: 76730c7b9b5a ("app/testpmd: use packet type parsing API") >> Cc: haijie1@huawei.com >>=20 >>> Signed-off-by: Raslan Darawsheh >> --- >> app/test-pmd/csumonly.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >>=20 >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c >> index 5b906eaa53..302cc4cc66 100644 >> --- a/app/test-pmd/csumonly.c >> +++ b/app/test-pmd/csumonly.c >> @@ -468,6 +468,7 @@ get_ethertype_by_ptype(struct rte_ether_hdr >> *eth_hdr, uint32_t ptype) >> { >> struct rte_vlan_hdr *vlan_hdr; >> uint16_t ethertype; >> + uint32_t i =3D 0; >>=20 >> switch (ptype) { >> case RTE_PTYPE_L3_IPV4: >> @@ -486,10 +487,11 @@ get_ethertype_by_ptype(struct rte_ether_hdr >> *eth_hdr, uint32_t ptype) >> return _htons(RTE_ETHER_TYPE_IPV6); >> default: >> ethertype =3D eth_hdr->ether_type; >> - while (eth_hdr->ether_type =3D=3D = _htons(RTE_ETHER_TYPE_VLAN) >> || >> - eth_hdr->ether_type =3D=3D = _htons(RTE_ETHER_TYPE_QINQ)) { >> + while (ethertype =3D=3D _htons(RTE_ETHER_TYPE_VLAN) || >> + ethertype =3D=3D _htons(RTE_ETHER_TYPE_QINQ)) { >> vlan_hdr =3D (struct rte_vlan_hdr *) >> - ((char *)eth_hdr + sizeof(*eth_hdr)); >> + ((char *)eth_hdr + sizeof(*eth_hdr) + >> + (i * sizeof(struct rte_vlan_hdr))); >Doesn't work; "i" is not incremented, and remains 0. >Instead do this: Initialize (struct rte_vlan_hdr = *)vlan_hdr=3DRTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, ether_type)) = before the loop, and move vlan_hdr+=3DRTE_VLAN_HLEN inside the loop. Yes you are correct I had a (i++ *) but must have slipped while rebasing = will handle your way in v2=20 MB: Noticed a bug in my suggestion: Due to the type of vlan_hdr, moving = it should be vlan_hdr++, not vlan_hdr+=3DRTE_VLAN_HLEN. MB: And you might want to impose a maximum boundary to the loop, as = suggested by Stephen. E.g. break out of the loop if = vlan_hdr>RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, = ether_type)+4*sizeof(struct rte_vlan_hdr)). Supporting up to 4 stacked = VLAN tags should suffice. >> ethertype =3D vlan_hdr->eth_proto; >> } >> return ethertype; >> -- >> 2.39.5 (Apple Git-154) =20 Kindest regards Raslan Darawsheh ------_=_NextPart_001_01DB9C90.9BBB4E36 Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable

 

From: Raslan Darawsheh = [mailto:rasland@nvidia.com]
Sent: Monday, 24 March 2025 = 08.35

 

>> From: Raslan = Darawsheh [mailto:rasland@nvidia.com]
>= > Sent: Sunday, 23 March 2025 13.28
>>
>> Updated = the `get_ethertype_by_ptype` function to correctly parse = VLAN
>> headers.
>>
>> Fixes: 76730c7b9b5a = ("app/testpmd: use packet type parsing API")
>> Cc: = haijie1@huawei.com
>>
>>> Signed-off-by: Raslan = Darawsheh <rasland@nvidia.com>
>> ---
>>  = app/test-pmd/csumonly.c | 8 +++++---
>>  1 file changed, 5 = insertions(+), 3 deletions(-)
>>
>> diff --git = a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index = 5b906eaa53..302cc4cc66 100644
>> --- = a/app/test-pmd/csumonly.c
>> +++ = b/app/test-pmd/csumonly.c
>> @@ -468,6 +468,7 @@ = get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, = uint32_t ptype)
>>  = {
>>        struct = rte_vlan_hdr = *vlan_hdr;
>>        = uint16_t ethertype;
>> +     uint32_t i =3D = 0;
>>
>>        = switch (ptype) {
>>        = case RTE_PTYPE_L3_IPV4:
>> @@ -486,10 +487,11 @@ = get_ethertype_by_ptype(struct rte_ether_hdr
>> *eth_hdr, = uint32_t = ptype)
>>         &= nbsp;      return = _htons(RTE_ETHER_TYPE_IPV6);
>>     &nb= sp;  = default:
>>         = ;       ethertype =3D = eth_hdr->ether_type;
>> = -            = while (eth_hdr->ether_type =3D=3D = _htons(RTE_ETHER_TYPE_VLAN)
>> ||
>> = -            =          eth_hdr->ether_type = =3D=3D _htons(RTE_ETHER_TYPE_QINQ)) {
>> = +            = while (ethertype =3D=3D _htons(RTE_ETHER_TYPE_VLAN) ||
>> = +            =          ethertype =3D=3D = _htons(RTE_ETHER_TYPE_QINQ)) = {
>>          =             &= nbsp; vlan_hdr =3D (struct rte_vlan_hdr *)
>> = -            =             &= nbsp;    ((char *)eth_hdr + = sizeof(*eth_hdr));
>> = +            =             &= nbsp;    ((char *)eth_hdr + sizeof(*eth_hdr) = +
>> = +            =             &= nbsp;    (i * sizeof(struct = rte_vlan_hdr)));

>Doesn't work; "i" is not = incremented, and remains 0.
>Instead do this: Initialize (struct = rte_vlan_hdr *)vlan_hdr=3DRTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, = ether_type)) before the loop, and move vlan_hdr+=3DRTE_VLAN_HLEN inside = the loop.
Yes you are correct I had a (i++ *) but must have slipped = while rebasing will handle your way in v2

MB: Noticed a bug in my suggestion: Due to the type of vlan_hdr, = moving it should be vlan_hdr++, not = vlan_hdr+=3DRTE_VLAN_HLEN.

MB: And you might want to impose a maximum boundary to the loop, as = suggested by Stephen. E.g. break out of the loop if = vlan_hdr>RTE_PTR_ADD(eth_hdr, offsetof(rte_ether_hdr, = ether_type)+4*sizeof(struct rte_vlan_hdr)). Supporting up to 4 stacked = VLAN tags should suffice.


>>     &nbs= p;            = ;      ethertype =3D = vlan_hdr->eth_proto;
>>      &n= bsp;         = }
>>          =       return ethertype;
>> = --
>> 2.39.5 (Apple Git-154)

 

Kindest regards

Raslan = Darawsheh

------_=_NextPart_001_01DB9C90.9BBB4E36--