Re: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer synchronization

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 13 Sep 2018 23:45:31 +0000
> From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> CC: Ola Liljedahl <ola.liljed...@arm.com>, "Kokkilagadda, Kiran"
>  <kiran.kokkilaga...@cavium.com>, "Gavin Hu (Arm Technology China)"
>  <gavin...@arm.com>, Ferruh Yigit <ferruh.yi...@intel.com>, "Jacob,  Jerin"
>  <jerin.jacobkollanukka...@cavium.com>, "dev@dpdk.org" <dev@dpdk.org>, nd
>  <n...@arm.com>, Steve Capper <steve.cap...@arm.com>, "Phil Yang (Arm
>  Technology China)" <phil.y...@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer
>  synchronization
> 
> External Email
> 
> -----Original Message-----
> > Date: Thu, 13 Sep 2018 17:40:53 +0000
> > From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > To: Jerin Jacob <jerin.ja...@caviumnetworks.com>, Ola Liljedahl
> > <ola.liljed...@arm.com>
> > CC: "Kokkilagadda, Kiran" <kiran.kokkilaga...@cavium.com>, "Gavin Hu
> > (Arm  Technology China)" <gavin...@arm.com>, Ferruh Yigit
> > <ferruh.yi...@intel.com>, "Jacob,  Jerin"
> >  <jerin.jacobkollanukka...@cavium.com>, "dev@dpdk.org" <dev@dpdk.org>,
> > nd  <n...@arm.com>, Steve Capper <steve.cap...@arm.com>, "Phil Yang (Arm
> > Technology China)" <phil.y...@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer
> > synchronization
> >
> >
> > Hi Jerin,
> >         Is there any reason for having 'RTE_RING_USE_C11_MEM_MODEL', which 
> > is specific to rte_ring? I do not see a need for choosing only some 
> > algorithms to work with C11 model. I suggest that we change this to 
> > 'RTE_USE_C11_MEM_MODEL' so that it can apply to all libraries/algorithms.
> 
> 
> Yes. Makes sense to me to keep only single config option.
> 
> rte_ring has 2 sets of algorithms for Arm architecture, one with C11 memory 
> model and the other with barriers. Going forward (for ex: for KNI), I think 
> we should support C11 memory model only and skip the barriers.

IMO, Both should be supported and set N as in the config/common_base.
Based on architecture or micro architecture the performance can vary.
So keeping both options and allowing to override to arch/micro arch
specific config file makes sense to me.(like existing model, as smp_*
ops are compiler NOP for x86)
 
> Also, do you see any issues in making C11 memory model default for Arm 
> architecture?

It is already set default Y to arm64. see config/common_armv8a_linuxapp.

And it is possible for micro architecture to override, see
config/defconfig_arm64-thunderx-linuxapp-gcc


