diff mbox

[dpdk-dev,7/7] vhost: simplify features set/get

Message ID 1471510123-4984-8-git-send-email-yuanhan.liu@linux.intel.com (mailing list archive)
State Accepted, archived
Delegated to: Yuanhan Liu
Headers

Commit Message

Yuanhan Liu Aug. 18, 2016, 8:48 a.m. UTC 
  No need to use a pointer to store/retrieve features.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/vhost_user.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

Comments

Maxime Coquelin Aug. 24, 2016, 8:11 a.m. UTC | #1 
On 08/18/2016 10:48 AM, Yuanhan Liu wrote:
> No need to use a pointer to store/retrieve features.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index ef4a0c1..eee99e9 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -155,23 +155,22 @@ vhost_user_reset_owner(struct virtio_net *dev)
>  /*
>   * The features that we support are requested.
>   */
> -static int
> -vhost_user_get_features(uint64_t *pu)
> +static uint64_t
> +vhost_user_get_features(void)
>  {
> -	*pu = VHOST_FEATURES;
> -	return 0;
> +	return VHOST_FEATURES;
>  }

This is not the topic of this series, but I wonder if it
could make sense to be able to override supported features
at device init time.

It may not match with the orignal purpose of supported features,
but could be useful at least for testing without recompilation.

For this patch:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
Yuanhan Liu Aug. 25, 2016, 3:03 a.m. UTC | #2 
On Wed, Aug 24, 2016 at 10:11:57AM +0200, Maxime Coquelin wrote:
> 
> 
> On 08/18/2016 10:48 AM, Yuanhan Liu wrote:
> >No need to use a pointer to store/retrieve features.
> >
> >Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >---
> > lib/librte_vhost/vhost_user.c | 20 ++++++++------------
> > 1 file changed, 8 insertions(+), 12 deletions(-)
> >
> >diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> >index ef4a0c1..eee99e9 100644
> >--- a/lib/librte_vhost/vhost_user.c
> >+++ b/lib/librte_vhost/vhost_user.c
> >@@ -155,23 +155,22 @@ vhost_user_reset_owner(struct virtio_net *dev)
> > /*
> >  * The features that we support are requested.
> >  */
> >-static int
> >-vhost_user_get_features(uint64_t *pu)
> >+static uint64_t
> >+vhost_user_get_features(void)
> > {
> >-	*pu = VHOST_FEATURES;
> >-	return 0;
> >+	return VHOST_FEATURES;
> > }
> 
> This is not the topic of this series, but I wonder if it
> could make sense to be able to override supported features
> at device init time.

Not quite sure I understood it correctly: is rte_vhost_feature_disable()
the answer you are looking for?

> It may not match with the orignal purpose of supported features,
> but could be useful at least for testing without recompilation.
> 
> For this patch:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Again, appreicate your time on review!

	--yliu
Maxime Coquelin Aug. 25, 2016, 7:18 a.m. UTC | #3 
On 08/25/2016 05:03 AM, Yuanhan Liu wrote:
> On Wed, Aug 24, 2016 at 10:11:57AM +0200, Maxime Coquelin wrote:
>>
>>
>> On 08/18/2016 10:48 AM, Yuanhan Liu wrote:
>>> No need to use a pointer to store/retrieve features.
>>>
>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>> ---
>>> lib/librte_vhost/vhost_user.c | 20 ++++++++------------
>>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index ef4a0c1..eee99e9 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -155,23 +155,22 @@ vhost_user_reset_owner(struct virtio_net *dev)
>>> /*
>>>  * The features that we support are requested.
>>>  */
>>> -static int
>>> -vhost_user_get_features(uint64_t *pu)
>>> +static uint64_t
>>> +vhost_user_get_features(void)
>>> {
>>> -	*pu = VHOST_FEATURES;
>>> -	return 0;
>>> +	return VHOST_FEATURES;
>>> }
>>
>> This is not the topic of this series, but I wonder if it
>> could make sense to be able to override supported features
>> at device init time.
>
> Not quite sure I understood it correctly: is rte_vhost_feature_disable()
> the answer you are looking for?
Not really.

