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

[Jerin Jacob]

-----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.
> 
>