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 78D4242D26; Thu, 22 Jun 2023 21:59:56 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0BD4640DDA; Thu, 22 Jun 2023 21:59:56 +0200 (CEST) Received: from mail-ot1-f53.google.com (mail-ot1-f53.google.com [209.85.210.53]) by mails.dpdk.org (Postfix) with ESMTP id 834B3406BA for ; Thu, 22 Jun 2023 21:59:54 +0200 (CEST) Received: by mail-ot1-f53.google.com with SMTP id 46e09a7af769-6b720fd0e60so41117a34.1 for ; Thu, 22 Jun 2023 12:59:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1687463993; x=1690055993; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=OCxueIvXvzvAyP+YaUX70lQGR1fuISj1/9H9OTrLomM=; b=dnzK8jGfW6MsJLT6V9AzBbRlLAbx9RamQwRV6+juEq2/zZVrDYJwP2j68w6IUPSdbA GtuntYKZCGaQgyPtkUI0wsAIVcd8mM699psOUbFrkF6ve87ihBiuse8uWiZxEF/p+Vkp NwJIWNKQK3mOafq4ShzEC1/mVeYPSkpSAxa80= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687463993; x=1690055993; 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=OCxueIvXvzvAyP+YaUX70lQGR1fuISj1/9H9OTrLomM=; b=jTqSj5ktUPHBVzPeFdjPcNa6/cPiI44n0cktZfW3hf8pTcdYhkCuG8qfTLc1WayN7l 9sM9Hi0hmdsk541/ncNVJlBpQGoKwJuTFBAMyowuFzvgK+6XPHgAZVnE0uAiwQPLVkZP ExjCeH4z90gEkpEiSp/B+ekOlqEbnw2ikqi0pVEJgGHw4eBQflgGfOKNalsmEegRnyAL cFBDbeGqfWS34i1NYUq/Ks+ncMZEZGSMcLnNRV0iXDqzZ+4WABL76M0bqPi6uHy3KHfv U0Wt9BAFiyVwGNz0O0O6DH9XPUswSkf5B+IISyHHKrwKSRWZA7CBM74bti7EZMaWBHz/ yUww== X-Gm-Message-State: AC+VfDxb1nv0omh26mGuXUQmkg0UyyzF+OwNBkNlGA8rQljoP0JpOGyx kGwWfYbpJ0w0hYSo9mgKHPSv29rRj4k/I5HWBZKIBQ== X-Google-Smtp-Source: ACHHUZ48KfIwDrLu+TLKatPN6Ie+K3OU1wCwsnXPym49xvuGnKcK1sPQmKyTGw2IhpwTL6R2lZH4Pq1QqfZi5z+/fYg= X-Received: by 2002:a05:6870:c813:b0:19f:4dc2:428e with SMTP id ee19-20020a056870c81300b0019f4dc2428emr16308159oab.14.1687463993626; Thu, 22 Jun 2023 12:59:53 -0700 (PDT) MIME-Version: 1.0 References: <1679084388-19267-1-git-send-email-roretzla@linux.microsoft.com> <1686087947-15471-1-git-send-email-roretzla@linux.microsoft.com> <20230609151320.GA1770@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> In-Reply-To: <20230609151320.GA1770@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> From: Patrick Robb Date: Thu, 22 Jun 2023 15:59:42 -0400 Message-ID: Subject: Re: [PATCH v5 0/6] replace rte atomics with GCC builtin atomics To: Tyler Retzlaff Cc: David Marchand , dev@dpdk.org, Olivier Matz , Bruce Richardson , Kevin Laatz , Qiming Yang , Qi Zhang , Wenjun Wu , Tetsuya Mukawa , Honnappa.Nagarahalli@arm.com, thomas@monjalon.net Content-Type: multipart/alternative; boundary="000000000000e96a0505febd53a0" 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 --000000000000e96a0505febd53a0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I want to report a possible regression from this patch series seen from CI testing on our Intel 82599ES 10G NIC, which we failed to report to patchwork when this initially went under CI due to a bug in our Jenkins reporting scripts. Use of the ixgbe driver appears to be affected. Tyler I apologize for the issues seen with reporting. We've made some temporary changes to avoid this happening again, and are currently reworking our reporting process entirely to provide greater reliability. Here is a DTS snippet showing the issue, and the full log for the failing virtio_smoke test can be downloaded here: https://dpdkdashboard.iol.unh.edu/results/dashboard/patchsets/26560/ 06/06/2023 18:22:58 TestVirtioSmoke: Start send packets and verify 06/06/2023 18:22:58 tester: ifconfig enp134s0f0 mtu 9000 06/06/2023 18:22:58 tester: 06/06/2023 18:42:59 TestVirtioSmoke: Test Case test_virtio_pvp Result FAILED: TIMEOUT on port start 0 06/06/2023 18:42:59 TestVirtioSmoke: port start 0 ixgbe_dev_wait_setup_link_complete(): IXGBE link thread not complete too long time! ixgbe_dev_wait_setup_link_complete(): IXGBE link thread not complete too long time! ixgbe_dev_wait_setup_link_complete(): IXGBE link thread not complete too long time! We initially took this Intel10G testing offline to investigate as we thought it was a lab infra failure. Obviously that wasn't the case, so ideally we will bring this back online when appropriate. But, I don't want to do so right now and start failing everyone's patchseries which are obviously unrelated to this. Comments on this are welcome, otherwise of course I will just return this test coverage to our CI when the state of the git tree allows for it. Apologies for the missing report and the timeline on this. We are taking action to deliver results more reliably going forward. On Fri, Jun 9, 2023 at 11:13=E2=80=AFAM Tyler Retzlaff wrote: > On Fri, Jun 09, 2023 at 05:01:53PM +0200, David Marchand wrote: > > On Tue, Jun 6, 2023 at 11:45=E2=80=AFPM Tyler Retzlaff > > wrote: > > > > > > Replace the use of rte_atomic.h types and functions, instead use GCC > > > supplied C++11 memory model builtins. > > > > > > This series covers the libraries and drivers that are built on Window= s. > > > > > > The code has be converted to use the __atomic builtins but there are > > > additional during conversion I notice that there may be some issues > > > that need to be addressed. > > > > > > I'll comment in the patches where my concerns are so the maintainers > > > may comment. > > > > > > v5: > > > * use relaxed ordering for counter increments in net/ring patch > > > * remove note comments from net/ring patch > > > > > > v4: > > > > > > * drop patch for lib/ring it will be provided by ARM / Honnappa > > > * rebase for changes in dma/idxd merge > > > * adapt __atomic_fetch_sub(...) - 1 =3D=3D 0 to be > (__atomic_fetch_sub(...) =3D=3D 1) > > > as per feedback. > > > * drop one /* NOTE: review for potential ordering optimization */ > since > > > the note reference non-critical to perf control path. > > > > > > note: > > > > > > Remainder of the NOTE comments have been retained since there > > > seems to be no consensus but stronger opinion/argument to keep > > > expressed. while I generally agree that changes should not > > > include ``TODO'' style comments I also agree that without these > > > comments in your face people are very unlikely to feel compelled > > > to make the review they are trying to solicit without them. if > > > it is absolute that the series won't be merged with them then I > > > will remove them, but please be explicit soon. > > > > > > v3: > > > * style, don't use c99 comments > > > > > > v2: > > > * comment code where optimizations may be possible now that memory > > > order can be specified. > > > * comment code where operations should potentially be atomic so tha= t > > > maintainers can review. > > > * change a couple of variables labeled as counters to be unsigned. > > > > > > Tyler Retzlaff (6): > > > stack: replace rte atomics with GCC builtin atomics > > > dma/idxd: replace rte atomics with GCC builtin atomics > > > net/ice: replace rte atomics with GCC builtin atomics > > > net/ixgbe: replace rte atomics with GCC builtin atomics > > > net/null: replace rte atomics with GCC builtin atomics > > > net/ring: replace rte atomics with GCC builtin atomics > > > > > > drivers/dma/idxd/idxd_internal.h | 3 +-- > > > drivers/dma/idxd/idxd_pci.c | 11 ++++++----- > > > drivers/net/ice/ice_dcf.c | 1 - > > > drivers/net/ice/ice_dcf_ethdev.c | 1 - > > > drivers/net/ice/ice_ethdev.c | 12 ++++++++---- > > > drivers/net/ixgbe/ixgbe_bypass.c | 1 - > > > drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++------ > > > drivers/net/ixgbe/ixgbe_ethdev.h | 3 ++- > > > drivers/net/ixgbe/ixgbe_flow.c | 1 - > > > drivers/net/ixgbe/ixgbe_rxtx.c | 1 - > > > drivers/net/null/rte_eth_null.c | 28 ++++++++++++++++++---------- > > > drivers/net/ring/rte_eth_ring.c | 20 ++++++++++---------- > > > lib/stack/rte_stack_lf_generic.h | 16 +++++++++------- > > > 13 files changed, 66 insertions(+), 50 deletions(-) > > > > I am not really enthousiastic about those NOTE:. > > I would prefer we get an explicit go/nogo from each maintainers, but > > this did not happen. > > I think that this indicates that those NOTE: will rot in the code now. > > > > Thomas proposed to track those NOTE: in the release announce mail and > > that we ping maintainers regularly. > > Let's see how it goes. > > Let's leave it for one release cycle, if with the the announce mail > maintainers take no action within that time I'll commit to going > through and cleaning them out before 23.11 rc1. > > > > > I am merging this series so we can progress on the $SUBJECT. > > Series applied, thanks. > > Thanks David, this will allow forward progress. > > > > > > > Tyler, about the patch on the ring library that was dropped by got no > > viable alternative, I'll wait for a decision from ARM and you. > > I'll wait for Honnappa to follow up and we'll decide what to do when he > does. > > > > > -- > > David Marchand > --=20 Patrick Robb Technical Service Manager UNH InterOperability Laboratory 21 Madbury Rd, Suite 100, Durham, NH 03824 www.iol.unh.edu --000000000000e96a0505febd53a0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I want to report a possible regression from this patc= h series=C2=A0seen from CI testing=C2=A0on=C2=A0our Intel 82599ES 10G NIC, = which we failed to report to patchwork when this initially went under CI du= e to a bug in our Jenkins reporting scripts. Use of the ixgbe driver appear= s to be affected. Tyler I apologize for the=C2=A0issues seen with reporting= . We've made some temporary changes to avoid this happening again, and = are currently reworking our reporting process entirely to provide greater r= eliability.=C2=A0

