| Message ID | 20220620131044.1858049-1-david.marchand@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Maxime Coquelin |
| Headers | show |
| Series | vdpa/mlx5: fix leak on event thread creation | expand |
| Context | Check | Description |
|---|---|---|
| ci/intel-Testing | success | Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
| ci/iol-intel-Functional | success | Functional Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/github-robot: build | success | github build: passed |
| ci/iol-x86_64-compile-testing | success | Testing PASS |
| ci/iol-x86_64-unit-testing | success | Testing PASS |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
| ci/iol-aarch64-unit-testing | success | Testing PASS |
| ci/iol-aarch64-compile-testing | success | Testing PASS |
| ci/checkpatch | success | coding style OK |
On Mon, Jun 20, 2022 at 3:11 PM David Marchand <david.marchand@redhat.com> wrote: > > As stated in the manual, pthread_attr_init return value should be > checked. > Besides, a pthread_attr_t should be destroyed once unused. > > In practice, we may have no leak (from what I read in glibc current code), > but this may change in the future. > Stick to a correct use of the API. > > Fixes: 5cf3fd3af4df ("vdpa/mlx5: add CPU core parameter to bind polling thread") > Cc: stable@dpdk.org > > Signed-off-by: David Marchand <david.marchand@redhat.com> Review, please?
On 7/1/22 10:30, David Marchand wrote: > On Mon, Jun 20, 2022 at 3:11 PM David Marchand > <david.marchand@redhat.com> wrote: >> >> As stated in the manual, pthread_attr_init return value should be >> checked. >> Besides, a pthread_attr_t should be destroyed once unused. >> >> In practice, we may have no leak (from what I read in glibc current code), >> but this may change in the future. >> Stick to a correct use of the API. >> >> Fixes: 5cf3fd3af4df ("vdpa/mlx5: add CPU core parameter to bind polling thread") >> Cc: stable@dpdk.org >> >> Signed-off-by: David Marchand <david.marchand@redhat.com> > > Review, please? > > I reviewed the patch but was waiting for Nvidia to test it. Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> Nvidia, could it be done ASAP so that it goes to -rc3? Thanks, Maxime
From: David Marchand > As stated in the manual, pthread_attr_init return value should be checked. > Besides, a pthread_attr_t should be destroyed once unused. > > In practice, we may have no leak (from what I read in glibc current code), but > this may change in the future. > Stick to a correct use of the API. > > Fixes: 5cf3fd3af4df ("vdpa/mlx5: add CPU core parameter to bind polling > thread") > Cc: stable@dpdk.org > > Signed-off-by: David Marchand <david.marchand@redhat.com> Acked-by: Matan Azrad <matan@nvidia.com> Thanks David!
diff --git a/drivers/vdpa/mlx5/mlx5_vdpa_event.c b/drivers/vdpa/mlx5/mlx5_vdpa_event.c index 7167a98db0..f40a68d187 100644 --- a/drivers/vdpa/mlx5/mlx5_vdpa_event.c +++ b/drivers/vdpa/mlx5/mlx5_vdpa_event.c @@ -467,6 +467,7 @@ mlx5_vdpa_cqe_event_setup(struct mlx5_vdpa_priv *priv) { int ret; rte_cpuset_t cpuset; + pthread_attr_t *attrp = NULL; pthread_attr_t attr; char name[16]; const struct sched_param sp = { @@ -476,22 +477,27 @@ mlx5_vdpa_cqe_event_setup(struct mlx5_vdpa_priv *priv) if (!priv->eventc) /* All virtqs are in poll mode. */ return 0; - pthread_attr_init(&attr); - ret = pthread_attr_setschedpolicy(&attr, SCHED_RR); + ret = pthread_attr_init(&attr); + if (ret != 0) { + DRV_LOG(ERR, "Failed to initialize thread attributes"); + goto out; + } + attrp = &attr; + ret = pthread_attr_setschedpolicy(attrp, SCHED_RR); if (ret) { DRV_LOG(ERR, "Failed to set thread sched policy = RR."); - return -1; + goto out; } - ret = pthread_attr_setschedparam(&attr, &sp); + ret = pthread_attr_setschedparam(attrp, &sp); if (ret) { DRV_LOG(ERR, "Failed to set thread priority."); - return -1; + goto out; } - ret = pthread_create(&priv->timer_tid, &attr, mlx5_vdpa_event_handle, + ret = pthread_create(&priv->timer_tid, attrp, mlx5_vdpa_event_handle, (void *)priv); if (ret) { DRV_LOG(ERR, "Failed to create timer thread."); - return -1; + goto out; } CPU_ZERO(&cpuset); if (priv->event_core != -1) @@ -501,12 +507,16 @@ mlx5_vdpa_cqe_event_setup(struct mlx5_vdpa_priv *priv) ret = pthread_setaffinity_np(priv->timer_tid, sizeof(cpuset), &cpuset); if (ret) { DRV_LOG(ERR, "Failed to set thread affinity."); - return -1; + goto out; } snprintf(name, sizeof(name), "vDPA-mlx5-%d", priv->vid); - ret = rte_thread_setname(priv->timer_tid, name); - if (ret) + if (rte_thread_setname(priv->timer_tid, name) != 0) DRV_LOG(DEBUG, "Cannot set timer thread name."); +out: + if (attrp != NULL) + pthread_attr_destroy(attrp); + if (ret != 0) + return -1; return 0; }
As stated in the manual, pthread_attr_init return value should be checked. Besides, a pthread_attr_t should be destroyed once unused. In practice, we may have no leak (from what I read in glibc current code), but this may change in the future. Stick to a correct use of the API. Fixes: 5cf3fd3af4df ("vdpa/mlx5: add CPU core parameter to bind polling thread") Cc: stable@dpdk.org Signed-off-by: David Marchand <david.marchand@redhat.com> --- Note: this is only compile tested. --- drivers/vdpa/mlx5/mlx5_vdpa_event.c | 30 +++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)