diff mbox series

[v3] rte_ethdev: safer memory access by calling Rx/Tx callback

Message ID 20200305164704.29900-1-tgw_team@tencent.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v3] rte_ethdev: safer memory access by calling Rx/Tx callback |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

ZY Qiu March 5, 2020, 4:47 p.m. UTC 
  When compiling with -O0,
the compiler does not optimize two memory accesses into one.
Leads to accessing a null pointer when queue post Rx burst callback
removal while traffic is running.

Signed-off-by: ZY Qiu <tgw_team@tencent.com>
---
 lib/librte_ethdev/rte_ethdev.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Jerin Jacob March 5, 2020, 5:23 p.m. UTC | #1 
On Thu, Mar 5, 2020 at 10:17 PM ZY Qiu <quzeyao@gmail.com> wrote:
>
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when queue post Rx burst callback
> removal while traffic is running.
>
> Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..c46612e3e 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>                                      rx_pkts, nb_pkts);
>
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -       if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> -               struct rte_eth_rxtx_callback *cb =
> -                               dev->post_rx_burst_cbs[queue_id];
> +       struct rte_eth_rxtx_callback *volatile cb =
> +                       dev->post_rx_burst_cbs[queue_id];

Is adding rte_compiler_barrier() here without changing to volatile
solving the problem in your case?
If so, I prefer to change to compiler_barrier() as volatile may have a
performance impact on fast-path code.

>
> +       if (unlikely(cb != NULL)) {
>                 do {
>                         nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>                                                 nb_pkts, cb->param);
> @@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
>  #endif
>
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -       struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
> +       struct rte_eth_rxtx_callback *volatile cb =
> +                       dev->pre_tx_burst_cbs[queue_id];
>
>         if (unlikely(cb != NULL)) {
>                 do {
> --
> 2.17.1
>
Ananyev, Konstantin March 11, 2020, 12:22 p.m. UTC | #2 
> -----Original Message-----
> From: ZY Qiu <quzeyao@gmail.com>
> Sent: Thursday, March 5, 2020 4:47 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; ZY Qiu
> <tgw_team@tencent.com>
> Subject: [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
> 
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when queue post Rx burst callback
> removal while traffic is running.
> 
> Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..c46612e3e 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>  				     rx_pkts, nb_pkts);
> 
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> -		struct rte_eth_rxtx_callback *cb =
> -				dev->post_rx_burst_cbs[queue_id];
> +	struct rte_eth_rxtx_callback *volatile cb =
> +			dev->post_rx_burst_cbs[queue_id];
> 
> +	if (unlikely(cb != NULL)) {
>  		do {
>  			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>  						nb_pkts, cb->param);
> @@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
>  #endif
> 
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -	struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
> +	struct rte_eth_rxtx_callback *volatile cb =
> +			dev->pre_tx_burst_cbs[queue_id];
> 
>  	if (unlikely(cb != NULL)) {
>  		do {
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1
Jerin Jacob March 11, 2020, 12:26 p.m. UTC | #3 
On Wed, Mar 11, 2020 at 5:52 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: ZY Qiu <quzeyao@gmail.com>
> > Sent: Thursday, March 5, 2020 4:47 PM
> > To: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
> > Cc: dev@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; ZY Qiu
> > <tgw_team@tencent.com>
> > Subject: [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
> >
> > When compiling with -O0,
> > the compiler does not optimize two memory accesses into one.
> > Leads to accessing a null pointer when queue post Rx burst callback
> > removal while traffic is running.
> >
> > Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> > ---
> >  lib/librte_ethdev/rte_ethdev.h | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index d1a593ad1..c46612e3e 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> >                                    rx_pkts, nb_pkts);
> >
> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > -     if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> > -             struct rte_eth_rxtx_callback *cb =
> > -                             dev->post_rx_burst_cbs[queue_id];
> > +     struct rte_eth_rxtx_callback *volatile cb =
> > +                     dev->post_rx_burst_cbs[queue_id];

I prefer to change to compiler_barrier() if it working as volatile may have a
performance impact on fast-path code.


> >
> > +     if (unlikely(cb != NULL)) {
> >               do {
> >                       nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> >                                               nb_pkts, cb->param);
> > @@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
> >  #endif
> >
> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > -     struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
> > +     struct rte_eth_rxtx_callback *volatile cb =
> > +                     dev->pre_tx_burst_cbs[queue_id];
> >
> >       if (unlikely(cb != NULL)) {
> >               do {
> > --
>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
> > 2.17.1
>
Ferruh Yigit Oct. 1, 2020, 3:14 p.m. UTC | #4 
On 3/11/2020 12:26 PM, Jerin Jacob wrote:
> On Wed, Mar 11, 2020 at 5:52 PM Ananyev, Konstantin
> <konstantin.ananyev@intel.com> wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: ZY Qiu <quzeyao@gmail.com>
>>> Sent: Thursday, March 5, 2020 4:47 PM
>>> To: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
>>> Cc: dev@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; ZY Qiu
>>> <tgw_team@tencent.com>
>>> Subject: [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
>>>
>>> When compiling with -O0,
>>> the compiler does not optimize two memory accesses into one.
>>> Leads to accessing a null pointer when queue post Rx burst callback
>>> removal while traffic is running.
>>>
>>> Signed-off-by: ZY Qiu <tgw_team@tencent.com>
>>> ---
>>>   lib/librte_ethdev/rte_ethdev.h | 9 +++++----
>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index d1a593ad1..c46612e3e 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>>>                                     rx_pkts, nb_pkts);
>>>
>>>   #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>>> -     if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
>>> -             struct rte_eth_rxtx_callback *cb =
>>> -                             dev->post_rx_burst_cbs[queue_id];
>>> +     struct rte_eth_rxtx_callback *volatile cb =
>>> +                     dev->post_rx_burst_cbs[queue_id];
> 
> I prefer to change to compiler_barrier() if it working as volatile may have a
> performance impact on fast-path code.
> 

Hi ZY Qiu,

Did you able to test with compiler barrier, is it solving your problem?

If there is no update on the issue, I will mark it as rejected.

Thanks,
ferruh


> 
>>>
>>> +     if (unlikely(cb != NULL)) {
>>>                do {
>>>                        nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>>>                                                nb_pkts, cb->param);
>>> @@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
>>>   #endif
>>>
>>>   #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>>> -     struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
>>> +     struct rte_eth_rxtx_callback *volatile cb =
>>> +                     dev->pre_tx_burst_cbs[queue_id];
>>>
>>>        if (unlikely(cb != NULL)) {
>>>                do {
>>> --
>>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>
>>> 2.17.1
>>
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593ad1..c46612e3e 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4388,10 +4388,10 @@  rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 				     rx_pkts, nb_pkts);
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
-		struct rte_eth_rxtx_callback *cb =
-				dev->post_rx_burst_cbs[queue_id];
+	struct rte_eth_rxtx_callback *volatile cb =
+			dev->post_rx_burst_cbs[queue_id];
 
+	if (unlikely(cb != NULL)) {
 		do {
 			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
 						nb_pkts, cb->param);
@@ -4652,7 +4652,8 @@  rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
 #endif
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
+	struct rte_eth_rxtx_callback *volatile cb =
+			dev->pre_tx_burst_cbs[queue_id];
 
 	if (unlikely(cb != NULL)) {
 		do {