> 
> >
> > Thank you,
> > Honnappa
> >
> > -----Original Message-----
> > From: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> > Sent: Wednesday, August 29, 2018 3:58 AM
> > To: Ola Liljedahl <ola.liljed...@arm.com>
> > Cc: Kokkilagadda, Kiran <kiran.kokkilaga...@cavium.com>; Honnappa
> > Nagarahalli <honnappa.nagaraha...@arm.com>; Gavin Hu
> > <gavin...@arm.com>; Ferruh Yigit <ferruh.yi...@intel.com>; Jacob,
> > Jerin <jerin.jacobkollanukka...@cavium.com>; dev@dpdk.org; nd
> > <n...@arm.com>; Steve Capper <steve.cap...@arm.com>
> > Subject: Re: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer
> > synchronization
> >
> > -----Original Message-----
> > > Date: Wed, 29 Aug 2018 08:47:56 +0000
> > > From: Ola Liljedahl <ola.liljed...@arm.com>
> > > To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> > > CC: "Kokkilagadda, Kiran" <kiran.kokkilaga...@cavium.com>, Honnappa
> > > Nagarahalli <honnappa.nagaraha...@arm.com>, Gavin Hu
> > > <gavin...@arm.com>,  Ferruh Yigit <ferruh.yi...@intel.com>, "Jacob,  
> > > Jerin"
> > >  <jerin.jacobkollanukka...@cavium.com>, "dev@dpdk.org"
> > > <dev@dpdk.org>, nd  <n...@arm.com>, Steve Capper
> > > <steve.cap...@arm.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer
> > > synchronization
> > > user-agent: Microsoft-MacOutlook/10.10.0.180812
> > >
> > >
> > > There was a mention of rte_ring which is a different data structure. But 
> > > perhaps I misunderstood why this was mentioned and the idea was only to 
> > > use the C11 memory model as is also used in rte_ring nowadays.
> > >
> > > But why would we have different code for x86 and for other architectures 
> > > (ARM, Power)? If we use the C11 memory model (and e.g. GCC __atomic 
> > > builtins), the code generated for x86 will be the same. 
> > > __atomic_load(__ATOMIC_ACQUIRE) and __atomic_store(__ATOMIC_RELEASE) 
> > > should translate to plain loads and stores on x86?
> >
> > # One reason was __atomic builtins  primitives were implemented in gcc 4.7 
> > and x86 would like to support < gcc 4.7 and ICC compiler.
> > # The theme was no change in the existing code for x86.I am not sure about 
> > the code generation for x86 with __atomic builtins, I let x86 maintainers 
> > to comments on this.
> >
> >
> > >
> > > -- Ola
> > >
> > > On 29/08/2018, 10:28, "Jerin Jacob" <jerin.ja...@caviumnetworks.com> 
> > > wrote:
> > >
> > >     -----Original Message-----
> > >     > Date: Wed, 29 Aug 2018 07:34:34 +0000
> > >     > From: Ola Liljedahl <ola.liljed...@arm.com>
> > >     > To: "Kokkilagadda, Kiran" <kiran.kokkilaga...@cavium.com>, Honnappa
> > >     >  Nagarahalli <honnappa.nagaraha...@arm.com>, Gavin Hu 
> > > <gavin...@arm.com>,
> > >     >  Ferruh Yigit <ferruh.yi...@intel.com>, "Jacob,  Jerin"
> > >     >  <jerin.jacobkollanukka...@cavium.com>
> > >     > CC: "dev@dpdk.org" <dev@dpdk.org>, nd <n...@arm.com>, Steve Capper
> > >     >  <steve.cap...@arm.com>
> > >     > Subject: Re: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer
> > >     >  synchronization
> > >     > user-agent: Microsoft-MacOutlook/10.10.0.180812
> > >     >
> > >     > Is the rte_kni kernel/user binary interface subject to backwards 
> > > compatibility requirements? Or can we change it for a new DPDK release?
> > >
> > >     What would be the change in interface? Is it removing the volatile for
> > >     C11 case, Then you can use anonymous union OR #define to keep the size
> > >     and offset of the element intact.
> > >
> > >     struct rte_kni_fifo {
> > >     #ifndef RTE_C11...
> > >             volatile unsigned write;     /**< Next position to be 
> > > written*/
> > >             volatile unsigned read;      /**< Next position to be read */
> > >     #else
> > >             unsigned write;     /**< Next position to be written*/
> > >             unsigned read;      /**< Next position to be read */
> > >     #endif
> > >             unsigned len;                /**< Circular buffer length */
> > >             unsigned elem_size;          /**< Pointer size - for 32/64 
> > > bitOS */
> > >             void *volatile buffer[];     /**< The buffer contains mbuf
> > >     pointers */
> > >     };
> > >
> > >     Anonymous union example:
> > >     https://git.dpdk.org/dpdk/tree/lib/librte_mbuf/rte_mbuf.h#n461
> > >
> > >     You can check the ABI breakage by devtools/validate-abi.sh
> > >
> > >     >
> > >     > -- Ola
> > >     >
> > >     > From: "Kokkilagadda, Kiran" <kiran.kokkilaga...@cavium.com>
> > >     > Date: Wednesday, 29 August 2018 at 07:50
> > >     > To: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>, Gavin Hu 
> > > <gavin...@arm.com>, Ferruh Yigit <ferruh.yi...@intel.com>, "Jacob, Jerin" 
> > > <jerin.jacobkollanukka...@cavium.com>
> > >     > Cc: "dev@dpdk.org" <dev@dpdk.org>, nd <n...@arm.com>, Ola Liljedahl 
> > > <ola.liljed...@arm.com>, Steve Capper <steve.cap...@arm.com>
> > >     > Subject: Re: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer 
> > > synchronization
> > >     >
> > >     >
> > >     > Agreed. Please go a head and make the changes. You need to make 
> > > same change in kernel side also. And please use c11 ring (see rte_ring) 
> > > mechanism so that it won't impact other platforms like intel. We need 
> > > this change just for arm and ppc.
> > >     >
> > >     > ________________________________
> > >     > From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > >     > Sent: Wednesday, August 29, 2018 10:29 AM
> > >     > To: Gavin Hu; Kokkilagadda, Kiran; Ferruh Yigit; Jacob, Jerin
> > >     > Cc: dev@dpdk.org; nd; Ola Liljedahl; Steve Capper
> > >     > Subject: RE: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer 
> > > synchronization
> > >     >
> > >     >
> > >     > External Email
> > >     >
> > >     > I agree with Gavin here. Store to fifo->write and fifo->read can 
> > > get hoisted resulting in accessing invalid buffer array entries or over 
> > > writing of the buffer array entries.
> > >     >
> > >     > IMO, we should solve this using c11 atomics. This will also help 
> > > remove the use of ‘volatile’ from ‘rte_kni_fifo’ structure.
> > >     >
> > >     >
> > >     >
> > >     > If you want us to put together a patch with this idea, please let 
> > > us know.
> > >     >
> > >     >
> > >     >
> > >     > Thank you,
> > >     >
> > >     > Honnappa
> > >     >
> > >     >
> > >     >
> > >     > From: Gavin Hu
> > >     > Sent: Tuesday, August 28, 2018 2:31 PM
> > >     > To: Kokkilagadda, Kiran <kiran.kokkilaga...@cavium.com>; Ferruh 
> > > Yigit <ferruh.yi...@intel.com>; Jacob, Jerin 
> > > <jerin.jacobkollanukka...@cavium.com>
> > >     > Cc: dev@dpdk.org; Honnappa Nagarahalli 
> > > <honnappa.nagaraha...@arm.com>; nd <n...@arm.com>; Ola Liljedahl 
> > > <ola.liljed...@arm.com>; Steve Capper <steve.cap...@arm.com>
> > >     > Subject: RE: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer 
> > > synchronization
> > >     >
> > >     >
> > >     >
> > >     > Assuming reader and writer may execute on different CPU's, this 
> > > become standard multithreaded programming.
> > >     >
> > >     > We are concerned about that update the reader pointer too 
> > > early(weak ordering may reorder it before reading from the slots), that 
> > > means the slots are released and may immediately overwritten by the 
> > > writer then you get “too new” data and get lost of the old data.
> > >     >
> > >     >
> > >     >
> > >     > From: Kokkilagadda, Kiran 
> > > <kiran.kokkilaga...@cavium.com<mailto:kiran.kokkilaga...@cavium.com>>
> > >     > Sent: Tuesday, August 28, 2018 6:44 PM
> > >     > To: Gavin Hu <gavin...@arm.com<mailto:gavin...@arm.com>>; Ferruh 
> > > Yigit <ferruh.yi...@intel.com<mailto:ferruh.yi...@intel.com>>; Jacob, 
> > > Jerin 
> > > <jerin.jacobkollanukka...@cavium.com<mailto:jerin.jacobkollanukka...@cavium.com>>
> > >     > Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Honnappa Nagarahalli 
> > > <honnappa.nagaraha...@arm.com<mailto:honnappa.nagaraha...@arm.com>>
> > >     > Subject: Re: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer 
> > > synchronization
> > >     >
> > >     >
> > >     >
> > >     > In this instance there won't be any problem, as until the value of 
> > > fifo->write changes, this loop won't get executed. As of now we didn't 
> > > see any issue with it and for performance reasons, we don't want to keep 
> > > read barrier.
> > >     >
> > >     >
> > >     >
> > >     >
> > >     >
> > >     > ________________________________
> > >     >
> > >     > From: Gavin Hu <gavin...@arm.com<mailto:gavin...@arm.com>>
> > >     > Sent: Monday, August 27, 2018 9:10 PM
> > >     > To: Ferruh Yigit; Kokkilagadda, Kiran; Jacob, Jerin
> > >     > Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Honnappa Nagarahalli
> > >     > Subject: RE: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer 
> > > synchronization
> > >     >
> > >     >
> > >     >
> > >     > External Email
> > >     >
> > >     > This fix is not complete, kni_fifo_get requires a read fence also, 
> > > otherwise it probably gets stale data on a weak ordering platform.
> > >     >
> > >     > > -----Original Message-----
> > >     > > From: dev <dev-boun...@dpdk.org<mailto:dev-boun...@dpdk.org>> On 
> > > Behalf Of Ferruh Yigit
> > >     > > Sent: Monday, August 27, 2018 10:08 PM
> > >     > > To: Kiran Kumar 
> > > <kkokkilaga...@caviumnetworks.com<mailto:kkokkilaga...@caviumnetworks.com>>;
> > >     > > 
> > > jerin.ja...@caviumnetworks.com<mailto:jerin.ja...@caviumnetworks.com>
> > >     > > Cc: dev@dpdk.org<mailto:dev@dpdk.org>
> > >     > > Subject: Re: [dpdk-dev] [PATCH v2] kni: fix kni Rx fifo producer
> > >     > > synchronization
> > >     > >
> > >     > > On 8/16/2018 10:55 AM, Kiran Kumar wrote:
> > >     > > > With existing code in kni_fifo_put, rx_q values are not being 
> > > updated
> > >     > > > before updating fifo_write. While reading rx_q in 
> > > kni_net_rx_normal,
> > >     > > > This is causing the sync issue on other core. So adding a write
> > >     > > > barrier to make sure the values being synced before updating 
> > > fifo_write.
> > >     > > >
> > >     > > > Fixes: 3fc5ca2f6352 ("kni: initial import")
> > >     > > >
> > >     > > > Signed-off-by: Kiran Kumar 
> > > <kkokkilaga...@caviumnetworks.com<mailto:kkokkilaga...@caviumnetworks.com>>
> > >     > > > Acked-by: Jerin Jacob 
> > > <jerin.ja...@caviumnetworks.com<mailto:jerin.ja...@caviumnetworks.com>>
> > >     > >
> > >     > > Acked-by: Ferruh Yigit 
> > > <ferruh.yi...@intel.com<mailto:ferruh.yi...@intel.com>>
> > >     > IMPORTANT NOTICE: The contents of this email and any attachments 
> > > are confidential and may also be privileged. If you are not the intended 
> > > recipient, please notify the sender immediately and do not disclose the 
> > > contents to any other person, use it for any purpose, or store or copy 
> > > the information in any medium. Thank you.
> > >
> > >