If a vhost device is closed before eth_dev_configure is done to the device, internal resources allocated to the device would not be freed. This patch fixes it. Fixes: 3d01b759d267 ("net/vhost: delay driver setup") Cc: stable@dpdk.org Signed-off-by: Itsuro Oda <oda@valinux.co.jp> Reviewd-by: Xiaolong Ye <xiaolong.ye@intel.com> --- v2: - fix commit message drivers/net/vhost/rte_eth_vhost.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c index 458ed58f5..1ed977e9b 100644 --- a/drivers/net/vhost/rte_eth_vhost.c +++ b/drivers/net/vhost/rte_eth_vhost.c @@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev) eth_dev_stop(dev); - rte_vhost_driver_unregister(internal->iface_name); - list = find_internal_resource(internal->iface_name); - if (!list) - return; - - pthread_mutex_lock(&internal_list_lock); - TAILQ_REMOVE(&internal_list, list, next); - pthread_mutex_unlock(&internal_list_lock); - rte_free(list); + if (list) { + rte_vhost_driver_unregister(internal->iface_name); + pthread_mutex_lock(&internal_list_lock); + TAILQ_REMOVE(&internal_list, list, next); + pthread_mutex_unlock(&internal_list_lock); + rte_free(list); + } if (dev->data->rx_queues) for (i = 0; i < dev->data->nb_rx_queues; i++) -- 2.17.0
If a vhost device is closed before eth_dev_configure is done to the device, internal resources allocated to the device would not be freed. This patch fixes it. Fixes: 3d01b759d267 ("net/vhost: delay driver setup") Cc: stable@dpdk.org Signed-off-by: Itsuro Oda <oda@valinux.co.jp> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com> --- v2: - fix commit message v3: - fix spell error of Reviewed-by drivers/net/vhost/rte_eth_vhost.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c index 458ed58f5..1ed977e9b 100644 --- a/drivers/net/vhost/rte_eth_vhost.c +++ b/drivers/net/vhost/rte_eth_vhost.c @@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev) eth_dev_stop(dev); - rte_vhost_driver_unregister(internal->iface_name); - list = find_internal_resource(internal->iface_name); - if (!list) - return; - - pthread_mutex_lock(&internal_list_lock); - TAILQ_REMOVE(&internal_list, list, next); - pthread_mutex_unlock(&internal_list_lock); - rte_free(list); + if (list) { + rte_vhost_driver_unregister(internal->iface_name); + pthread_mutex_lock(&internal_list_lock); + TAILQ_REMOVE(&internal_list, list, next); + pthread_mutex_unlock(&internal_list_lock); + rte_free(list); + } if (dev->data->rx_queues) for (i = 0; i < dev->data->nb_rx_queues; i++) -- 2.17.0
Hi Itsuro, On 05/03/2020 02:54, Itsuro Oda wrote: > If a vhost device is closed before eth_dev_configure is done > to the device, internal resources allocated to the device > would not be freed. This patch fixes it. > > Fixes: 3d01b759d267 ("net/vhost: delay driver setup") > Cc: stable@dpdk.org > > Signed-off-by: Itsuro Oda <oda@valinux.co.jp> > Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com> This fixes an issue with the patch you backported for 18.11. Is the issue also present in the backported version? If so, this patch is not in upstream dpdk or gone through validation. So the choices are, 1. revert your patches from 18.11 2. go ahead on stable without this patch 3. delay until this patch is in master (but not until validated) and then backport to stable Itsuro/Maxime, what do you think? thanks, Kevin. > --- > v2: > - fix commit message > > v3: > - fix spell error of Reviewed-by > > drivers/net/vhost/rte_eth_vhost.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c > index 458ed58f5..1ed977e9b 100644 > --- a/drivers/net/vhost/rte_eth_vhost.c > +++ b/drivers/net/vhost/rte_eth_vhost.c > @@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev) > > eth_dev_stop(dev); > > - rte_vhost_driver_unregister(internal->iface_name); > - > list = find_internal_resource(internal->iface_name); > - if (!list) > - return; > - > - pthread_mutex_lock(&internal_list_lock); > - TAILQ_REMOVE(&internal_list, list, next); > - pthread_mutex_unlock(&internal_list_lock); > - rte_free(list); > + if (list) { > + rte_vhost_driver_unregister(internal->iface_name); > + pthread_mutex_lock(&internal_list_lock); > + TAILQ_REMOVE(&internal_list, list, next); > + pthread_mutex_unlock(&internal_list_lock); > + rte_free(list); > + } > > if (dev->data->rx_queues) > for (i = 0; i < dev->data->nb_rx_queues; i++) >
On 3/19/20 6:15 PM, Kevin Traynor wrote: > Hi Itsuro, > > On 05/03/2020 02:54, Itsuro Oda wrote: >> If a vhost device is closed before eth_dev_configure is done >> to the device, internal resources allocated to the device >> would not be freed. This patch fixes it. >> >> Fixes: 3d01b759d267 ("net/vhost: delay driver setup") >> Cc: stable@dpdk.org >> >> Signed-off-by: Itsuro Oda <oda@valinux.co.jp> >> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com> > > This fixes an issue with the patch you backported for 18.11. Is the > issue also present in the backported version? > > If so, this patch is not in upstream dpdk or gone through validation. So > the choices are, > > 1. revert your patches from 18.11 > 2. go ahead on stable without this patch > 3. delay until this patch is in master (but not until validated) and > then backport to stable > > Itsuro/Maxime, what do you think? I think you should drop Itsuro patches for now, as long as this patch is not in master. Secondary process was broken for several revisions in Vhost, so it is better to keep it broken for now than risking regressions on primary process support. If this patch is in master before the next 18.11 is done, then we can pick all the patches. Thanks, Maxime > thanks, > Kevin. > >> --- >> v2: >> - fix commit message >> >> v3: >> - fix spell error of Reviewed-by >> >> drivers/net/vhost/rte_eth_vhost.c | 16 +++++++--------- >> 1 file changed, 7 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c >> index 458ed58f5..1ed977e9b 100644 >> --- a/drivers/net/vhost/rte_eth_vhost.c >> +++ b/drivers/net/vhost/rte_eth_vhost.c >> @@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev) >> >> eth_dev_stop(dev); >> >> - rte_vhost_driver_unregister(internal->iface_name); >> - >> list = find_internal_resource(internal->iface_name); >> - if (!list) >> - return; >> - >> - pthread_mutex_lock(&internal_list_lock); >> - TAILQ_REMOVE(&internal_list, list, next); >> - pthread_mutex_unlock(&internal_list_lock); >> - rte_free(list); >> + if (list) { >> + rte_vhost_driver_unregister(internal->iface_name); >> + pthread_mutex_lock(&internal_list_lock); >> + TAILQ_REMOVE(&internal_list, list, next); >> + pthread_mutex_unlock(&internal_list_lock); >> + rte_free(list); >> + } >> >> if (dev->data->rx_queues) >> for (i = 0; i < dev->data->nb_rx_queues; i++) >> >
On 20/03/2020 16:23, Maxime Coquelin wrote: > > > On 3/19/20 6:15 PM, Kevin Traynor wrote: >> Hi Itsuro, >> >> On 05/03/2020 02:54, Itsuro Oda wrote: >>> If a vhost device is closed before eth_dev_configure is done >>> to the device, internal resources allocated to the device >>> would not be freed. This patch fixes it. >>> >>> Fixes: 3d01b759d267 ("net/vhost: delay driver setup") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Itsuro Oda <oda@valinux.co.jp> >>> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com> >> >> This fixes an issue with the patch you backported for 18.11. Is the >> issue also present in the backported version? >> >> If so, this patch is not in upstream dpdk or gone through validation. So >> the choices are, >> >> 1. revert your patches from 18.11 >> 2. go ahead on stable without this patch >> 3. delay until this patch is in master (but not until validated) and >> then backport to stable >> >> Itsuro/Maxime, what do you think? > > I think you should drop Itsuro patches for now, as long as this patch is > not in master. Secondary process was broken for several revisions in > Vhost, so it is better to keep it broken for now than risking > regressions on primary process support. > I agree, vhost primary is too well used to take risks on regression while there are still some outstanding issues being resolved with these patches for secondary process. Patches reverted. > If this patch is in master before the next 18.11 is done, then we can > pick all the patches. > Yes, we can do that. thanks, Kevin. > Thanks, > Maxime >> thanks, >> Kevin. >> >>> --- >>> v2: >>> - fix commit message >>> >>> v3: >>> - fix spell error of Reviewed-by >>> >>> drivers/net/vhost/rte_eth_vhost.c | 16 +++++++--------- >>> 1 file changed, 7 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c >>> index 458ed58f5..1ed977e9b 100644 >>> --- a/drivers/net/vhost/rte_eth_vhost.c >>> +++ b/drivers/net/vhost/rte_eth_vhost.c >>> @@ -1065,16 +1065,14 @@ eth_dev_close(struct rte_eth_dev *dev) >>> >>> eth_dev_stop(dev); >>> >>> - rte_vhost_driver_unregister(internal->iface_name); >>> - >>> list = find_internal_resource(internal->iface_name); >>> - if (!list) >>> - return; >>> - >>> - pthread_mutex_lock(&internal_list_lock); >>> - TAILQ_REMOVE(&internal_list, list, next); >>> - pthread_mutex_unlock(&internal_list_lock); >>> - rte_free(list); >>> + if (list) { >>> + rte_vhost_driver_unregister(internal->iface_name); >>> + pthread_mutex_lock(&internal_list_lock); >>> + TAILQ_REMOVE(&internal_list, list, next); >>> + pthread_mutex_unlock(&internal_list_lock); >>> + rte_free(list); >>> + } >>> >>> if (dev->data->rx_queues) >>> for (i = 0; i < dev->data->nb_rx_queues; i++) >>> >>
On 3/5/20 3:54 AM, Itsuro Oda wrote:
> If a vhost device is closed before eth_dev_configure is done
> to the device, internal resources allocated to the device
> would not be freed. This patch fixes it.
>
> Fixes: 3d01b759d267 ("net/vhost: delay driver setup")
> Cc: stable@dpdk.org
>
> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
> v2:
> - fix commit message
>
> v3:
> - fix spell error of Reviewed-by
>
> drivers/net/vhost/rte_eth_vhost.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
Applied to dpdk-next-virtio/master.
Thanks,
Maxime