[dpdk-dev,1/2] vhost: reference count fix for nb_started_ports
Checks
Commit Message
From: Wen Chiu <wchiu@brocade.com>
Only increment and decrement nb_started_ports on the first and last
device start and stop. Otherwise, nb_started_ports can become negative
if a device is stopped multiple times.
Fixes: ee584e9710b9 ("vhost: add driver on top of the library")
Signed-off-by: Wen Chiu <wchiu@brocade.com>
Reviewed-by: Chas Williams <ciwillia@brocade.com>
---
drivers/net/vhost/rte_eth_vhost.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
Comments
On Wed, Dec 28, 2016 at 04:10:51PM -0500, Charles (Chas) Williams wrote:
> From: Wen Chiu <wchiu@brocade.com>
>
> Only increment and decrement nb_started_ports on the first and last
> device start and stop. Otherwise, nb_started_ports can become negative
> if a device is stopped multiple times.
How could you be able to stop dev (precisely, invoke eth_dev_stop)
multiple times, judging that eth_dev_stop() will be invoked once
only?
void
rte_eth_dev_stop(uint8_t port_id)
{
struct rte_eth_dev *dev;
RTE_ETH_VALID_PORTID_OR_RET(port_id);
dev = &rte_eth_devices[port_id];
RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_stop);
==> if (dev->data->dev_started == 0) {
RTE_PMD_DEBUG_TRACE("Device with port_id=%" PRIu8
" already stopped\n",
port_id);
return;
}
==> dev->data->dev_started = 0;
(*dev->dev_ops->dev_stop)(dev);
}
Multiple threads?
--yliu
On 12/29/2016 03:51 AM, Yuanhan Liu wrote:
> On Wed, Dec 28, 2016 at 04:10:51PM -0500, Charles (Chas) Williams wrote:
>> From: Wen Chiu <wchiu@brocade.com>
>>
>> Only increment and decrement nb_started_ports on the first and last
>> device start and stop. Otherwise, nb_started_ports can become negative
>> if a device is stopped multiple times.
>
> How could you be able to stop dev (precisely, invoke eth_dev_stop)
> multiple times, judging that eth_dev_stop() will be invoked once
> only?
>
> void
> rte_eth_dev_stop(uint8_t port_id)
> {
> struct rte_eth_dev *dev;
>
> RTE_ETH_VALID_PORTID_OR_RET(port_id);
> dev = &rte_eth_devices[port_id];
>
> RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_stop);
>
> ==> if (dev->data->dev_started == 0) {
> RTE_PMD_DEBUG_TRACE("Device with port_id=%" PRIu8
> " already stopped\n",
> port_id);
> return;
> }
>
> ==> dev->data->dev_started = 0;
> (*dev->dev_ops->dev_stop)(dev);
> }
>
> Multiple threads?
No, we aren't using multiple threads for control. But eth_dev_stop()
is called in rte_pmd_vhost_remove():
static int
rte_pmd_vhost_remove(const char *name)
{
...
pthread_mutex_lock(&internal_list_lock);
TAILQ_REMOVE(&internal_list, list, next);
pthread_mutex_unlock(&internal_list_lock);
rte_free(list);
eth_dev_stop(eth_dev);
...
So, if we .dev_stop() and deatch the virtual device, eth_dev_stop()
gets called twice. Calling .dev_stop() when you are about to detach
the device seems completely reasonable. It also seems reasonable to
call eth_dev_stop() inside rte_pmd_vhost_remove() in case the end
user didn't do a .dev_stop().
@@ -782,11 +782,11 @@ eth_dev_start(struct rte_eth_dev *dev)
internal->flags);
if (ret)
return ret;
- }
- /* We need only one message handling thread */
- if (rte_atomic16_add_return(&nb_started_ports, 1) == 1)
- ret = vhost_driver_session_start();
+ /* We need only one message handling thread */
+ if (rte_atomic16_add_return(&nb_started_ports, 1) == 1)
+ ret = vhost_driver_session_start();
+ }
return ret;
}
@@ -796,11 +796,12 @@ eth_dev_stop(struct rte_eth_dev *dev)
{
struct pmd_internal *internal = dev->data->dev_private;
- if (rte_atomic16_cmpset(&internal->once, 1, 0))
+ if (rte_atomic16_cmpset(&internal->once, 1, 0)) {
rte_vhost_driver_unregister(internal->iface_name);
- if (rte_atomic16_sub_return(&nb_started_ports, 1) == 0)
- vhost_driver_session_stop();
+ if (rte_atomic16_sub_return(&nb_started_ports, 1) == 0)
+ vhost_driver_session_stop();
+ }
}
static int