[dpdk-dev,7/7] vhost: simplify features set/get
Commit Message
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
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
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
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
-----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
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
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
@@ -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: