-----Original Message-----
> Date: Wed, 29 Aug 2018 07:34:34 +0000
> From: Ola Liljedahl <[email protected]>
> To: "Kokkilagadda, Kiran" <[email protected]>, Honnappa
> Nagarahalli <[email protected]>, Gavin Hu <[email protected]>,
> Ferruh Yigit <[email protected]>, "Jacob, Jerin"
> <[email protected]>
> CC: "[email protected]" <[email protected]>, nd <[email protected]>, Steve Capper
> <[email protected]>
> 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" <[email protected]>
> Date: Wednesday, 29 August 2018 at 07:50
> To: Honnappa Nagarahalli <[email protected]>, Gavin Hu
> <[email protected]>, Ferruh Yigit <[email protected]>, "Jacob, Jerin"
> <[email protected]>
> Cc: "[email protected]" <[email protected]>, nd <[email protected]>, Ola Liljedahl
> <[email protected]>, Steve Capper <[email protected]>
> 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 <[email protected]>
> Sent: Wednesday, August 29, 2018 10:29 AM
> To: Gavin Hu; Kokkilagadda, Kiran; Ferruh Yigit; Jacob, Jerin
> Cc: [email protected]; 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 <[email protected]>; Ferruh Yigit
> <[email protected]>; Jacob, Jerin <[email protected]>
> Cc: [email protected]; Honnappa Nagarahalli <[email protected]>; nd
> <[email protected]>; Ola Liljedahl <[email protected]>; Steve Capper
> <[email protected]>
> 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
> <[email protected]<mailto:[email protected]>>
> Sent: Tuesday, August 28, 2018 6:44 PM
> To: Gavin Hu <[email protected]<mailto:[email protected]>>; Ferruh Yigit
> <[email protected]<mailto:[email protected]>>; Jacob, Jerin
> <[email protected]<mailto:[email protected]>>
> Cc: [email protected]<mailto:[email protected]>; Honnappa Nagarahalli
> <[email protected]<mailto:[email protected]>>
> 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 <[email protected]<mailto:[email protected]>>
> Sent: Monday, August 27, 2018 9:10 PM
> To: Ferruh Yigit; Kokkilagadda, Kiran; Jacob, Jerin
> Cc: [email protected]<mailto:[email protected]>; 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 <[email protected]<mailto:[email protected]>> On Behalf Of
> > Ferruh Yigit
> > Sent: Monday, August 27, 2018 10:08 PM
> > To: Kiran Kumar
> > <[email protected]<mailto:[email protected]>>;
> > [email protected]<mailto:[email protected]>
> > Cc: [email protected]<mailto:[email protected]>
> > 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
> > > <[email protected]<mailto:[email protected]>>
> > > Acked-by: Jerin Jacob
> > > <[email protected]<mailto:[email protected]>>
> >
> > Acked-by: Ferruh Yigit
> > <[email protected]<mailto:[email protected]>>
> 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.
