| Message ID | 20201220211405.313012-4-maxime.coquelin@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Maxime Coquelin |
| Headers | show |
| Series | net/virtio: Virtio PMD rework | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | warning | coding style issues |
> -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Monday, December 21, 2020 5:13 AM > To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; olivier.matz@6wind.com; > amorenoz@redhat.com; david.marchand@redhat.com > Cc: Maxime Coquelin <maxime.coquelin@redhat.com> > Subject: [PATCH 03/40] net/virtio: refactor virtio-user device > > This patch moves the virtio_hw structure into the virtio_user_dev > structure, with the goal of making virtio_hw bus-agnostic. > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > drivers/net/virtio/virtio_ethdev.c | 2 +- > drivers/net/virtio/virtio_pci.h | 1 - > .../net/virtio/virtio_user/virtio_user_dev.h | 1 + > drivers/net/virtio/virtio_user_ethdev.c | 62 ++++++++----------- > 4 files changed, 27 insertions(+), 39 deletions(-) > > diff --git a/drivers/net/virtio/virtio_ethdev.c > b/drivers/net/virtio/virtio_ethdev.c > index b3086297c0..3ace25ac80 100644 > --- a/drivers/net/virtio/virtio_ethdev.c > +++ b/drivers/net/virtio/virtio_ethdev.c > @@ -747,7 +747,7 @@ virtio_dev_close(struct rte_eth_dev *dev) > > #ifdef RTE_VIRTIO_USER > if (hw->bus_type == VIRTIO_BUS_USER) > - virtio_user_dev_uninit(hw->virtio_user_dev); > + virtio_user_dev_uninit(dev->data->dev_private); > else > #endif > if (dev->device) { > diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h > index 6388f0a74d..b35a596169 100644 > --- a/drivers/net/virtio/virtio_pci.h > +++ b/drivers/net/virtio/virtio_pci.h > @@ -277,7 +277,6 @@ struct virtio_hw { > uint16_t *notify_base; > struct virtio_pci_common_cfg *common_cfg; > struct virtio_net_config *dev_cfg; > - void *virtio_user_dev; > /* > * App management thread and virtio interrupt handler thread > * both can change device state, this lock is meant to avoid > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h > b/drivers/net/virtio/virtio_user/virtio_user_dev.h > index e053897d8f..59f4dd1f24 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h > @@ -24,6 +24,7 @@ struct virtio_user_queue { > }; > > struct virtio_user_dev { > + struct virtio_hw hw; > enum virtio_user_backend_type backend_type; > /* for vhost_user backend */ > int vhostfd; > diff --git a/drivers/net/virtio/virtio_user_ethdev.c > b/drivers/net/virtio/virtio_user_ethdev.c > index 516d0ee577..1f1f63a1a5 100644 > --- a/drivers/net/virtio/virtio_user_ethdev.c > +++ b/drivers/net/virtio/virtio_user_ethdev.c > @@ -26,13 +26,13 @@ > #include "virtio_user/virtio_user_dev.h" > #include "virtio_user/vhost.h" > > -#define virtio_user_get_dev(hw) \ > - ((struct virtio_user_dev *)(hw)->virtio_user_dev) > +#define virtio_user_get_dev(hw) container_of(hw, struct virtio_user_dev, hw) > > static void > -virtio_user_reset_queues_packed(struct rte_eth_dev *dev) > +virtio_user_reset_queues_packed(struct rte_eth_dev *eth_dev) > { > - struct virtio_hw *hw = dev->data->dev_private; > + struct virtio_user_dev *dev = eth_dev->data->dev_private; > + struct virtio_hw *hw = &dev->hw; > struct virtnet_rx *rxvq; > struct virtnet_tx *txvq; > uint16_t i; > @@ -48,14 +48,14 @@ virtio_user_reset_queues_packed(struct rte_eth_dev *dev) > rte_delay_ms(1); > > /* Vring reset for each Tx queue and Rx queue. */ > - for (i = 0; i < dev->data->nb_rx_queues; i++) { > - rxvq = dev->data->rx_queues[i]; > + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { > + rxvq = eth_dev->data->rx_queues[i]; > virtqueue_rxvq_reset_packed(rxvq->vq); > - virtio_dev_rx_queue_setup_finish(dev, i); > + virtio_dev_rx_queue_setup_finish(eth_dev, i); > } > > - for (i = 0; i < dev->data->nb_tx_queues; i++) { > - txvq = dev->data->tx_queues[i]; > + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > + txvq = eth_dev->data->tx_queues[i]; > virtqueue_txvq_reset_packed(txvq->vq); > } > > @@ -69,7 +69,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev) > { > int ret, connectfd, old_status; > struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id]; > - struct virtio_hw *hw = eth_dev->data->dev_private; > + struct virtio_hw *hw = &dev->hw; > uint64_t protocol_features; > > connectfd = accept(dev->listenfd, NULL, NULL); > @@ -605,21 +605,15 @@ virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev) > struct virtio_hw *hw; > struct virtio_user_dev *dev; > > - eth_dev = rte_eth_vdev_allocate(vdev, sizeof(*hw)); > + eth_dev = rte_eth_vdev_allocate(vdev, sizeof(*dev)); > if (!eth_dev) { > PMD_INIT_LOG(ERR, "cannot alloc rte_eth_dev"); > return NULL; > } > > data = eth_dev->data; > - hw = eth_dev->data->dev_private; > - > - dev = rte_zmalloc(NULL, sizeof(*dev), 0); > - if (!dev) { > - PMD_INIT_LOG(ERR, "malloc virtio_user_dev failed"); > - rte_eth_dev_release_port(eth_dev); > - return NULL; > - } > + dev = eth_dev->data->dev_private; > + hw = &dev->hw; > > hw->port_id = data->port_id; > dev->port_id = data->port_id; > @@ -634,17 +628,13 @@ virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev) > hw->use_vec_tx = 0; > hw->use_inorder_rx = 0; > hw->use_inorder_tx = 0; > - hw->virtio_user_dev = dev; > + > return eth_dev; > } > > static void > virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev) > { > - struct rte_eth_dev_data *data = eth_dev->data; > - struct virtio_hw *hw = data->dev_private; > - > - rte_free(hw->virtio_user_dev); > rte_eth_dev_release_port(eth_dev); > } > > @@ -653,11 +643,12 @@ virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev) > * Returns 0 on success. > */ > static int > -virtio_user_pmd_probe(struct rte_vdev_device *dev) > +virtio_user_pmd_probe(struct rte_vdev_device *vdev) > { > struct rte_kvargs *kvlist = NULL; > struct rte_eth_dev *eth_dev; > struct virtio_hw *hw; > + struct virtio_user_dev *dev; > enum virtio_user_backend_type backend_type = VIRTIO_USER_BACKEND_UNKNOWN; > uint64_t queues = VIRTIO_USER_DEF_Q_NUM; > uint64_t cq = VIRTIO_USER_DEF_CQ_EN; > @@ -673,7 +664,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) > int ret = -1; > > if (rte_eal_process_type() == RTE_PROC_SECONDARY) { > - const char *name = rte_vdev_device_name(dev); > + const char *name = rte_vdev_device_name(vdev); > eth_dev = rte_eth_dev_attach_secondary(name); > if (!eth_dev) { > PMD_INIT_LOG(ERR, "Failed to probe %s", name); > @@ -687,12 +678,12 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) > } > > eth_dev->dev_ops = &virtio_user_secondary_eth_dev_ops; > - eth_dev->device = &dev->device; > + eth_dev->device = &vdev->device; > rte_eth_dev_probing_finish(eth_dev); > return 0; > } > > - kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), valid_args); > + kvlist = rte_kvargs_parse(rte_vdev_device_args(vdev), valid_args); > if (!kvlist) { > PMD_INIT_LOG(ERR, "error when parsing param"); > goto end; > @@ -832,14 +823,15 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) > } > } > > - eth_dev = virtio_user_eth_dev_alloc(dev); > + eth_dev = virtio_user_eth_dev_alloc(vdev); > if (!eth_dev) { > PMD_INIT_LOG(ERR, "virtio_user fails to alloc device"); > goto end; > } > > - hw = eth_dev->data->dev_private; > - if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq, > + dev = eth_dev->data->dev_private; > + hw = &dev->hw; > + if (virtio_user_dev_init(dev, path, queues, cq, > queue_size, mac_addr, &ifname, server_mode, > mrg_rxbuf, in_order, packed_vq, backend_type) < 0) { > PMD_INIT_LOG(ERR, "virtio_user_dev_init fails"); > @@ -912,7 +904,6 @@ static int virtio_user_pmd_dma_map(struct rte_vdev_device > *vdev, void *addr, > const char *name; > struct rte_eth_dev *eth_dev; > struct virtio_user_dev *dev; > - struct virtio_hw *hw; > > if (!vdev) > return -EINVAL; > @@ -923,8 +914,7 @@ static int virtio_user_pmd_dma_map(struct rte_vdev_device > *vdev, void *addr, > if (!eth_dev) > return 0; > > - hw = (struct virtio_hw *)eth_dev->data->dev_private; > - dev = hw->virtio_user_dev; > + dev = eth_dev->data->dev_private; > > if (dev->ops->dma_map) > return dev->ops->dma_map(dev, addr, iova, len); > @@ -938,7 +928,6 @@ static int virtio_user_pmd_dma_unmap(struct > rte_vdev_device *vdev, void *addr, > const char *name; > struct rte_eth_dev *eth_dev; > struct virtio_user_dev *dev; > - struct virtio_hw *hw; > > if (!vdev) > return -EINVAL; > @@ -949,8 +938,7 @@ static int virtio_user_pmd_dma_unmap(struct > rte_vdev_device *vdev, void *addr, > if (!eth_dev) > return 0; > > - hw = (struct virtio_hw *)eth_dev->data->dev_private; > - dev = hw->virtio_user_dev; > + dev = eth_dev->data->dev_private; > > if (dev->ops->dma_unmap) > return dev->ops->dma_unmap(dev, addr, iova, len); > -- > 2.29.2 Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
On Sun, Dec 20, 2020 at 10:14 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > This patch moves the virtio_hw structure into the virtio_user_dev > structure, with the goal of making virtio_hw bus-agnostic. > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Just one comment, the rest lgtm. > diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c > index 516d0ee577..1f1f63a1a5 100644 > --- a/drivers/net/virtio/virtio_user_ethdev.c > +++ b/drivers/net/virtio/virtio_user_ethdev.c > @@ -26,13 +26,13 @@ > #include "virtio_user/virtio_user_dev.h" > #include "virtio_user/vhost.h" > > -#define virtio_user_get_dev(hw) \ > - ((struct virtio_user_dev *)(hw)->virtio_user_dev) > +#define virtio_user_get_dev(hw) container_of(hw, struct virtio_user_dev, hw) Since the hw parameter is expanded as both the object and the field name too, this macro prevents us from calling anything but virtio_user_get_dev(hw).
On 1/5/21 10:16 PM, David Marchand wrote: > On Sun, Dec 20, 2020 at 10:14 PM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: >> >> This patch moves the virtio_hw structure into the virtio_user_dev >> structure, with the goal of making virtio_hw bus-agnostic. >> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > > Just one comment, the rest lgtm. > > >> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c >> index 516d0ee577..1f1f63a1a5 100644 >> --- a/drivers/net/virtio/virtio_user_ethdev.c >> +++ b/drivers/net/virtio/virtio_user_ethdev.c >> @@ -26,13 +26,13 @@ >> #include "virtio_user/virtio_user_dev.h" >> #include "virtio_user/vhost.h" >> >> -#define virtio_user_get_dev(hw) \ >> - ((struct virtio_user_dev *)(hw)->virtio_user_dev) >> +#define virtio_user_get_dev(hw) container_of(hw, struct virtio_user_dev, hw) > > Since the hw parameter is expanded as both the object and the field > name too, this macro prevents us from calling anything but > virtio_user_get_dev(hw). Indeed! I propose to change the parameter to hwp (hw pointer). Thanks, Maxime
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index b3086297c0..3ace25ac80 100644 --- a/drivers/net/virtio/virtio_ethdev.c +++ b/drivers/net/virtio/virtio_ethdev.c @@ -747,7 +747,7 @@ virtio_dev_close(struct rte_eth_dev *dev) #ifdef RTE_VIRTIO_USER if (hw->bus_type == VIRTIO_BUS_USER) - virtio_user_dev_uninit(hw->virtio_user_dev); + virtio_user_dev_uninit(dev->data->dev_private); else #endif if (dev->device) { diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h index 6388f0a74d..b35a596169 100644 --- a/drivers/net/virtio/virtio_pci.h +++ b/drivers/net/virtio/virtio_pci.h @@ -277,7 +277,6 @@ struct virtio_hw { uint16_t *notify_base; struct virtio_pci_common_cfg *common_cfg; struct virtio_net_config *dev_cfg; - void *virtio_user_dev; /* * App management thread and virtio interrupt handler thread * both can change device state, this lock is meant to avoid diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h index e053897d8f..59f4dd1f24 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h @@ -24,6 +24,7 @@ struct virtio_user_queue { }; struct virtio_user_dev { + struct virtio_hw hw; enum virtio_user_backend_type backend_type; /* for vhost_user backend */ int vhostfd; diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index 516d0ee577..1f1f63a1a5 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -26,13 +26,13 @@ #include "virtio_user/virtio_user_dev.h" #include "virtio_user/vhost.h" -#define virtio_user_get_dev(hw) \ - ((struct virtio_user_dev *)(hw)->virtio_user_dev) +#define virtio_user_get_dev(hw) container_of(hw, struct virtio_user_dev, hw) static void -virtio_user_reset_queues_packed(struct rte_eth_dev *dev) +virtio_user_reset_queues_packed(struct rte_eth_dev *eth_dev) { - struct virtio_hw *hw = dev->data->dev_private; + struct virtio_user_dev *dev = eth_dev->data->dev_private; + struct virtio_hw *hw = &dev->hw; struct virtnet_rx *rxvq; struct virtnet_tx *txvq; uint16_t i; @@ -48,14 +48,14 @@ virtio_user_reset_queues_packed(struct rte_eth_dev *dev) rte_delay_ms(1); /* Vring reset for each Tx queue and Rx queue. */ - for (i = 0; i < dev->data->nb_rx_queues; i++) { - rxvq = dev->data->rx_queues[i]; + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { + rxvq = eth_dev->data->rx_queues[i]; virtqueue_rxvq_reset_packed(rxvq->vq); - virtio_dev_rx_queue_setup_finish(dev, i); + virtio_dev_rx_queue_setup_finish(eth_dev, i); } - for (i = 0; i < dev->data->nb_tx_queues; i++) { - txvq = dev->data->tx_queues[i]; + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { + txvq = eth_dev->data->tx_queues[i]; virtqueue_txvq_reset_packed(txvq->vq); } @@ -69,7 +69,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev) { int ret, connectfd, old_status; struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id]; - struct virtio_hw *hw = eth_dev->data->dev_private; + struct virtio_hw *hw = &dev->hw; uint64_t protocol_features; connectfd = accept(dev->listenfd, NULL, NULL); @@ -605,21 +605,15 @@ virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev) struct virtio_hw *hw; struct virtio_user_dev *dev; - eth_dev = rte_eth_vdev_allocate(vdev, sizeof(*hw)); + eth_dev = rte_eth_vdev_allocate(vdev, sizeof(*dev)); if (!eth_dev) { PMD_INIT_LOG(ERR, "cannot alloc rte_eth_dev"); return NULL; } data = eth_dev->data; - hw = eth_dev->data->dev_private; - - dev = rte_zmalloc(NULL, sizeof(*dev), 0); - if (!dev) { - PMD_INIT_LOG(ERR, "malloc virtio_user_dev failed"); - rte_eth_dev_release_port(eth_dev); - return NULL; - } + dev = eth_dev->data->dev_private; + hw = &dev->hw; hw->port_id = data->port_id; dev->port_id = data->port_id; @@ -634,17 +628,13 @@ virtio_user_eth_dev_alloc(struct rte_vdev_device *vdev) hw->use_vec_tx = 0; hw->use_inorder_rx = 0; hw->use_inorder_tx = 0; - hw->virtio_user_dev = dev; + return eth_dev; } static void virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev) { - struct rte_eth_dev_data *data = eth_dev->data; - struct virtio_hw *hw = data->dev_private; - - rte_free(hw->virtio_user_dev); rte_eth_dev_release_port(eth_dev); } @@ -653,11 +643,12 @@ virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev) * Returns 0 on success. */ static int -virtio_user_pmd_probe(struct rte_vdev_device *dev) +virtio_user_pmd_probe(struct rte_vdev_device *vdev) { struct rte_kvargs *kvlist = NULL; struct rte_eth_dev *eth_dev; struct virtio_hw *hw; + struct virtio_user_dev *dev; enum virtio_user_backend_type backend_type = VIRTIO_USER_BACKEND_UNKNOWN; uint64_t queues = VIRTIO_USER_DEF_Q_NUM; uint64_t cq = VIRTIO_USER_DEF_CQ_EN; @@ -673,7 +664,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) int ret = -1; if (rte_eal_process_type() == RTE_PROC_SECONDARY) { - const char *name = rte_vdev_device_name(dev); + const char *name = rte_vdev_device_name(vdev); eth_dev = rte_eth_dev_attach_secondary(name); if (!eth_dev) { PMD_INIT_LOG(ERR, "Failed to probe %s", name); @@ -687,12 +678,12 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) } eth_dev->dev_ops = &virtio_user_secondary_eth_dev_ops; - eth_dev->device = &dev->device; + eth_dev->device = &vdev->device; rte_eth_dev_probing_finish(eth_dev); return 0; } - kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), valid_args); + kvlist = rte_kvargs_parse(rte_vdev_device_args(vdev), valid_args); if (!kvlist) { PMD_INIT_LOG(ERR, "error when parsing param"); goto end; @@ -832,14 +823,15 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev) } } - eth_dev = virtio_user_eth_dev_alloc(dev); + eth_dev = virtio_user_eth_dev_alloc(vdev); if (!eth_dev) { PMD_INIT_LOG(ERR, "virtio_user fails to alloc device"); goto end; } - hw = eth_dev->data->dev_private; - if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq, + dev = eth_dev->data->dev_private; + hw = &dev->hw; + if (virtio_user_dev_init(dev, path, queues, cq, queue_size, mac_addr, &ifname, server_mode, mrg_rxbuf, in_order, packed_vq, backend_type) < 0) { PMD_INIT_LOG(ERR, "virtio_user_dev_init fails"); @@ -912,7 +904,6 @@ static int virtio_user_pmd_dma_map(struct rte_vdev_device *vdev, void *addr, const char *name; struct rte_eth_dev *eth_dev; struct virtio_user_dev *dev; - struct virtio_hw *hw; if (!vdev) return -EINVAL; @@ -923,8 +914,7 @@ static int virtio_user_pmd_dma_map(struct rte_vdev_device *vdev, void *addr, if (!eth_dev) return 0; - hw = (struct virtio_hw *)eth_dev->data->dev_private; - dev = hw->virtio_user_dev; + dev = eth_dev->data->dev_private; if (dev->ops->dma_map) return dev->ops->dma_map(dev, addr, iova, len); @@ -938,7 +928,6 @@ static int virtio_user_pmd_dma_unmap(struct rte_vdev_device *vdev, void *addr, const char *name; struct rte_eth_dev *eth_dev; struct virtio_user_dev *dev; - struct virtio_hw *hw; if (!vdev) return -EINVAL; @@ -949,8 +938,7 @@ static int virtio_user_pmd_dma_unmap(struct rte_vdev_device *vdev, void *addr, if (!eth_dev) return 0; - hw = (struct virtio_hw *)eth_dev->data->dev_private; - dev = hw->virtio_user_dev; + dev = eth_dev->data->dev_private; if (dev->ops->dma_unmap) return dev->ops->dma_unmap(dev, addr, iova, len);
This patch moves the virtio_hw structure into the virtio_user_dev structure, with the goal of making virtio_hw bus-agnostic. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- drivers/net/virtio/virtio_ethdev.c | 2 +- drivers/net/virtio/virtio_pci.h | 1 - .../net/virtio/virtio_user/virtio_user_dev.h | 1 + drivers/net/virtio/virtio_user_ethdev.c | 62 ++++++++----------- 4 files changed, 27 insertions(+), 39 deletions(-)