diff mbox

[dpdk-dev] net/mlx5: only register memory callback when probing a device

Message ID 20180530144253.8935-1-david.marchand@6wind.com (mailing list archive)
State Accepted, archived
Headers

Checks

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

Commit Message

David Marchand May 30, 2018, 2:42 p.m. UTC 
  The callback should be invoked only for memory that has been registered
in a device, hence, no need to track cleanup events if no device is
present.

Bugzilla-ID: 56
Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support")

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 drivers/net/mlx5/mlx5.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Yongseok Koh May 30, 2018, 3:59 p.m. UTC | #1 
> On May 30, 2018, at 7:42 AM, David Marchand <david.marchand@6wind.com> wrote:
> 
> The callback should be invoked only for memory that has been registered
> in a device, hence, no need to track cleanup events if no device is
> present.
> 
> Bugzilla-ID: 56
> Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support")
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---

Good finding! Thank you so much for the patch.
Patch is good. Just to be sure, I've run traffic with testpmd and also run
multi-process test examples with traffic. It runs well.

Acked-by: Yongseok Koh <yskoh@mellanox.com>
 
Thanks

> drivers/net/mlx5/mlx5.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index d77a20db4..601d03f71 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -138,6 +138,8 @@ mlx5_prepare_shared_data(void)
> 			LIST_INIT(&mlx5_shared_data->mem_event_cb_list);
> 			rte_rwlock_init(&mlx5_shared_data->mem_event_rwlock);
> 		}
> +		rte_mem_event_callback_register("MLX5_MEM_EVENT_CB",
> +						mlx5_mr_mem_event_cb, NULL);
> 	}
> 	rte_spinlock_unlock(&mlx5_shared_data_lock);
> }
> @@ -1460,8 +1462,6 @@ rte_mlx5_pmd_init(void)
> 	}
> 	mlx5_glue->fork_init();
> 	rte_pci_register(&mlx5_driver);
> -	rte_mem_event_callback_register("MLX5_MEM_EVENT_CB",
> -					mlx5_mr_mem_event_cb, NULL);
> }
> 
> RTE_PMD_EXPORT_NAME(net_mlx5, __COUNTER__);
> -- 
> 2.11.0
>
Ferruh Yigit May 30, 2018, 4:02 p.m. UTC | #2 
On 5/30/2018 3:42 PM, David Marchand wrote:
> The callback should be invoked only for memory that has been registered
> in a device, hence, no need to track cleanup events if no device is
> present.
> 
> Bugzilla-ID: 56
> Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support")
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>

Tested-by: Ferruh Yigit <ferruh.yigit@intel.com>


(Reproduced using the suggested tespmd update in Bug 56, verified this patch
fixes it.)
David Marchand May 30, 2018, 4:04 p.m. UTC | #3 
On Wed, May 30, 2018 at 5:59 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
>> On May 30, 2018, at 7:42 AM, David Marchand <david.marchand@6wind.com> wrote:
>>
>> The callback should be invoked only for memory that has been registered
>> in a device, hence, no need to track cleanup events if no device is
>> present.
>>
>> Bugzilla-ID: 56
>> Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support")
>>
>> Signed-off-by: David Marchand <david.marchand@6wind.com>
>> ---
>
> Good finding! Thank you so much for the patch.
> Patch is good. Just to be sure, I've run traffic with testpmd and also run
> multi-process test examples with traffic. It runs well.
>
> Acked-by: Yongseok Koh <yskoh@mellanox.com>

Well, this is a quick fix, the mlx5_prepare_shared_data() now does
more than initialising the shared data...
Might be better to revisit later, but this should do the job for 18.05.

Thanks.
Thomas Monjalon May 30, 2018, 7:07 p.m. UTC | #4 
30/05/2018 18:04, David Marchand:
> On Wed, May 30, 2018 at 5:59 PM, Yongseok Koh <yskoh@mellanox.com> wrote:
> >> On May 30, 2018, at 7:42 AM, David Marchand <david.marchand@6wind.com> wrote:
> >>
> >> The callback should be invoked only for memory that has been registered
> >> in a device, hence, no need to track cleanup events if no device is
> >> present.
> >>
> >> Bugzilla-ID: 56
> >> Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support")
> >>
> >> Signed-off-by: David Marchand <david.marchand@6wind.com>
> >> ---
> >
> > Good finding! Thank you so much for the patch.
> > Patch is good. Just to be sure, I've run traffic with testpmd and also run
> > multi-process test examples with traffic. It runs well.
> >
> > Acked-by: Yongseok Koh <yskoh@mellanox.com>
> 
> Well, this is a quick fix, the mlx5_prepare_shared_data() now does
> more than initialising the shared data...
> Might be better to revisit later, but this should do the job for 18.05.

Applied, thanks
diff mbox

Patch

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index d77a20db4..601d03f71 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -138,6 +138,8 @@  mlx5_prepare_shared_data(void)
 			LIST_INIT(&mlx5_shared_data->mem_event_cb_list);
 			rte_rwlock_init(&mlx5_shared_data->mem_event_rwlock);
 		}
+		rte_mem_event_callback_register("MLX5_MEM_EVENT_CB",
+						mlx5_mr_mem_event_cb, NULL);
 	}
 	rte_spinlock_unlock(&mlx5_shared_data_lock);
 }
@@ -1460,8 +1462,6 @@  rte_mlx5_pmd_init(void)
 	}
 	mlx5_glue->fork_init();
 	rte_pci_register(&mlx5_driver);
-	rte_mem_event_callback_register("MLX5_MEM_EVENT_CB",
-					mlx5_mr_mem_event_cb, NULL);
 }
 
 RTE_PMD_EXPORT_NAME(net_mlx5, __COUNTER__);