| Message ID | 1555083927-24499-1-git-send-email-viacheslavo@mellanox.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Shahaf Shuler |
| Headers | show |
| Series | [1/1] net/mlx5: fix memory region cleanup routine | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/intel-Performance-Testing | success | Performance Testing PASS |
| ci/mellanox-Performance-Testing | success | Performance Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
> On Apr 12, 2019, at 8:45 AM, Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote: > > mlx5 driver has a global list of Memory Regions created by > device, and there is a ml5_mr_release() routine which makes > a memory cleanup at device closing. The head of device MR list > was fetched outside the rwlock protected section. Also some > noticed typos are fixed. > > Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support") > Cc: stable@dpdk.org > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> > --- Nice catch, Slava Can you please submit the same fix for mlx4? Acked-by: Yongseok Koh <yskoh@mellanox.com> Thanks > drivers/net/mlx5/mlx5_mr.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c > index 44b6591..a3732d4 100644 > --- a/drivers/net/mlx5/mlx5_mr.c > +++ b/drivers/net/mlx5/mlx5_mr.c > @@ -466,7 +466,7 @@ struct mr_update_mp_data { > } > > /** > - * Releass resources of detached MR having no online entry. > + * Release resources of detached MR having no online entry. > * > * @param dev > * Pointer to Ethernet device. > @@ -516,7 +516,7 @@ struct mr_update_mp_data { > } > > /** > - * Create a new global Memroy Region (MR) for a missing virtual address. > + * Create a new global Memory Region (MR) for a missing virtual address. > * This API should be called on a secondary process, then a request is sent to > * the primary process in order to create a MR for the address. As the global MR > * list is on the shared memory, following LKey lookup should succeed unless the > @@ -562,7 +562,7 @@ struct mr_update_mp_data { > } > > /** > - * Create a new global Memroy Region (MR) for a missing virtual address. > + * Create a new global Memory Region (MR) for a missing virtual address. > * Register entire virtually contiguous memory chunk around the address. > * This must be called from the primary process. > * > @@ -673,7 +673,7 @@ struct mr_update_mp_data { > bmp_mem = RTE_PTR_ALIGN_CEIL(mr + 1, RTE_CACHE_LINE_SIZE); > mr->ms_bmp = rte_bitmap_init(ms_n, bmp_mem, bmp_size); > if (mr->ms_bmp == NULL) { > - DEBUG("port %u unable to initialize bitamp for a new MR of" > + DEBUG("port %u unable to initialize bitmap for a new MR of" > " address (%p).", > dev->data->port_id, (void *)addr); > rte_errno = EINVAL; > @@ -811,7 +811,7 @@ struct mr_update_mp_data { > } > > /** > - * Create a new global Memroy Region (MR) for a missing virtual address. > + * Create a new global Memory Region (MR) for a missing virtual address. > * This can be called from primary and secondary process. > * > * @param dev > @@ -1600,7 +1600,7 @@ struct mr_update_mp_data { > mlx5_mr_release(struct rte_eth_dev *dev) > { > struct mlx5_priv *priv = dev->data->dev_private; > - struct mlx5_mr *mr_next = LIST_FIRST(&priv->mr.mr_list); > + struct mlx5_mr *mr_next; > > /* Remove from memory callback device list. */ > rte_rwlock_write_lock(&mlx5_shared_data->mem_event_rwlock); > @@ -1610,6 +1610,7 @@ struct mr_update_mp_data { > mlx5_mr_dump_dev(dev); > rte_rwlock_write_lock(&priv->mr.rwlock); > /* Detach from MR list and move to free list. */ > + mr_next = LIST_FIRST(&priv->mr.mr_list); > while (mr_next != NULL) { > struct mlx5_mr *mr = mr_next; > > -- > 1.8.3.1 >
Friday, April 12, 2019 8:55 PM, Yongseok Koh: > Subject: Re: [dpdk-stable] [PATCH 1/1] net/mlx5: fix memory region cleanup > routine > > > > On Apr 12, 2019, at 8:45 AM, Viacheslav Ovsiienko > <viacheslavo@mellanox.com> wrote: > > > > mlx5 driver has a global list of Memory Regions created by device, and > > there is a ml5_mr_release() routine which makes a memory cleanup at > > device closing. The head of device MR list was fetched outside the > > rwlock protected section. Also some noticed typos are fixed. > > > > Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support") > > Cc: stable@dpdk.org > > > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> > > --- > > Nice catch, Slava > Can you please submit the same fix for mlx4? > > Acked-by: Yongseok Koh <yskoh@mellanox.com> Slava - please merge both mlx4 and mlx5 into the same patch. Keep Koh's acked-by. I will take the v2. > > Thanks > > > drivers/net/mlx5/mlx5_mr.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c > > index 44b6591..a3732d4 100644 > > --- a/drivers/net/mlx5/mlx5_mr.c > > +++ b/drivers/net/mlx5/mlx5_mr.c > > @@ -466,7 +466,7 @@ struct mr_update_mp_data { } > > > > /** > > - * Releass resources of detached MR having no online entry. > > + * Release resources of detached MR having no online entry. > > * > > * @param dev > > * Pointer to Ethernet device. > > @@ -516,7 +516,7 @@ struct mr_update_mp_data { } > > > > /** > > - * Create a new global Memroy Region (MR) for a missing virtual address. > > + * Create a new global Memory Region (MR) for a missing virtual address. > > * This API should be called on a secondary process, then a request is > > sent to > > * the primary process in order to create a MR for the address. As the > > global MR > > * list is on the shared memory, following LKey lookup should succeed > > unless the @@ -562,7 +562,7 @@ struct mr_update_mp_data { } > > > > /** > > - * Create a new global Memroy Region (MR) for a missing virtual address. > > + * Create a new global Memory Region (MR) for a missing virtual address. > > * Register entire virtually contiguous memory chunk around the address. > > * This must be called from the primary process. > > * > > @@ -673,7 +673,7 @@ struct mr_update_mp_data { > > bmp_mem = RTE_PTR_ALIGN_CEIL(mr + 1, RTE_CACHE_LINE_SIZE); > > mr->ms_bmp = rte_bitmap_init(ms_n, bmp_mem, bmp_size); > > if (mr->ms_bmp == NULL) { > > - DEBUG("port %u unable to initialize bitamp for a new MR of" > > + DEBUG("port %u unable to initialize bitmap for a new MR of" > > " address (%p).", > > dev->data->port_id, (void *)addr); > > rte_errno = EINVAL; > > @@ -811,7 +811,7 @@ struct mr_update_mp_data { } > > > > /** > > - * Create a new global Memroy Region (MR) for a missing virtual address. > > + * Create a new global Memory Region (MR) for a missing virtual address. > > * This can be called from primary and secondary process. > > * > > * @param dev > > @@ -1600,7 +1600,7 @@ struct mr_update_mp_data { > > mlx5_mr_release(struct rte_eth_dev *dev) { > > struct mlx5_priv *priv = dev->data->dev_private; > > - struct mlx5_mr *mr_next = LIST_FIRST(&priv->mr.mr_list); > > + struct mlx5_mr *mr_next; > > > > /* Remove from memory callback device list. */ > > rte_rwlock_write_lock(&mlx5_shared_data->mem_event_rwlock); > > @@ -1610,6 +1610,7 @@ struct mr_update_mp_data { > > mlx5_mr_dump_dev(dev); > > rte_rwlock_write_lock(&priv->mr.rwlock); > > /* Detach from MR list and move to free list. */ > > + mr_next = LIST_FIRST(&priv->mr.mr_list); > > while (mr_next != NULL) { > > struct mlx5_mr *mr = mr_next; > > > > -- > > 1.8.3.1 > >
> -----Original Message----- > From: Shahaf Shuler > Sent: Sunday, April 14, 2019 9:58 > To: Yongseok Koh <yskoh@mellanox.com>; Slava Ovsiienko > <viacheslavo@mellanox.com> > Cc: dev <dev@dpdk.org>; stable@dpdk.org > Subject: RE: [dpdk-stable] [PATCH 1/1] net/mlx5: fix memory region cleanup > routine > > Friday, April 12, 2019 8:55 PM, Yongseok Koh: > > Subject: Re: [dpdk-stable] [PATCH 1/1] net/mlx5: fix memory region > > cleanup routine > > > > > > > On Apr 12, 2019, at 8:45 AM, Viacheslav Ovsiienko > > <viacheslavo@mellanox.com> wrote: > > > > > > mlx5 driver has a global list of Memory Regions created by device, > > > and there is a ml5_mr_release() routine which makes a memory cleanup > > > at device closing. The head of device MR list was fetched outside > > > the rwlock protected section. Also some noticed typos are fixed. > > > > > > Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support") > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> > > > --- > > > > Nice catch, Slava > > Can you please submit the same fix for mlx4? > > > > Acked-by: Yongseok Koh <yskoh@mellanox.com> > > Slava - please merge both mlx4 and mlx5 into the same patch. > Keep Koh's acked-by. > > I will take the v2. There are some troubles with passing check-log script for mlx4/mlx5 merged patch. So, I prepared the separated patch for mlx4 with right headline and fix reference. http://patches.dpdk.org/patch/52791/ If you don't mind - please apply both (for mlx4 and mlx5) patches. Thanks. With best regards, Slava > > > > > Thanks > > > > > drivers/net/mlx5/mlx5_mr.c | 13 +++++++------ > > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c > > > index 44b6591..a3732d4 100644 > > > --- a/drivers/net/mlx5/mlx5_mr.c > > > +++ b/drivers/net/mlx5/mlx5_mr.c > > > @@ -466,7 +466,7 @@ struct mr_update_mp_data { } > > > > > > /** > > > - * Releass resources of detached MR having no online entry. > > > + * Release resources of detached MR having no online entry. > > > * > > > * @param dev > > > * Pointer to Ethernet device. > > > @@ -516,7 +516,7 @@ struct mr_update_mp_data { } > > > > > > /** > > > - * Create a new global Memroy Region (MR) for a missing virtual > address. > > > + * Create a new global Memory Region (MR) for a missing virtual > address. > > > * This API should be called on a secondary process, then a request > > > is sent to > > > * the primary process in order to create a MR for the address. As > > > the global MR > > > * list is on the shared memory, following LKey lookup should > > > succeed unless the @@ -562,7 +562,7 @@ struct mr_update_mp_data { } > > > > > > /** > > > - * Create a new global Memroy Region (MR) for a missing virtual > address. > > > + * Create a new global Memory Region (MR) for a missing virtual > address. > > > * Register entire virtually contiguous memory chunk around the address. > > > * This must be called from the primary process. > > > * > > > @@ -673,7 +673,7 @@ struct mr_update_mp_data { > > > bmp_mem = RTE_PTR_ALIGN_CEIL(mr + 1, RTE_CACHE_LINE_SIZE); > > > mr->ms_bmp = rte_bitmap_init(ms_n, bmp_mem, bmp_size); > > > if (mr->ms_bmp == NULL) { > > > - DEBUG("port %u unable to initialize bitamp for a new MR of" > > > + DEBUG("port %u unable to initialize bitmap for a new MR of" > > > " address (%p).", > > > dev->data->port_id, (void *)addr); > > > rte_errno = EINVAL; > > > @@ -811,7 +811,7 @@ struct mr_update_mp_data { } > > > > > > /** > > > - * Create a new global Memroy Region (MR) for a missing virtual > address. > > > + * Create a new global Memory Region (MR) for a missing virtual > address. > > > * This can be called from primary and secondary process. > > > * > > > * @param dev > > > @@ -1600,7 +1600,7 @@ struct mr_update_mp_data { > > > mlx5_mr_release(struct rte_eth_dev *dev) { > > > struct mlx5_priv *priv = dev->data->dev_private; > > > - struct mlx5_mr *mr_next = LIST_FIRST(&priv->mr.mr_list); > > > + struct mlx5_mr *mr_next; > > > > > > /* Remove from memory callback device list. */ > > > rte_rwlock_write_lock(&mlx5_shared_data->mem_event_rwlock); > > > @@ -1610,6 +1610,7 @@ struct mr_update_mp_data { > > > mlx5_mr_dump_dev(dev); > > > rte_rwlock_write_lock(&priv->mr.rwlock); > > > /* Detach from MR list and move to free list. */ > > > + mr_next = LIST_FIRST(&priv->mr.mr_list); > > > while (mr_next != NULL) { > > > struct mlx5_mr *mr = mr_next; > > > > > > -- > > > 1.8.3.1 > > >
Monday, April 15, 2019 11:46 AM, Slava Ovsiienko: > Subject: RE: [dpdk-stable] [PATCH 1/1] net/mlx5: fix memory region cleanup > routine > [...] > > I will take the v2. > There are some troubles with passing check-log script for mlx4/mlx5 merged > patch. > So, I prepared the separated patch for mlx4 with right headline and fix > reference. > http://patches.dpdk.org/patch/52791/ > If you don't mind - please apply both (for mlx4 and mlx5) patches. Thanks. > Ok, applied to next-net-mlx, thanks.
diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c index 44b6591..a3732d4 100644 --- a/drivers/net/mlx5/mlx5_mr.c +++ b/drivers/net/mlx5/mlx5_mr.c @@ -466,7 +466,7 @@ struct mr_update_mp_data { } /** - * Releass resources of detached MR having no online entry. + * Release resources of detached MR having no online entry. * * @param dev * Pointer to Ethernet device. @@ -516,7 +516,7 @@ struct mr_update_mp_data { } /** - * Create a new global Memroy Region (MR) for a missing virtual address. + * Create a new global Memory Region (MR) for a missing virtual address. * This API should be called on a secondary process, then a request is sent to * the primary process in order to create a MR for the address. As the global MR * list is on the shared memory, following LKey lookup should succeed unless the @@ -562,7 +562,7 @@ struct mr_update_mp_data { } /** - * Create a new global Memroy Region (MR) for a missing virtual address. + * Create a new global Memory Region (MR) for a missing virtual address. * Register entire virtually contiguous memory chunk around the address. * This must be called from the primary process. * @@ -673,7 +673,7 @@ struct mr_update_mp_data { bmp_mem = RTE_PTR_ALIGN_CEIL(mr + 1, RTE_CACHE_LINE_SIZE); mr->ms_bmp = rte_bitmap_init(ms_n, bmp_mem, bmp_size); if (mr->ms_bmp == NULL) { - DEBUG("port %u unable to initialize bitamp for a new MR of" + DEBUG("port %u unable to initialize bitmap for a new MR of" " address (%p).", dev->data->port_id, (void *)addr); rte_errno = EINVAL; @@ -811,7 +811,7 @@ struct mr_update_mp_data { } /** - * Create a new global Memroy Region (MR) for a missing virtual address. + * Create a new global Memory Region (MR) for a missing virtual address. * This can be called from primary and secondary process. * * @param dev @@ -1600,7 +1600,7 @@ struct mr_update_mp_data { mlx5_mr_release(struct rte_eth_dev *dev) { struct mlx5_priv *priv = dev->data->dev_private; - struct mlx5_mr *mr_next = LIST_FIRST(&priv->mr.mr_list); + struct mlx5_mr *mr_next; /* Remove from memory callback device list. */ rte_rwlock_write_lock(&mlx5_shared_data->mem_event_rwlock); @@ -1610,6 +1610,7 @@ struct mr_update_mp_data { mlx5_mr_dump_dev(dev); rte_rwlock_write_lock(&priv->mr.rwlock); /* Detach from MR list and move to free list. */ + mr_next = LIST_FIRST(&priv->mr.mr_list); while (mr_next != NULL) { struct mlx5_mr *mr = mr_next;
mlx5 driver has a global list of Memory Regions created by device, and there is a ml5_mr_release() routine which makes a memory cleanup at device closing. The head of device MR list was fetched outside the rwlock protected section. Also some noticed typos are fixed. Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support") Cc: stable@dpdk.org Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> --- drivers/net/mlx5/mlx5_mr.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)