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 F0F6F44143; Mon, 3 Jun 2024 04:33:11 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8130E402B7; Mon, 3 Jun 2024 04:33:11 +0200 (CEST) Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) by mails.dpdk.org (Postfix) with ESMTP id D52624029C for ; Mon, 3 Jun 2024 04:33:09 +0200 (CEST) Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-1f44b5d0c50so30380355ad.2 for ; Sun, 02 Jun 2024 19:33:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1717381989; x=1717986789; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=XJySzOmuRRZfKcGqPoEKjvo/HizhAseuwd4kxgmNZpE=; b=uYvdnsG8ljOImz2CxtyKQnYNEqEo2JCWKZByWkN9XsnjHd6mPVlNFaZSadC3rSqg9Y PzRoxWf9rITKnBGOu7oZk3uzTVUBu3JaZMqNfNcwKgGFWcrPOSsXK+JJW34Xu0N0B3BN zgjm6OuEnsbzS6fQeXj9iX5J9aFIpO2TJsgyeWd19HWmAGhrfdN+Yg1gp/44uvzWoqhQ mb786eriJwSUvsYClNYMGSCjfuWj2Lmcv8VzPWhO3HpkbJTMl1rZb88sV9OGw/DX6Boe ZzwTRgIoU1Ey1OR4BDj77M2BhKf4OqLS8ryUPYGsjlmiKV3ncVzkhVIK7a4isxz1Pulp yOSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717381989; x=1717986789; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=XJySzOmuRRZfKcGqPoEKjvo/HizhAseuwd4kxgmNZpE=; b=lqCO6GH+ThMJNZhIPKAGWbGkyIs8ZYfr02sDSsPeeCQsUAbOsP4HC802gl7bN+knPc 8+2/Mkfjmbeybz3HP/mA8vL+x+5mIWIFP6nCEWtO4L7JndeQ9KNwtelpUJTCZtj+nU4U R9Mp/StATEtSg+Z24wJkZF5uaQGTew1x0weT7uX125+OAU2prscm8hrTa6GYwbTUggJH XH0llD+Yqzci9VLKZkmefwsXbsL/wvDZ2B+zOnK/wsIaD/Y79SpPcGNlaT3/5bHKE+ji 8ApSpmb6wdb4swZEqTfCVdT9hP6qQKhDNH7i+rKN1nITb5gbsae6s/JXKstYIY7XtQlX PXiQ== X-Forwarded-Encrypted: i=1; AJvYcCXpc95hIqts6ehZ3kK+pqRP7a1JTNgVhQoJ/iusbAGb0NNoiZlTQFtFPTz9z9rJHoOIoJcYfIK+G9UZU1c= X-Gm-Message-State: AOJu0YwYiKWChjhKIJMrK8/Mv/4fLC+ACdWqd9LM+7JazEx6oLvPGGhr IWWjuVGl8sJE7glwMfG64haHY1hiOxSMvW7DdbC94JV2Pk6PmO7UHrKOVDRBIoQ= X-Google-Smtp-Source: AGHT+IHVU0yUzMO9SOzn/JOe9Dm4DxmQws016djANlEKZHujNrgKLRWrLr2pFltUjFVxQC2d0oRgug== X-Received: by 2002:a17:902:e84e:b0:1f6:6ad1:fdf9 with SMTP id d9443c01a7336-1f66ad21fb6mr32493975ad.57.1717381988586; Sun, 02 Jun 2024 19:33:08 -0700 (PDT) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-1f63241d4d9sm53070535ad.296.2024.06.02.19.33.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 02 Jun 2024 19:33:08 -0700 (PDT) Date: Sun, 2 Jun 2024 19:33:04 -0700 From: Stephen Hemminger To: Jie Hai Cc: Ferruh Yigit , huangdengdui , , "John W. Linville" , Ciara Loftus , Shepard Siegel , Ed Czeck , John Miller , Igor Russkikh , Steven Webster , Matt Peters , Selwin Sebastian , Julien Aube , Ajit Khaparde , Somnath Kotur , Chas Williams , "Min Hu (Connor)" , Nithin Dabilpuram , Kiran Kumar K , Sunil Kumar Kori , Satha Rao , Harman Kalra , Yuying Zhang , Rahul Lakkireddy , Hemant Agrawal , Sachin Saxena , Shai Brandes , Evgeny Schemeilin , Ron Beider , Amit Bernstein , Wajeeh Atrash , Gagandeep Singh , Apeksha Gupta , John Daley , Hyong Youb Kim , Gaetan Rivet , Jeroen de Borst , Rushil Gupta , Joshua Washington , Ziyang Xuan , Xiaoyun Wang , Guoyang Zhou , Yisen Zhuang , Jingjing Wu , Andrew Boyer , Rosen Xu , Long Li , Jakub Grajciar , Matan Azrad , Viacheslav Ovsiienko , Dariusz Sosnowski , Ori Kam , Suanming Mou , Zyta Szpak , Liron Himi , Martin Spinler , Chaoyong He , Jiawen Wu , Tetsuya Mukawa , Vamsi Attunuru , Devendra Singh Rawat , Alok Prasad , Bruce Richardson , Andrew Rybchenko , Cristian Dumitrescu , Jerin Jacob , Maciej Czekaj , Jian Wang , Maxime Coquelin , Chenbo Xia , Jochen Behrens , , , Subject: Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled Message-ID: <20240602193304.75716520@hermes.local> In-Reply-To: References: <20240206011030.2007689-1-haijie1@huawei.com> <11b8feac-4a9e-4d2c-8995-ed492d684750@amd.com> <7438563e-c7b4-6e13-68bf-74ff423546af@huawei.com> <6246e1f8-dcd4-468d-a05d-2e292f6e1714@amd.com> <6c831a66-f916-48d4-68d9-4c3bcfcb4979@huawei.com> <1ec105cf-c8d2-4010-867d-30970c25a2a1@amd.com> <7e376ab0-67a0-4a3c-a528-7928077e7b56@huawei.com> <760c70e6-ca2d-4d5e-9a05-809b81d32dd3@huawei.com> <541e9d04-bed3-47b0-8b11-746f9047e1a0@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 On Mon, 3 Jun 2024 09:38:19 +0800 Jie Hai wrote: > On 2024/3/1 19:10, Ferruh Yigit wrote: > > On 3/1/2024 6:55 AM, huangdengdui wrote: =20 > >> > >> > >> On 2024/2/29 17:25, Ferruh Yigit wrote: =20 > >>> On 2/29/2024 3:58 AM, huangdengdui wrote: =20 > >>>> > >>>> > >>>> On 2024/2/28 21:07, Ferruh Yigit wrote: =20 > >>>>> On 2/28/2024 2:27 AM, huangdengdui wrote: =20 > >>>>>> > >>>>>> > >>>>>> On 2024/2/27 0:43, Ferruh Yigit wrote: =20 > >>>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote: =20 > >>>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote: =20 > >>>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote: =20 > >>>>>>>>>> Hi, Ferruh, > >>>>>>>>>> > >>>>>>>>>> Thanks for your review. > >>>>>>>>>> > >>>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote: =20 > >>>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote: =20 > >>>>>>>>>>>> From: Dengdui Huang > >>>>>>>>>>>> > >>>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be trunc= ated and > >>>>>>>>>>>> the CRC is still be stripped in following cases: > >>>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length > >>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 is less than or equal to 60B. > >>>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length > >>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0 is less than or equal to 60B. > >>>>>>>>>>>> =20 > >>>>>>>>>>> > >>>>>>>>>>> If a device doesn't support the offload by some packets, it c= an be > >>>>>>>>>>> option to disable offload for that device, instead of calcula= ting it in > >>>>>>>>>>> software and append it. =20 > >>>>>>>>>> > >>>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific pa= cket > >>>>>>>>>> type and small packet(<60B) case. > >>>>>>>>>> What's more, the small ethernet packet is not common. > >>>>>>>>>> =20 > >>>>>>>>>>> Unless you have a specific usecase, or requirement to support= the > >>>>>>>>>>> offload. =20 > >>>>>>>>>> > >>>>>>>>>> Yes, some users of hns3 are already using this feature. > >>>>>>>>>> So we cannot drop this offload > >>>>>>>>>> =20 > >>>>>>>>>>> <...> > >>>>>>>>>>> =20 > >>>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue, > >>>>>>>>>>>> =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 goto pkt_err; > >>>>>>>>>>>> =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 rxm->packet_type =3D hns3_rx_calc_ptype(rxq, l234_info, > >>>>>>>>>>>> ol_info); > >>>>>>>>>>>> - > >>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 if (rxm->packet_type =3D=3D RTE_PTYPE_L2_ETHER_TIMESYNC) > >>>>>>>>>>>> =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 rxm->ol_flags |=3D RTE_MBUF_F_RX_IEEE1588_PTP; > >>>>>>>>>>>> =C2=A0=C2=A0 +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if= (unlikely(rxq->crc_len > 0)) { > >>>>>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 if (hns3_need_recalculate_crc(rxq, rxm)) > >>>>>>>>>>>> +=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 hns3_recalculate_crc(rxq, rxm); > >>>>>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 rxm->pkt_len -=3D rxq->crc_len; > >>>>>>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 rxm->data_len -=3D rxq->crc_len; > >>>>>>>>>>>> =20 > >>>>>>>>>>> > >>>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is > >>>>>>>>>>> practically same as stripping CRC. > >>>>>>>>>>> > >>>>>>>>>>> We don't count CRC length in the statistics, but it should be > >>>>>>>>>>> accessible > >>>>>>>>>>> in the payload by the user. =20 > >>>>>>>>>> Our drivers are behaving exactly as you say. > >>>>>>>>>> =20 > >>>>>>>>> > >>>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by > >>>>>>>>> 'rxq->crc_len', can you please explain what above lines does? > >>>>>>>>> > >>>>>>>>> =20 > >>>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue, > >>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rxdp->rx.bd_ba= se_info =3D 0; > >>>>>>>> > >>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rxm->data_off = =3D RTE_PKTMBUF_HEADROOM; > >>>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rxm->pkt_len =3D (ui= nt16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) - > >>>>>>>> -=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 rxq->crc_len; > >>>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rxm->pkt_len =3D rte= _le_to_cpu_16(rxd.rx.pkt_len); > >>>>>>>> > >>>>>>>> In the previous code above, the 'pkt_len' is set to the length o= btained > >>>>>>>> from the BD. the length obtained from the BD already contains CR= C length. > >>>>>>>> But as you said above, the DPDK requires that the length of the = mbuf > >>>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from > >>>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logi= c, it > >>>>>>>> just moves the code around. > >>>>>>>> =20 > >>>>>>> > >>>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, i= ndeed > >>>>>>> it is other way around and this is our confusion. > >>>>>>> > >>>>>>> CRC length shouldn't be in the statistics, I mean in received byt= es stats. > >>>>>>> Assume that received packet is 128 bytes and we know it has the C= RC, > >>>>>>> Rx received bytes stat should be 124 (rx_bytes =3D 128 - CRC =3D = 124) > >>>>>>> > >>>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length, > >>>>>>> including CRC. > >>>>>>> > >>>>>>> As application explicitly requested to KEEP CRC, it will know las= t 4 > >>>>>>> bytes are CRC. > >>>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, = so if > >>>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know i= f 4 > >>>>>>> bytes after 'mbuf->data_len' is valid CRC or not. > >>>>>>> =20 > >>>>>> I agree with you. > >>>>>> > >>>>>> But the implementation of other PMDs supported KEEP_CRC is like th= is. > >>>>>> In addition, there are probably many users that are already using = it. > >>>>>> If we modify it, it may cause applications incompatible. > >>>>>> > >>>>>> what do you think? > >>>>>> =20 > >>>>> This is documented in the ethdev [1], better to follow the document= ation > >>>>> for all PMDs, can you please highlight the relevant driver code, we= can > >>>>> discuss it with their maintainers. > >>>>> > >>>>> Alternatively we can document this additionally in the KEEP_CRC fea= ture > >>>>> document if it helps for the applications. > >>>>> > >>>>> > >>>>> [1] > >>>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=3Dv23.11#n= 257 =20 > >>>> > >>>> Currently=EF=BC=8Cthis documentation does not describe whether pkt_l= en and data_len should contain crc_len. > >>>> =20 > >>> > >>> I think it is clear that pkt_len and data_len should contain crc_len,= we > >>> can ask for more comments. =20 > >> This patch doesn't change the logic for hns3 PMD and the implementatio= n of > >> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patc= h first? > >> =20 > >=20 > > If hns3 behaving against the documented behavior, I don't understand why > > you are pushing for merging this patch, instead of fixing it. > > =20 >=20 > >=20 > > Other drivers behavior is something else, not directly related to this > > patch, but again if you can provide references we can discuss with their > > maintainers. > > =20 > Hi, all maintainers, > We need your opinions on whether pkt_len and data_len should contain CRC= =20 > len. The KEEP CRC feature is related. As if it is enabled, most drivers > will substract CRC len from pkt_len and data_len. which means users > cannot read the CRC data through the DPDK framework interface. >=20 > Among the drivers that support keeping CRC, only the bnxt, cfpl, idpf, > qede and sfc get the pkt_len and data_len from the descriptor and not > subtract CRC len by drivers. I don't know if the length of these drivers= =20 > includes the CRC len or not, please confirm that, thanks. >=20 >=20 > Back to the current patch. > Hi, Ferruh. > Obviously, if we need to give users access to the CRC data, we'll have > to modify the defination in ethdev and usage in most drivers. >=20 > I don't think this change will be backported. Am I wrong? >=20 > But this patch for hns3 bugfix, need to be backported. >=20 > That's why we can separate this patch from the confirmation of the > meaning of pkt_len and data_len. > So can this patch merge first? >=20 > Thanks, > Jie Hai >=20 > > =20 > >>> =20 > >>>> Do you mean that we add this description in the KEEP_CRC feature doc= ument > >>>> and notify all drivers that support KEEP_CRC to follow this document= ation? > >>>> > >>>> If so, can you merge this patch first? > >>>> Then we send a RFC to disscuss it with all PMDs maintainer. > >>>> =20 > >>> > >>> Not for drivers, just a suggestion that if we should update feature > >>> documentation with above information for users. So there is no > >>> dependency to features document update. > >>> > >>> =20 > >> Sorry I'm more confused. What should we do next? =20 > >=20 > > There is already API documentation about KEEP_CRC, I think that is > > already sufficient for driver developers. > >=20 > > I am just brainstorming if updating './doc/guides/nics/features.rst' can > > help end user, but it is not an action or blocker for this patch. > >=20 > > Next step is to update this path. IMHO the only sane thing is: -if keep crc is enabled then pkt_len and data_len include the extra bytes= for the CRC. -if keep crc is disabled, then pkt_len and data_len match the length of t= he packet without the CRC. Other than driver testing, never saw much point to using keep crc.