I meant a per-device supported features, and something you could set
also via the vhost PMD options, without needing to recompile.

But maybe it would make more sense to do it a guest level?

Maxime
Xu, Qian Q Aug. 25, 2016, 8:36 a.m. UTC | #4 
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
Sent: Thursday, August 25, 2016 3:19 PM
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 7/7] vhost: simplify features set/get



On 08/25/2016 05:03 AM, Yuanhan Liu wrote:
> On Wed, Aug 24, 2016 at 10:11:57AM +0200, Maxime Coquelin wrote:
>>
>>
>> On 08/18/2016 10:48 AM, Yuanhan Liu wrote:
>>> No need to use a pointer to store/retrieve features.
>>>
>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>> ---
>>> lib/librte_vhost/vhost_user.c | 20 ++++++++------------
>>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c 
>>> b/lib/librte_vhost/vhost_user.c index ef4a0c1..eee99e9 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -155,23 +155,22 @@ vhost_user_reset_owner(struct virtio_net *dev)
>>> /*
>>>  * The features that we support are requested.
>>>  */
>>> -static int
>>> -vhost_user_get_features(uint64_t *pu)
>>> +static uint64_t
>>> +vhost_user_get_features(void)
>>> {
>>> -	*pu = VHOST_FEATURES;
>>> -	return 0;
>>> +	return VHOST_FEATURES;
>>> }
>>
>> This is not the topic of this series, but I wonder if it could make 
>> sense to be able to override supported features at device init time.
>
> Not quite sure I understood it correctly: is 
> rte_vhost_feature_disable() the answer you are looking for?
Not really.

I meant a per-device supported features, and something you could set also via the vhost PMD options, without needing to recompile.

But maybe it would make more sense to do it a guest level?

Maxime

-------Agreed, as you know we have the vhost PMD port and virtio-user PMD port and we can launch both vhost/virtio on the host, but we 
Can't set the mergeable in the command line, we may need add some feature settings for virtio-user interface. It will be much easier to 
Run tests or application. 

Qian
Maxime Coquelin Aug. 25, 2016, 9:10 a.m. UTC | #5 
On 08/25/2016 10:36 AM, Xu, Qian Q wrote:
>
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Thursday, August 25, 2016 3:19 PM
> To: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 7/7] vhost: simplify features set/get
>
>
>
> On 08/25/2016 05:03 AM, Yuanhan Liu wrote:
>> On Wed, Aug 24, 2016 at 10:11:57AM +0200, Maxime Coquelin wrote:
>>>
>>>
>>> On 08/18/2016 10:48 AM, Yuanhan Liu wrote:
>>>> No need to use a pointer to store/retrieve features.
>>>>
>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>> ---
>>>> lib/librte_vhost/vhost_user.c | 20 ++++++++------------
>>>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/lib/librte_vhost/vhost_user.c
>>>> b/lib/librte_vhost/vhost_user.c index ef4a0c1..eee99e9 100644
>>>> --- a/lib/librte_vhost/vhost_user.c
>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>> @@ -155,23 +155,22 @@ vhost_user_reset_owner(struct virtio_net *dev)
>>>> /*
>>>>  * The features that we support are requested.
>>>>  */
>>>> -static int
>>>> -vhost_user_get_features(uint64_t *pu)
>>>> +static uint64_t
>>>> +vhost_user_get_features(void)
>>>> {
>>>> -	*pu = VHOST_FEATURES;
>>>> -	return 0;
>>>> +	return VHOST_FEATURES;
>>>> }
>>>
>>> This is not the topic of this series, but I wonder if it could make
>>> sense to be able to override supported features at device init time.
>>
>> Not quite sure I understood it correctly: is
>> rte_vhost_feature_disable() the answer you are looking for?
> Not really.
>
> I meant a per-device supported features, and something you could set also via the vhost PMD options, without needing to recompile.
>
> But maybe it would make more sense to do it a guest level?
>
> Maxime
>
> -------Agreed, as you know we have the vhost PMD port and virtio-user PMD port and we can launch both vhost/virtio on the host, but we
> Can't set the mergeable in the command line, we may need add some feature settings for virtio-user interface. It will be much easier to
> Run tests or application.

