DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: Adrian Moreno <amorenoz@redhat.com>,
	"Xia, Chenbo" <chenbo.xia@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"olivier.matz@6wind.com" <olivier.matz@6wind.com>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>
Subject: Re: [dpdk-dev] [PATCH 25/40] net/virtio: add Virtio-user features ops
Date: Fri, 15 Jan 2021 15:19:42 +0100
Message-ID: <933531fd-7ab0-d3a2-23a9-046767d46077@redhat.com> (raw)
In-Reply-To: <6b3ee0bc-1e26-0b48-6f84-90c4948e228b@redhat.com>



On 1/13/21 2:43 PM, Adrian Moreno wrote:
> Hi Chenbo
> 
> On 1/6/21 12:54 PM, Xia, Chenbo wrote:
>> Hi Maxime,
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Monday, December 21, 2020 5:14 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 25/40] net/virtio: add Virtio-user features ops
>>>
>>> This patch introduce new callbacks for getting
>>
>> s/introduce/introduces
>>
>>> and setting Virtio features, and implements them
>>> for the different backend types.
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>  drivers/net/virtio/virtio_user/vhost.h        |   2 +
>>>  drivers/net/virtio/virtio_user/vhost_kernel.c | 150 +++++++++---------
>>>  .../net/virtio/virtio_user/vhost_kernel_tap.c |  23 +++
>>>  .../net/virtio/virtio_user/vhost_kernel_tap.h |   1 +
>>>  drivers/net/virtio/virtio_user/vhost_user.c   |  63 +++++++-
>>>  drivers/net/virtio/virtio_user/vhost_vdpa.c   |  38 +++--
>>>  .../net/virtio/virtio_user/virtio_user_dev.c  |   5 +-
>>>  drivers/net/virtio/virtio_user_ethdev.c       |   3 +-
>>>  8 files changed, 188 insertions(+), 97 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>>> b/drivers/net/virtio/virtio_user/vhost.h
>>> index 8e819ecfb8..16978e27ed 100644
>>> --- a/drivers/net/virtio/virtio_user/vhost.h
>>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>>> @@ -102,6 +102,8 @@ struct virtio_user_dev;
>>>  struct virtio_user_backend_ops {
>>>  	int (*setup)(struct virtio_user_dev *dev);
>>>  	int (*set_owner)(struct virtio_user_dev *dev);
>>> +	int (*get_features)(struct virtio_user_dev *dev, uint64_t *features);
>>> +	int (*set_features)(struct virtio_user_dev *dev, uint64_t features);
>>>  	int (*send_request)(struct virtio_user_dev *dev,
>>>  			    enum vhost_user_request req,
>>>  			    void *arg);
>>> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> b/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> index b79dcad179..f44df8ef1f 100644
>>> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c
>>> @@ -38,6 +38,28 @@ struct vhost_memory_kernel {
>>>  #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
>>>  #define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct
>>> vhost_vring_file)
>>>
>>> +/* with below features, vhost kernel does not need to do the checksum and TSO,
>>> + * these info will be passed to virtio_user through virtio net header.
>>> + */
>>> +#define VHOST_KERNEL_GUEST_OFFLOADS_MASK	\
>>> +	((1ULL << VIRTIO_NET_F_GUEST_CSUM) |	\
>>> +	 (1ULL << VIRTIO_NET_F_GUEST_TSO4) |	\
>>> +	 (1ULL << VIRTIO_NET_F_GUEST_TSO6) |	\
>>> +	 (1ULL << VIRTIO_NET_F_GUEST_ECN)  |	\
>>> +	 (1ULL << VIRTIO_NET_F_GUEST_UFO))
>>> +
>>> +/* with below features, when flows from virtio_user to vhost kernel
>>> + * (1) if flows goes up through the kernel networking stack, it does not need
>>> + * to verify checksum, which can save CPU cycles;
>>> + * (2) if flows goes through a Linux bridge and outside from an interface
>>> + * (kernel driver), checksum and TSO will be done by GSO in kernel or even
>>> + * offloaded into real physical device.
>>> + */
>>> +#define VHOST_KERNEL_HOST_OFFLOADS_MASK		\
>>> +	((1ULL << VIRTIO_NET_F_HOST_TSO4) |	\
>>> +	 (1ULL << VIRTIO_NET_F_HOST_TSO6) |	\
>>> +	 (1ULL << VIRTIO_NET_F_CSUM))
>>> +
>>>  static uint64_t max_regions = 64;
>>>
>>>  static void
>>> @@ -77,10 +99,57 @@ vhost_kernel_set_owner(struct virtio_user_dev *dev)
>>>  	return vhost_kernel_ioctl(dev->vhostfds[0], VHOST_SET_OWNER, NULL);
>>>  }
>>>
>>> +static int
>>> +vhost_kernel_get_features(struct virtio_user_dev *dev, uint64_t *features)
>>> +{
>>> +	int ret;
>>> +	unsigned int tap_features;
>>> +
>>> +	ret = vhost_kernel_ioctl(dev->vhostfds[0], VHOST_GET_FEATURES, features);
>>> +	if (ret < 0) {
>>> +		PMD_DRV_LOG(ERR, "Failed to get features");
>>> +		return -1;
>>> +	}
>>> +
>>> +	ret = tap_support_features(&tap_features);
>>> +	if (ret < 0) {
>>> +		PMD_DRV_LOG(ERR, "Failed to get TAP features)");
>>
>> should delete ')' after 'features'?
>>
>>> +		return -1;
>>> +	}
>>> +
>>> +	/* with tap as the backend, all these features are supported
>>> +	 * but not claimed by vhost-net, so we add them back when
>>> +	 * reporting to upper layer.
>>> +	 */
>>
>> <snip>
>>
>>>  	.dma_map = vhost_vdpa_dma_map,
>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> index f8e4581951..0a85d058a8 100644
>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> @@ -141,7 +141,7 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev)
>>>  	/* Strip VIRTIO_NET_F_CTRL_VQ, as devices do not really need to know */
>>>  	features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
>>>  	features &= ~(1ull << VIRTIO_NET_F_STATUS);
>>> -	ret = dev->ops->send_request(dev, VHOST_USER_SET_FEATURES, &features);
>>> +	ret = dev->ops->set_features(dev, features);
>>
>> I noticed that virtio_user_dev_set_features is called by virtio_user_set_status.
>> The former may fail but the latter will ignore the failure. So this will happen:
>> setting features already failed but virtio-user still continue to do things. IMHO, 
>> this is not very good (similar things may happen for virtio_user_start_device).
>> What do you think?
>>
> You're right. Although  set_status(features_ok) should be followed by
> get_status() to check if device has set feature_ok bit, the problem is that
> set_status is called on device initialization, which would happen even when the
> vhost backed is not yet connected (server-mode). Failing there would tear down
> the ethdev which would make the future "hotplug" fail. Hopefully this will all
> be fixed by Maxime's refactoring :)
> So +1 to reconsider improving error propagation here.