Here is a DTS snippet showing the issue, and the f= ull log for the failing=C2=A0virtio_smoke test can be downloaded here:=C2= =A0https://dpdkdashboard.iol.unh.edu/results/dashboard/patchsets/2656= 0/

06/06/2023 18:22:58 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0TestVirtioSmoke: Start send packets and verify
06/06/2023 = 18:22:58 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 tester: ifconfig enp134s0f0 mtu 9000
06/06/2023 18:22:= 58 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 tester:
06/06/2023 18:42:59 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0TestVirtioSmoke: Test Case test_virtio_pvp Resul= t FAILED: TIMEOUT on port start 0
06/06/2023 18:42:59 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0TestVirtioSmoke: port start 0

= ixgbe_dev_wait_setup_link_complete(): IXGBE link thread not complete too lo= ng time!
ixgbe_dev_wait_setup_link_complete(): IXGBE link thread not com= plete too long time!
ixgbe_dev_wait_setup_link_complete(): IXGBE link th= read not complete too long time!

We initially took this Intel10G tes= ting offline to investigate as we thought it was a lab infra failure. Obvio= usly that wasn't the case, so ideally we will bring this back online wh= en appropriate. But, I don't want to do so right now and start failing = everyone's patchseries=C2=A0which are obviously unrelated to this. Comm= ents on this are welcome, otherwise of course I will just return this test = coverage to our CI when the state of the git tree allows for it.=C2=A0