I never tried virtio-user PMD on host, but my proposal should be
applicable there too.

Regards,
Maxime
Yuanhan Liu Aug. 26, 2016, 4:15 a.m. UTC | #6 
On Thu, Aug 25, 2016 at 09:18:30AM +0200, Maxime Coquelin wrote:
> 
> 
> On 08/25/2016 05:03 AM, Yuanhan Liu wrote:
> >On Wed, Aug 24, 2016 at 10:11:57AM +0200, Maxime Coquelin wrote:
> >>
> >>
> >>On 08/18/2016 10:48 AM, Yuanhan Liu wrote:
> >>>No need to use a pointer to store/retrieve features.
> >>>
> >>>Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>>---
> >>>lib/librte_vhost/vhost_user.c | 20 ++++++++------------
> >>>1 file changed, 8 insertions(+), 12 deletions(-)
> >>>
> >>>diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> >>>index ef4a0c1..eee99e9 100644
> >>>--- a/lib/librte_vhost/vhost_user.c
> >>>+++ b/lib/librte_vhost/vhost_user.c
> >>>@@ -155,23 +155,22 @@ vhost_user_reset_owner(struct virtio_net *dev)
> >>>/*
> >>> * The features that we support are requested.
> >>> */
> >>>-static int
> >>>-vhost_user_get_features(uint64_t *pu)
> >>>+static uint64_t
> >>>+vhost_user_get_features(void)
> >>>{
> >>>-	*pu = VHOST_FEATURES;
> >>>-	return 0;
> >>>+	return VHOST_FEATURES;
> >>>}
> >>
> >>This is not the topic of this series, but I wonder if it
> >>could make sense to be able to override supported features
> >>at device init time.
> >
> >Not quite sure I understood it correctly: is rte_vhost_feature_disable()
> >the answer you are looking for?
> Not really.
> 
> I meant a per-device supported features, and something you could set
> also via the vhost PMD options, without needing to recompile.

I see. I think my following proposal would fix it then.

    http://dpdk.org/ml/archives/dev/2016-June/040021.html

Note you might want to follow the discussion, as it mentioned
per-device config later.

Another note, it lacked a fancy name at last discussion. For
now, I think I come up with a better one: --dpdk-env :)

	--yliu

> 
> But maybe it would make more sense to do it a guest level?
> 
> Maxime
diff mbox

Patch

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index ef4a0c1..eee99e9 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -155,23 +155,22 @@  vhost_user_reset_owner(struct virtio_net *dev)
 /*
  * The features that we support are requested.
  */
-static int
-vhost_user_get_features(uint64_t *pu)
+static uint64_t
+vhost_user_get_features(void)
 {
-	*pu = VHOST_FEATURES;
-	return 0;
+	return VHOST_FEATURES;
 }
 
 /*
  * We receive the negotiated features supported by us and the virtio device.
  */
 static int
-vhost_user_set_features(struct virtio_net *dev, uint64_t *pu)
+vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 {
-	if (*pu & ~VHOST_FEATURES)
+	if (features & ~VHOST_FEATURES)
 		return -1;
 
-	dev->features = *pu;
+	dev->features = features;
 	if (dev->features &
 		((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
 		dev->vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
@@ -802,7 +801,6 @@  vhost_user_msg_handler(int vid, int fd)
 {
 	struct virtio_net *dev;
 	struct VhostUserMsg msg;
-	uint64_t features = 0;
 	int ret;
 
 	dev = get_device(vid);
@@ -828,14 +826,12 @@  vhost_user_msg_handler(int vid, int fd)
 		vhost_message_str[msg.request]);
 	switch (msg.request) {
 	case VHOST_USER_GET_FEATURES:
-		ret = vhost_user_get_features(&features);
-		msg.payload.u64 = features;
+		msg.payload.u64 = vhost_user_get_features();
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_message(fd, &msg);
 		break;
 	case VHOST_USER_SET_FEATURES:
-		features = msg.payload.u64;
-		vhost_user_set_features(dev, &features);
+		vhost_user_set_features(dev, msg.payload.u64);
 		break;
 
 	case VHOST_USER_GET_PROTOCOL_FEATURES: