diff mbox series

[v3,2/4] kni: fix kni fifo synchronization

Message ID 1538989906-8349-2-git-send-email-phil.yang@arm.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series [v3,1/4] config: use one single config option for C11 memory model |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Phil Yang Oct. 8, 2018, 9:11 a.m. UTC 
  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. The same situation happens
in kni_fifo_get as well.

So syncing the values by adding memory barriers to make sure the values
being synced before updating fifo_write and fifo_read.

Fixes: 3fc5ca2 ("kni: initial import")

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
Reviewed-by: Ola Liljedahl <Ola.Liljedahl@arm.com>
Reviewed-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

---
 lib/librte_kni/rte_kni_fifo.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Stephen Hemminger Oct. 8, 2018, 9:53 p.m. UTC | #1 
On Mon,  8 Oct 2018 17:11:44 +0800
Phil Yang <phil.yang@arm.com> wrote:

> diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
> index ac26a8c..70ac14e 100644
> --- a/lib/librte_kni/rte_kni_fifo.h
> +++ b/lib/librte_kni/rte_kni_fifo.h
> @@ -28,8 +28,9 @@ kni_fifo_put(struct rte_kni_fifo *fifo, void **data, unsigned num)
>  {
>  	unsigned i = 0;
>  	unsigned fifo_write = fifo->write;
> -	unsigned fifo_read = fifo->read;
>  	unsigned new_write = fifo_write;
> +	rte_smp_rmb();
> +	unsigned fifo_read = fifo->read;
>  

The patch makes sense, but this function should be changed to match kernel code style.
That means no declarations after initial block, and use 'unsigned int' rather than 'unsigned'

Also. why is i initialized? Best practice now is to not do gratitious initialization
since it defeats compiler checks for accidental  use of uninitialized variables.

What makes sense is something like:

kni_fifo_put(struct rte_kni_fifo *fifo, void **data, unsigned num)
{
	unsigned int i, fifo_read, fifo_write, new_write;

	fifo_write = fifo->write;
	new_write = fifo_write;
	rte_smb_rmb();
	fifo_read = fifo->read;

Sorry, blaming you for issues which are inherited from original KNI code.
Maybe someone should run kernel checkpatch (not DPDK checkpatch) on it and fix those.
Phil Yang Oct. 10, 2018, 9:58 a.m. UTC | #2 
Hi Hemminger,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, October 9, 2018 5:53 AM
> To: Phil Yang (Arm Technology China) <Phil.Yang@arm.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; Gavin Hu (Arm
> Technology China) <Gavin.Hu@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ola Liljedahl <Ola.Liljedahl@arm.com>;
> ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev] [PATCH v3 2/4] kni: fix kni fifo synchronization
>
> On Mon,  8 Oct 2018 17:11:44 +0800
> Phil Yang <phil.yang@arm.com> wrote:
>
> > diff --git a/lib/librte_kni/rte_kni_fifo.h
> > b/lib/librte_kni/rte_kni_fifo.h index ac26a8c..70ac14e 100644
> > --- a/lib/librte_kni/rte_kni_fifo.h
> > +++ b/lib/librte_kni/rte_kni_fifo.h
> > @@ -28,8 +28,9 @@ kni_fifo_put(struct rte_kni_fifo *fifo, void **data,
> > unsigned num)  {
> >  unsigned i = 0;
> >  unsigned fifo_write = fifo->write;
> > -unsigned fifo_read = fifo->read;
> >  unsigned new_write = fifo_write;
> > +rte_smp_rmb();
> > +unsigned fifo_read = fifo->read;
> >
>
> The patch makes sense, but this function should be changed to match kernel
> code style.
> That means no declarations after initial block, and use 'unsigned int' rather than
> 'unsigned'
>
> Also. why is i initialized? Best practice now is to not do gratitious initialization
> since it defeats compiler checks for accidental  use of uninitialized variables.
>
> What makes sense is something like:
>
> kni_fifo_put(struct rte_kni_fifo *fifo, void **data, unsigned num) {
> unsigned int i, fifo_read, fifo_write, new_write;
>
> fifo_write = fifo->write;
> new_write = fifo_write;
> rte_smb_rmb();
> fifo_read = fifo->read;
>
> Sorry, blaming you for issues which are inherited from original KNI code.
> Maybe someone should run kernel checkpatch (not DPDK checkpatch) on it and
> fix those.

Thanks for your comment.

I think I can submit a new separate patch to fix this historical issue.

Thanks,
Phil Yang
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.
Gavin Hu Oct. 10, 2018, 10:06 a.m. UTC | #3 
> -----Original Message-----
> From: Phil Yang (Arm Technology China)
> Sent: Wednesday, October 10, 2018 5:59 PM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; Gavin Hu (Arm
> Technology China) <Gavin.Hu@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ola Liljedahl <Ola.Liljedahl@arm.com>;
> ferruh.yigit@intel.com
> Subject: RE: [dpdk-dev] [PATCH v3 2/4] kni: fix kni fifo synchronization
>
> Hi Hemminger,
>
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, October 9, 2018 5:53 AM
> > To: Phil Yang (Arm Technology China) <Phil.Yang@arm.com>
> > Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; Gavin Hu (Arm
> > Technology China) <Gavin.Hu@arm.com>; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; Ola Liljedahl <Ola.Liljedahl@arm.com>;
> > ferruh.yigit@intel.com
> > Subject: Re: [dpdk-dev] [PATCH v3 2/4] kni: fix kni fifo
> > synchronization
> >
> > On Mon,  8 Oct 2018 17:11:44 +0800
> > Phil Yang <phil.yang@arm.com> wrote:
> >
> > > diff --git a/lib/librte_kni/rte_kni_fifo.h
> > > b/lib/librte_kni/rte_kni_fifo.h index ac26a8c..70ac14e 100644
> > > --- a/lib/librte_kni/rte_kni_fifo.h
> > > +++ b/lib/librte_kni/rte_kni_fifo.h
> > > @@ -28,8 +28,9 @@ kni_fifo_put(struct rte_kni_fifo *fifo, void
> > > **data, unsigned num)  {
> > >  unsigned i = 0;
> > >  unsigned fifo_write = fifo->write;
> > > -unsigned fifo_read = fifo->read;
> > >  unsigned new_write = fifo_write;
> > > +rte_smp_rmb();
> > > +unsigned fifo_read = fifo->read;
> > >
> >
> > The patch makes sense, but this function should be changed to match
> > kernel code style.
> > That means no declarations after initial block, and use 'unsigned int'
> > rather than 'unsigned'
> >
> > Also. why is i initialized? Best practice now is to not do gratitious
> > initialization since it defeats compiler checks for accidental  use of
> uninitialized variables.
> >
> > What makes sense is something like:
> >
> > kni_fifo_put(struct rte_kni_fifo *fifo, void **data, unsigned num) {
> > unsigned int i, fifo_read, fifo_write, new_write;
> >
> > fifo_write = fifo->write;
> > new_write = fifo_write;
> > rte_smb_rmb();
> > fifo_read = fifo->read;
> >
> > Sorry, blaming you for issues which are inherited from original KNI code.
> > Maybe someone should run kernel checkpatch (not DPDK checkpatch) on it
> > and fix those.
>
> Thanks for your comment.
>
> I think I can submit a new separate patch to fix this historical issue.
>
> Thanks,
> Phil Yang

I advised a separate patch to make this patch to the point and clean.

-Gavin
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.
Ferruh Yigit Oct. 10, 2018, 2:42 p.m. UTC | #4 
On 10/10/2018 11:06 AM, Gavin Hu (Arm Technology China) wrote:
> 
> 
>> -----Original Message-----
>> From: Phil Yang (Arm Technology China)
>> Sent: Wednesday, October 10, 2018 5:59 PM
>> To: Stephen Hemminger <stephen@networkplumber.org>
>> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; Gavin Hu (Arm
>> Technology China) <Gavin.Hu@arm.com>; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; Ola Liljedahl <Ola.Liljedahl@arm.com>;
>> ferruh.yigit@intel.com
>> Subject: RE: [dpdk-dev] [PATCH v3 2/4] kni: fix kni fifo synchronization
>>
>> Hi Hemminger,
>>
>>> -----Original Message-----
>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>> Sent: Tuesday, October 9, 2018 5:53 AM
>>> To: Phil Yang (Arm Technology China) <Phil.Yang@arm.com>
>>> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com; Gavin Hu (Arm
>>> Technology China) <Gavin.Hu@arm.com>; Honnappa Nagarahalli
>>> <Honnappa.Nagarahalli@arm.com>; Ola Liljedahl <Ola.Liljedahl@arm.com>;
>>> ferruh.yigit@intel.com
>>> Subject: Re: [dpdk-dev] [PATCH v3 2/4] kni: fix kni fifo
>>> synchronization
>>>
>>> On Mon,  8 Oct 2018 17:11:44 +0800
>>> Phil Yang <phil.yang@arm.com> wrote:
>>>
>>>> diff --git a/lib/librte_kni/rte_kni_fifo.h
>>>> b/lib/librte_kni/rte_kni_fifo.h index ac26a8c..70ac14e 100644
>>>> --- a/lib/librte_kni/rte_kni_fifo.h
>>>> +++ b/lib/librte_kni/rte_kni_fifo.h
>>>> @@ -28,8 +28,9 @@ kni_fifo_put(struct rte_kni_fifo *fifo, void
>>>> **data, unsigned num)  {
>>>>  unsigned i = 0;
>>>>  unsigned fifo_write = fifo->write;
>>>> -unsigned fifo_read = fifo->read;
>>>>  unsigned new_write = fifo_write;
>>>> +rte_smp_rmb();
>>>> +unsigned fifo_read = fifo->read;
>>>>
>>>
>>> The patch makes sense, but this function should be changed to match
>>> kernel code style.
>>> That means no declarations after initial block, and use 'unsigned int'
>>> rather than 'unsigned'
>>>
>>> Also. why is i initialized? Best practice now is to not do gratitious
>>> initialization since it defeats compiler checks for accidental  use of
>> uninitialized variables.
>>>
>>> What makes sense is something like:
>>>
>>> kni_fifo_put(struct rte_kni_fifo *fifo, void **data, unsigned num) {
>>> unsigned int i, fifo_read, fifo_write, new_write;
>>>
>>> fifo_write = fifo->write;
>>> new_write = fifo_write;
>>> rte_smb_rmb();
>>> fifo_read = fifo->read;
>>>
>>> Sorry, blaming you for issues which are inherited from original KNI code.
>>> Maybe someone should run kernel checkpatch (not DPDK checkpatch) on it
>>> and fix those.
>>
>> Thanks for your comment.
>>
>> I think I can submit a new separate patch to fix this historical issue.
>>
>> Thanks,
>> Phil Yang
> 
> I advised a separate patch to make this patch to the point and clean.

+1 to separate patch for clean up.
diff mbox series

Patch

diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
index ac26a8c..70ac14e 100644
--- a/lib/librte_kni/rte_kni_fifo.h
+++ b/lib/librte_kni/rte_kni_fifo.h
@@ -28,8 +28,9 @@  kni_fifo_put(struct rte_kni_fifo *fifo, void **data, unsigned num)
 {
 	unsigned i = 0;
 	unsigned fifo_write = fifo->write;
-	unsigned fifo_read = fifo->read;
 	unsigned new_write = fifo_write;
+	rte_smp_rmb();
+	unsigned fifo_read = fifo->read;
 
 	for (i = 0; i < num; i++) {
 		new_write = (new_write + 1) & (fifo->len - 1);
@@ -39,6 +40,7 @@  kni_fifo_put(struct rte_kni_fifo *fifo, void **data, unsigned num)
 		fifo->buffer[fifo_write] = data[i];
 		fifo_write = new_write;
 	}
+	rte_smp_wmb();
 	fifo->write = fifo_write;
 	return i;
 }
@@ -51,7 +53,9 @@  kni_fifo_get(struct rte_kni_fifo *fifo, void **data, unsigned num)
 {
 	unsigned i = 0;
 	unsigned new_read = fifo->read;
+	rte_smp_rmb();
 	unsigned fifo_write = fifo->write;
+
 	for (i = 0; i < num; i++) {
 		if (new_read == fifo_write)
 			break;
@@ -59,6 +63,7 @@  kni_fifo_get(struct rte_kni_fifo *fifo, void **data, unsigned num)
 		data[i] = fifo->buffer[new_read];
 		new_read = (new_read + 1) & (fifo->len - 1);
 	}
+	rte_smp_rmb();
 	fifo->read = new_read;
 	return i;
 }
@@ -69,5 +74,8 @@  kni_fifo_get(struct rte_kni_fifo *fifo, void **data, unsigned num)
 static inline uint32_t
 kni_fifo_count(struct rte_kni_fifo *fifo)
 {
-	return (fifo->len + fifo->write - fifo->read) & (fifo->len - 1);
+	unsigned fifo_write = fifo->write;
+	rte_smp_rmb();
+	unsigned fifo_read = fifo->read;
+	return (fifo->len + fifo_write - fifo_read) & (fifo->len - 1);
 }