Apologies for the missing report and the timeline on t= his. We are taking action to deliver results more reliably going forward.

On Fri, Jun 9, 2023 at 11:13=E2=80=AFAM Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
On = Fri, Jun 09, 2023 at 05:01:53PM +0200, David Marchand wrote:
> On Tue, Jun 6, 2023 at 11:45=E2=80=AFPM Tyler Retzlaff
> <= roretzla@linux.microsoft.com> wrote:
> >
> > Replace the use of rte_atomic.h types and functions, instead use = GCC
> > supplied C++11 memory model builtins.
> >
> > This series covers the libraries and drivers that are built on Wi= ndows.
> >
> > The code has be converted to use the __atomic builtins but there = are
> > additional during conversion I notice that there may be some issu= es
> > that need to be addressed.
> >
> > I'll comment in the patches where my concerns are so the main= tainers
> > may comment.
> >
> > v5:
> >=C2=A0 =C2=A0* use relaxed ordering for counter increments in net/= ring patch
> >=C2=A0 =C2=A0* remove note comments from net/ring patch
> >
> > v4:
> >
> >=C2=A0 =C2=A0* drop patch for lib/ring it will be provided by ARM = / Honnappa
> >=C2=A0 =C2=A0* rebase for changes in dma/idxd merge
> >=C2=A0 =C2=A0* adapt __atomic_fetch_sub(...) - 1 =3D=3D 0 to be (_= _atomic_fetch_sub(...) =3D=3D 1)
> >=C2=A0 =C2=A0 =C2=A0as per feedback.
> >=C2=A0 =C2=A0* drop one /* NOTE: review for potential ordering opt= imization */ since
> >=C2=A0 =C2=A0 =C2=A0the note reference non-critical to perf contro= l path.
> >
> >=C2=A0 =C2=A0note:
> >
> >=C2=A0 =C2=A0Remainder of the NOTE comments have been retained sin= ce there
> >=C2=A0 =C2=A0seems to be no consensus but stronger opinion/argumen= t to keep
> >=C2=A0 =C2=A0expressed. while I generally agree that changes shoul= d not
> >=C2=A0 =C2=A0include ``TODO'' style comments I also agree = that without these
> >=C2=A0 =C2=A0comments in your face people are very unlikely to fee= l compelled
> >=C2=A0 =C2=A0to make the review they are trying to solicit without= them. if
> >=C2=A0 =C2=A0it is absolute that the series won't be merged wi= th them then I
> >=C2=A0 =C2=A0will remove them, but please be explicit soon.
> >
> > v3:
> >=C2=A0 =C2=A0* style, don't use c99 comments
> >
> > v2:
> >=C2=A0 =C2=A0* comment code where optimizations may be possible no= w that memory
> >=C2=A0 =C2=A0 =C2=A0order can be specified.
> >=C2=A0 =C2=A0* comment code where operations should potentially be= atomic so that
> >=C2=A0 =C2=A0 =C2=A0maintainers can review.
> >=C2=A0 =C2=A0* change a couple of variables labeled as counters to= be unsigned.
> >
> > Tyler Retzlaff (6):
> >=C2=A0 =C2=A0stack: replace rte atomics with GCC builtin atomics > >=C2=A0 =C2=A0dma/idxd: replace rte atomics with GCC builtin atomic= s
> >=C2=A0 =C2=A0net/ice: replace rte atomics with GCC builtin atomics=
> >=C2=A0 =C2=A0net/ixgbe: replace rte atomics with GCC builtin atomi= cs
> >=C2=A0 =C2=A0net/null: replace rte atomics with GCC builtin atomic= s
> >=C2=A0 =C2=A0net/ring: replace rte atomics with GCC builtin atomic= s
> >
> >=C2=A0 drivers/dma/idxd/idxd_internal.h |=C2=A0 3 +--
> >=C2=A0 drivers/dma/idxd/idxd_pci.c=C2=A0 =C2=A0 =C2=A0 | 11 ++++++= -----
> >=C2=A0 drivers/net/ice/ice_dcf.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2= =A0 1 -
> >=C2=A0 drivers/net/ice/ice_dcf_ethdev.c |=C2=A0 1 -
> >=C2=A0 drivers/net/ice/ice_ethdev.c=C2=A0 =C2=A0 =C2=A0| 12 ++++++= ++----
> >=C2=A0 drivers/net/ixgbe/ixgbe_bypass.c |=C2=A0 1 -
> >=C2=A0 drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++------ > >=C2=A0 drivers/net/ixgbe/ixgbe_ethdev.h |=C2=A0 3 ++-
> >=C2=A0 drivers/net/ixgbe/ixgbe_flow.c=C2=A0 =C2=A0|=C2=A0 1 -
> >=C2=A0 drivers/net/ixgbe/ixgbe_rxtx.c=C2=A0 =C2=A0|=C2=A0 1 -
> >=C2=A0 drivers/net/null/rte_eth_null.c=C2=A0 | 28 ++++++++++++++++= ++----------
> >=C2=A0 drivers/net/ring/rte_eth_ring.c=C2=A0 | 20 ++++++++++------= ----
> >=C2=A0 lib/stack/rte_stack_lf_generic.h | 16 +++++++++-------
> >=C2=A0 13 files changed, 66 insertions(+), 50 deletions(-)
>
> I am not really enthousiastic about those NOTE:.
> I would prefer we get an explicit go/nogo from each maintainers, but > this did not happen.
> I think that this indicates that those NOTE: will rot in the code now.=
>
> Thomas proposed to track those NOTE: in the release announce mail and<= br> > that we ping maintainers regularly.
> Let's see how it goes.

Let's leave it for one release cycle, if with the the announce mail
maintainers take no action within that time I'll commit to going
through and cleaning them out before 23.11 rc1.

>
> I am merging this series so we can progress on the $SUBJECT.
> Series applied, thanks.

Thanks David, this will allow forward progress.

>
>
> Tyler, about the patch on the ring library that was dropped by got no<= br> > viable alternative, I'll wait for a decision from ARM and you.

I'll wait for Honnappa to follow up and we'll decide what to do whe= n he
does.

>
> --
> David Marchand


--

Patrick Robb

<= span style=3D"font-size:10pt;font-family:Arial;color:rgb(0,0,0);background-= color:transparent;vertical-align:baseline;white-space:pre-wrap">Technical S= ervice Manager

UNH InterOperability Laboratory

21 Madbury Rd, Suite 100, Durham, NH 03824

www.iol.unh.edu


--000000000000e96a0505febd53a0--