I agree with the idea of improving the error propagation, maybe it can
be done after the series is merged as further improvements.

With making server mode made blocking, it will be easier to handle.
But there is still the reconnection case that could be problematic, I
prefer not risking introducing more regressions for this series :).

Is that OK for you?

Cheers,
Maxime

> 
>> Thanks,
>> Chenbo
>>
>>>  	if (ret < 0)
>>>  		goto error;
>>>  	PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
>>> @@ -488,8 +488,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char
>>> *path, int queues,
>>>  			return -1;
>>>  		}
>>>
>>> -		if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>> -					   &dev->device_features) < 0) {
>>> +		if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>>>  			PMD_INIT_LOG(ERR, "get_features failed: %s",
>>>  				     strerror(errno));
>>>  			return -1;
>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>> index 283f5c7a36..4d2635c8aa 100644
>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>> @@ -85,8 +85,7 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>>>
>>>  	virtio_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>>
>>> -	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>> -				   &dev->device_features) < 0) {
>>> +	if (dev->ops->get_features(dev, &dev->device_features) < 0) {
>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
>>>  			     strerror(errno));
>>>  		return -1;
>>> --
>>> 2.29.2
>>
> 


  parent reply	other threads:[~2021-01-15 14:19 UTC|newest]

Thread overview: 149+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-20 21:13 [dpdk-dev] [PATCH 00/40] net/virtio: Virtio PMD rework Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 01/40] bus/vdev: add helper to get vdev from eth dev Maxime Coquelin
2020-12-30  3:02   ` Xia, Chenbo
2021-01-05 21:15   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 02/40] net/virtio: Introduce Virtio bus type Maxime Coquelin
2020-12-30  3:02   ` Xia, Chenbo
2021-01-05 21:15   ` David Marchand
2021-01-14  9:24     ` Maxime Coquelin
2021-01-14 10:54       ` Maxime Coquelin
2021-01-14 11:55         ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 03/40] net/virtio: refactor virtio-user device Maxime Coquelin
2020-12-30  3:02   ` Xia, Chenbo
2021-01-05 21:16   ` David Marchand
2021-01-14  9:26     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 04/40] net/virtio: introduce PCI device metadata Maxime Coquelin
2020-12-30  3:02   ` Xia, Chenbo
2021-01-05 21:16   ` David Marchand
2021-01-14 11:05     ` Maxime Coquelin
2021-01-14 14:40       ` David Marchand
2021-01-14 14:44         ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 05/40] net/virtio: move PCI device init in dedicated file Maxime Coquelin
2020-12-30  3:02   ` Xia, Chenbo
2021-01-05 21:19   ` David Marchand
2021-01-14 16:04     ` Maxime Coquelin
2021-01-14 16:14       ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 06/40] net/virtio: move PCI specific dev init to PCI ethdev init Maxime Coquelin
2020-12-30  3:05   ` Xia, Chenbo
2021-01-06  8:58   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 07/40] net/virtio: move MSIX detection to PCI ethdev Maxime Coquelin
2020-12-30  3:05   ` Xia, Chenbo
2021-01-06  8:22   ` David Marchand
2021-01-14 17:19     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 08/40] net/virtio: force IOVA as VA mode for Virtio-user Maxime Coquelin
2020-12-30  3:06   ` Xia, Chenbo
2021-01-06  9:06   ` David Marchand
2021-01-06  9:11     ` Thomas Monjalon
2021-01-06  9:22       ` Maxime Coquelin
2021-01-06 16:37       ` Kinsella, Ray
2021-01-06  9:14     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 09/40] net/virtio: store PCI type in Virtio device metadata Maxime Coquelin
2020-12-30  3:07   ` Xia, Chenbo
2021-01-06  9:14   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 10/40] net/virtio: add callback for device closing Maxime Coquelin
2020-12-30  3:07   ` Xia, Chenbo
2021-01-06  9:33   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 11/40] net/virtio: validate features at bus level Maxime Coquelin
2020-12-30  3:08   ` Xia, Chenbo
2021-01-06  9:33   ` David Marchand
2021-01-15  9:20     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 12/40] net/virtio: remove bus type enum Maxime Coquelin
2020-12-30  3:08   ` Xia, Chenbo
2021-01-06  9:33   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 13/40] net/virtio: move PCI-specific fields to PCI device Maxime Coquelin
2020-12-30  3:08   ` Xia, Chenbo
2021-01-06  9:58   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 14/40] net/virtio: pack virtio HW struct Maxime Coquelin
2020-12-30  3:09   ` Xia, Chenbo
2021-01-06  9:58   ` David Marchand
2021-01-15  9:35     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 15/40] net/virtio: move legacy IO to Virtio PCI Maxime Coquelin
2020-12-30  3:09   ` Xia, Chenbo
2021-01-06 10:09   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 16/40] net/virtio: introduce generic virtio header Maxime Coquelin
2020-12-30  3:09   ` Xia, Chenbo
2021-01-06 10:08   ` David Marchand
2021-01-15  9:39     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 17/40] net/virtio: move features definition to generic header Maxime Coquelin
2020-12-30  3:14   ` Xia, Chenbo
2021-01-14  8:40     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 18/40] net/virtio: move virtqueue defines in " Maxime Coquelin
2020-12-30  3:14   ` Xia, Chenbo
2021-01-06 15:53   ` David Marchand
2021-01-15 10:55     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 19/40] net/virtio: move config definitions to " Maxime Coquelin
2020-12-30  3:15   ` Xia, Chenbo
2021-01-06 16:01   ` David Marchand
2021-01-15 11:01     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 20/40] net/virtio: make interrupt handling more generic Maxime Coquelin
2020-12-30  3:17   ` Xia, Chenbo
2021-01-14  8:43     ` Maxime Coquelin
2021-01-06 16:07   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 21/40] net/virtio: move vring alignment to generic header Maxime Coquelin
2020-12-30  3:18   ` Xia, Chenbo
2021-01-06 16:13   ` David Marchand
2020-12-20 21:13 ` [dpdk-dev] [PATCH 22/40] net/virtio: remove last PCI refs in non-PCI code Maxime Coquelin
2020-12-30  3:25   ` Xia, Chenbo
2021-01-14  8:46     ` Maxime Coquelin
2021-01-06 16:18   ` David Marchand
2021-01-15 11:10     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 23/40] net/virtio: make Vhost-user req sender consistent Maxime Coquelin
2021-01-06 11:50   ` Xia, Chenbo
2021-01-15  9:47     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 24/40] net/virtio: add Virtio-user ops to set owner Maxime Coquelin
2021-01-06 11:50   ` Xia, Chenbo
2020-12-20 21:13 ` [dpdk-dev] [PATCH 25/40] net/virtio: add Virtio-user features ops Maxime Coquelin
2021-01-06 11:54   ` Xia, Chenbo
2021-01-13 13:43     ` Adrian Moreno
2021-01-13 13:54       ` Maxime Coquelin
2021-01-15 14:19       ` Maxime Coquelin [this message]
2021-01-13 13:57   ` Adrian Moreno
2021-01-15 14:29     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 26/40] net/virtio: add Virtio-user protocol " Maxime Coquelin
2021-01-06 11:55   ` Xia, Chenbo
2020-12-20 21:13 ` [dpdk-dev] [PATCH 27/40] net/virtio: add Virtio-user memory tables ops Maxime Coquelin
2021-01-06 11:57   ` Xia, Chenbo
2021-01-15  9:57     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 28/40] net/virtio: add Virtio-user vring setting ops Maxime Coquelin
2021-01-05 21:24   ` David Marchand
2021-01-06 12:01   ` Xia, Chenbo
2021-01-15 10:12     ` Maxime Coquelin
2021-01-06 12:03   ` Xia, Chenbo
2021-01-15 10:15     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 29/40] net/virtio: add Virtio-user vring file ops Maxime Coquelin
2021-01-05 21:24   ` David Marchand
2021-01-06 12:04   ` Xia, Chenbo
2021-01-15 10:17     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 30/40] net/virtio: add Virtio-user vring address ops Maxime Coquelin
2021-01-06 12:06   ` Xia, Chenbo
2021-01-15 10:19     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 31/40] net/virtio: add Virtio-user status ops Maxime Coquelin
2021-01-06 12:09   ` Xia, Chenbo
2021-01-15 10:48     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 32/40] net/virtio: remove useless request ops Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 33/40] net/virtio: improve Virtio-user errors handling Maxime Coquelin
2021-01-07  2:26   ` Xia, Chenbo
2021-01-15 11:09     ` Maxime Coquelin
2020-12-20 21:13 ` [dpdk-dev] [PATCH 34/40] net/virtio: move Vhost-user reqs to Vhost-user backend Maxime Coquelin
2020-12-20 21:14 ` [dpdk-dev] [PATCH 35/40] net/virtio: make server mode blocking Maxime Coquelin
2021-01-07  3:20   ` Xia, Chenbo
2021-01-15 11:13     ` Maxime Coquelin
2020-12-20 21:14 ` [dpdk-dev] [PATCH 36/40] net/virtio: move protocol features to Vhost-user Maxime Coquelin
2020-12-20 21:14 ` [dpdk-dev] [PATCH 37/40] net/virtio: introduce backend data Maxime Coquelin
2021-01-05 21:26   ` David Marchand
2021-01-13 17:18   ` Adrian Moreno
2021-01-15 16:49     ` Maxime Coquelin
2020-12-20 21:14 ` [dpdk-dev] [PATCH 38/40] net/virtio: move Vhost-user specifics to its backend Maxime Coquelin
2021-01-07  6:32   ` Xia, Chenbo
2021-01-15 12:03     ` Maxime Coquelin
2020-12-20 21:14 ` [dpdk-dev] [PATCH 39/40] net/virtio: move Vhost-kernel data " Maxime Coquelin
2021-01-07  6:42   ` Xia, Chenbo
2021-01-11  8:02   ` Xia, Chenbo
2021-01-15 11:54     ` Maxime Coquelin
2021-01-18 20:36     ` Maxime Coquelin
2020-12-20 21:14 ` [dpdk-dev] [PATCH 40/40] net/virtio: move Vhost-vDPA " Maxime Coquelin
2020-12-22 15:20   ` Maxime Coquelin
2021-01-07  6:50   ` Xia, Chenbo
2021-01-15 12:08     ` Maxime Coquelin
2021-01-11  8:05   ` Xia, Chenbo
2020-12-21 10:58 ` [dpdk-dev] [PATCH 00/40] net/virtio: Virtio PMD rework Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=933531fd-7ab0-d3a2-23a9-046767d46077@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --cc=amorenoz@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=olivier.matz@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git