diff mbox series

kni: fix kni rx fifo producer synchronization

Message ID 1533810233-7706-1-git-send-email-kkokkilagadda@caviumnetworks.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series kni: fix kni rx fifo producer synchronization |

Checks

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

Commit Message

Kiran Kumar Aug. 9, 2018, 10:23 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. 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 <kkokkilagadda@caviumnetworks.com>
---
 lib/librte_kni/rte_kni_fifo.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Ferruh Yigit Aug. 9, 2018, 11:30 a.m. UTC | #1 
On 8/9/2018 11:23 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 <kkokkilagadda@caviumnetworks.com>
> ---
>  lib/librte_kni/rte_kni_fifo.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
> index ac26a8c..4d6b33e 100644
> --- a/lib/librte_kni/rte_kni_fifo.h
> +++ b/lib/librte_kni/rte_kni_fifo.h
> @@ -39,6 +39,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;

For Intel this is just a compiler barrier so no issue but not sure if a memory
barrier is required here,

Related code block is:

|-          for (i = 0; i < num; i++) {
||                  new_write = (new_write + 1) & (fifo->len - 1);
||
||                  if (new_write == fifo_read)
||                          break;
||                  fifo->buffer[fifo_write] = data[i];
||                  fifo_write = new_write;
||          }
|           fifo->write = fifo_write;

"fifo_write" is updated in the loop, so there is a dependency to it for
"fifo->write". Can memory writes be reordered when there is a dependency?

Cc'ed a few more people for comment.
Jerin Jacob Aug. 16, 2018, 6:52 a.m. UTC | #2 
-----Original Message-----
> Date: Thu, 9 Aug 2018 12:30:57 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Kiran Kumar <kkokkilagadda@caviumnetworks.com>
> CC: dev@dpdk.org, Jerin Jacob <jerin.jacob@caviumnetworks.com>, Konstantin
>  Ananyev <konstantin.ananyev@intel.com>, Bruce Richardson
>  <bruce.richardson@intel.com>, Santosh Shukla
>  <santosh.shukla@caviumnetworks.com>
> Subject: Re: [dpdk-dev] [PATCH] kni: fix kni rx fifo producer
>  synchronization
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.9.1
> 
> 
> On 8/9/2018 11:23 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 <kkokkilagadda@caviumnetworks.com>
> > ---
> >  lib/librte_kni/rte_kni_fifo.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
> > index ac26a8c..4d6b33e 100644
> > --- a/lib/librte_kni/rte_kni_fifo.h
> > +++ b/lib/librte_kni/rte_kni_fifo.h
> > @@ -39,6 +39,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;
> 
> For Intel this is just a compiler barrier so no issue but not sure if a memory
> barrier is required here,
> 
> Related code block is:
> 
> |-          for (i = 0; i < num; i++) {
> ||                  new_write = (new_write + 1) & (fifo->len - 1);
> ||
> ||                  if (new_write == fifo_read)
> ||                          break;
> ||                  fifo->buffer[fifo_write] = data[i];
> ||                  fifo_write = new_write;
> ||          }
> |           fifo->write = fifo_write;
> 
> "fifo_write" is updated in the loop, so there is a dependency to it for
> "fifo->write". Can memory writes be reordered when there is a dependency?

From CPU0 compiler instruction generation perspective, It will take care of the
dependency. In weakly ordered machine, it does NOT grandees CPU1 sees
fifo->write as final write.

So, IMO, This patch make sense. We are able to reproduce the problem on
arm64 machine with certain traffic.

> 
> Cc'ed a few more people for comment.
Jerin Jacob Aug. 16, 2018, 9:01 a.m. UTC | #3 
-----Original Message-----
> Date: Thu,  9 Aug 2018 15:53:53 +0530
> From: Kiran Kumar <kkokkilagadda@caviumnetworks.com>
> To: ferruh.yigit@intel.com
> CC: dev@dpdk.org, Kiran Kumar <kkokkilagadda@caviumnetworks.com>
> Subject: [dpdk-dev]  [PATCH] kni: fix kni rx fifo producer synchronization
> X-Mailer: git-send-email 2.7.4
> 
> External Email
> 
> 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")
> 

Please fix following check patch issue.
Wrong headline lowercase:
	kni: fix kni rx fifo producer synchronization

and Cc stable@dpdk.org


With above change:

Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>


> Signed-off-by: Kiran Kumar <kkokkilagadda@caviumnetworks.com>
> ---
>  lib/librte_kni/rte_kni_fifo.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
> index ac26a8c..4d6b33e 100644
> --- a/lib/librte_kni/rte_kni_fifo.h
> +++ b/lib/librte_kni/rte_kni_fifo.h
> @@ -39,6 +39,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;
>  }
> --
> 2.7.4
>
Ferruh Yigit Aug. 16, 2018, 12:28 p.m. UTC | #4 
On 8/16/2018 7:52 AM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 9 Aug 2018 12:30:57 +0100
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Kiran Kumar <kkokkilagadda@caviumnetworks.com>
>> CC: dev@dpdk.org, Jerin Jacob <jerin.jacob@caviumnetworks.com>, Konstantin
>>  Ananyev <konstantin.ananyev@intel.com>, Bruce Richardson
>>  <bruce.richardson@intel.com>, Santosh Shukla
>>  <santosh.shukla@caviumnetworks.com>
>> Subject: Re: [dpdk-dev] [PATCH] kni: fix kni rx fifo producer
>>  synchronization
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.9.1
>>
>>
>> On 8/9/2018 11:23 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 <kkokkilagadda@caviumnetworks.com>
>>> ---
>>>  lib/librte_kni/rte_kni_fifo.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
>>> index ac26a8c..4d6b33e 100644
>>> --- a/lib/librte_kni/rte_kni_fifo.h
>>> +++ b/lib/librte_kni/rte_kni_fifo.h
>>> @@ -39,6 +39,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;
>>
>> For Intel this is just a compiler barrier so no issue but not sure if a memory
>> barrier is required here,
>>
>> Related code block is:
>>
>> |-          for (i = 0; i < num; i++) {
>> ||                  new_write = (new_write + 1) & (fifo->len - 1);
>> ||
>> ||                  if (new_write == fifo_read)
>> ||                          break;
>> ||                  fifo->buffer[fifo_write] = data[i];
>> ||                  fifo_write = new_write;
>> ||          }
>> |           fifo->write = fifo_write;
>>
>> "fifo_write" is updated in the loop, so there is a dependency to it for
>> "fifo->write". Can memory writes be reordered when there is a dependency?
> 
> From CPU0 compiler instruction generation perspective, It will take care of the
> dependency. In weakly ordered machine, it does NOT grandees CPU1 sees
> fifo->write as final write.
> 
> So, IMO, This patch make sense. We are able to reproduce the problem on
> arm64 machine with certain traffic.

OK, thanks for clarification.

> 
>>
>> Cc'ed a few more people for comment.
diff mbox series

Patch

diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
index ac26a8c..4d6b33e 100644
--- a/lib/librte_kni/rte_kni_fifo.h
+++ b/lib/librte_kni/rte_kni_fifo.h
@@ -39,6 +39,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;
 }