* [dpdk-stable] [PATCH] vhost: fix checking of device features
[not found] <CGME20170628124049eucas1p1688178d88249ae416d653abfc19d0478@eucas1p1.samsung.com>
@ 2017-06-28 12:40 ` Ivan Dyukov
2017-06-28 12:54 ` Maxime Coquelin
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ivan Dyukov @ 2017-06-28 12:40 UTC (permalink / raw)
To: yliu, maxime.coquelin, dev; +Cc: i.maximets, heetae82.ahn, Ivan Dyukov, stable
To compare enabled features in current device we must use bit
mask instead of bit position.
CC: stable@dpdk.org
Fixes: c843af3aa13e ("vhost: access header only")
Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
---
lib/librte_vhost/virtio_net.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index ebfda1c..4fae4c1 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -601,9 +601,11 @@ static inline bool
virtio_net_with_host_offload(struct virtio_net *dev)
{
if (dev->features &
- (VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
- VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 |
- VIRTIO_NET_F_HOST_UFO))
+ ((1ULL << VIRTIO_NET_F_CSUM) |
+ (1ULL << VIRTIO_NET_F_HOST_ECN) |
+ (1ULL << VIRTIO_NET_F_HOST_TSO4) |
+ (1ULL << VIRTIO_NET_F_HOST_TSO6) |
+ (1ULL << VIRTIO_NET_F_HOST_UFO)))
return true;
return false;
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-stable] [PATCH] vhost: fix checking of device features
2017-06-28 12:40 ` [dpdk-stable] [PATCH] vhost: fix checking of device features Ivan Dyukov
@ 2017-06-28 12:54 ` Maxime Coquelin
2017-06-29 6:07 ` Ivan Dyukov
2017-06-29 6:13 ` [dpdk-stable] [dpdk-dev] " Tan, Jianfeng
2017-07-01 23:36 ` [dpdk-stable] " Yuanhan Liu
2 siblings, 1 reply; 9+ messages in thread
From: Maxime Coquelin @ 2017-06-28 12:54 UTC (permalink / raw)
To: Ivan Dyukov, yliu, dev; +Cc: i.maximets, heetae82.ahn, stable
On 06/28/2017 02:40 PM, Ivan Dyukov wrote:
> To compare enabled features in current device we must use bit
> mask instead of bit position.
>
> CC: stable@dpdk.org
> Fixes: c843af3aa13e ("vhost: access header only")
>
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
> lib/librte_vhost/virtio_net.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
Thanks for the fix Ivan, and sorry for introducing this bug.
Out of curiosity, did you noticed it because it broke offloading,
or just by code review?
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index ebfda1c..4fae4c1 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -601,9 +601,11 @@ static inline bool
> virtio_net_with_host_offload(struct virtio_net *dev)
> {
> if (dev->features &
> - (VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
> - VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 |
> - VIRTIO_NET_F_HOST_UFO))
> + ((1ULL << VIRTIO_NET_F_CSUM) |
> + (1ULL << VIRTIO_NET_F_HOST_ECN) |
> + (1ULL << VIRTIO_NET_F_HOST_TSO4) |
> + (1ULL << VIRTIO_NET_F_HOST_TSO6) |
> + (1ULL << VIRTIO_NET_F_HOST_UFO)))
> return true;
>
> return false;
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-stable] [PATCH] vhost: fix checking of device features
2017-06-28 12:54 ` Maxime Coquelin
@ 2017-06-29 6:07 ` Ivan Dyukov
2017-06-29 7:21 ` Maxime Coquelin
0 siblings, 1 reply; 9+ messages in thread
From: Ivan Dyukov @ 2017-06-29 6:07 UTC (permalink / raw)
To: Maxime Coquelin, yliu, dev; +Cc: i.maximets, heetae82.ahn, stable
On 06/28/2017 03:54 PM, Maxime Coquelin wrote:
>
>
> On 06/28/2017 02:40 PM, Ivan Dyukov wrote:
>> To compare enabled features in current device we must use bit
>> mask instead of bit position.
>>
>> CC: stable@dpdk.org
>> Fixes: c843af3aa13e ("vhost: access header only")
>>
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>> ---
>> lib/librte_vhost/virtio_net.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> Thanks for the fix Ivan, and sorry for introducing this bug.
> Out of curiosity, did you noticed it because it broke offloading,
> or just by code review?
I didn't see any breakages. It's just code review.
>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
>> diff --git a/lib/librte_vhost/virtio_net.c
>> b/lib/librte_vhost/virtio_net.c
>> index ebfda1c..4fae4c1 100644
>> --- a/lib/librte_vhost/virtio_net.c
>> +++ b/lib/librte_vhost/virtio_net.c
>> @@ -601,9 +601,11 @@ static inline bool
>> virtio_net_with_host_offload(struct virtio_net *dev)
>> {
>> if (dev->features &
>> - (VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
>> - VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 |
>> - VIRTIO_NET_F_HOST_UFO))
>> + ((1ULL << VIRTIO_NET_F_CSUM) |
>> + (1ULL << VIRTIO_NET_F_HOST_ECN) |
>> + (1ULL << VIRTIO_NET_F_HOST_TSO4) |
>> + (1ULL << VIRTIO_NET_F_HOST_TSO6) |
>> + (1ULL << VIRTIO_NET_F_HOST_UFO)))
>> return true;
>> return false;
>>
>
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] vhost: fix checking of device features
2017-06-28 12:40 ` [dpdk-stable] [PATCH] vhost: fix checking of device features Ivan Dyukov
2017-06-28 12:54 ` Maxime Coquelin
@ 2017-06-29 6:13 ` Tan, Jianfeng
2017-06-29 7:27 ` Maxime Coquelin
2017-07-01 23:36 ` [dpdk-stable] " Yuanhan Liu
2 siblings, 1 reply; 9+ messages in thread
From: Tan, Jianfeng @ 2017-06-29 6:13 UTC (permalink / raw)
To: Ivan Dyukov, yliu, maxime.coquelin, dev; +Cc: i.maximets, heetae82.ahn, stable
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
> Sent: Wednesday, June 28, 2017 8:41 PM
> To: yliu@fridaylinux.org; maxime.coquelin@redhat.com; dev@dpdk.org
> Cc: i.maximets@samsung.com; heetae82.ahn@samsung.com; Ivan Dyukov;
> stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] vhost: fix checking of device features
>
> To compare enabled features in current device we must use bit
> mask instead of bit position.
>
> CC: stable@dpdk.org
> Fixes: c843af3aa13e ("vhost: access header only")
>
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
> lib/librte_vhost/virtio_net.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index ebfda1c..4fae4c1 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -601,9 +601,11 @@ static inline bool
> virtio_net_with_host_offload(struct virtio_net *dev)
> {
> if (dev->features &
> - (VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
> - VIRTIO_NET_F_HOST_TSO4 |
> VIRTIO_NET_F_HOST_TSO6 |
> - VIRTIO_NET_F_HOST_UFO))
> + ((1ULL << VIRTIO_NET_F_CSUM) |
> + (1ULL << VIRTIO_NET_F_HOST_ECN) |
> + (1ULL << VIRTIO_NET_F_HOST_TSO4) |
> + (1ULL << VIRTIO_NET_F_HOST_TSO6) |
> + (1ULL << VIRTIO_NET_F_HOST_UFO)))
Another problem in this piece of code, we don't support VIRTIO_NET_F_HOST_ECN and VIRTIO_NET_F_HOST_UFO in vhost-user. We might consider to remove those two lines?
Thanks,
Jianfeng
> return true;
>
> return false;
> --
> 2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-stable] [PATCH] vhost: fix checking of device features
2017-06-29 6:07 ` Ivan Dyukov
@ 2017-06-29 7:21 ` Maxime Coquelin
2017-06-29 7:37 ` Maxime Coquelin
0 siblings, 1 reply; 9+ messages in thread
From: Maxime Coquelin @ 2017-06-29 7:21 UTC (permalink / raw)
To: Ivan Dyukov, yliu, dev; +Cc: i.maximets, heetae82.ahn, stable
On 06/29/2017 08:07 AM, Ivan Dyukov wrote:
> On 06/28/2017 03:54 PM, Maxime Coquelin wrote:
>>
>>
>> On 06/28/2017 02:40 PM, Ivan Dyukov wrote:
>>> To compare enabled features in current device we must use bit
>>> mask instead of bit position.
>>>
>>> CC: stable@dpdk.org
>>> Fixes: c843af3aa13e ("vhost: access header only")
>>>
>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>> ---
>>> lib/librte_vhost/virtio_net.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> Thanks for the fix Ivan, and sorry for introducing this bug.
>> Out of curiosity, did you noticed it because it broke offloading,
>> or just by code review?
> I didn't see any breakages. It's just code review.
Ok, thanks.
Maxime
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] vhost: fix checking of device features
2017-06-29 6:13 ` [dpdk-stable] [dpdk-dev] " Tan, Jianfeng
@ 2017-06-29 7:27 ` Maxime Coquelin
2017-06-29 8:05 ` Tan, Jianfeng
0 siblings, 1 reply; 9+ messages in thread
From: Maxime Coquelin @ 2017-06-29 7:27 UTC (permalink / raw)
To: Tan, Jianfeng, Ivan Dyukov, yliu, dev; +Cc: i.maximets, heetae82.ahn, stable
On 06/29/2017 08:13 AM, Tan, Jianfeng wrote:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
>> Sent: Wednesday, June 28, 2017 8:41 PM
>> To: yliu@fridaylinux.org; maxime.coquelin@redhat.com; dev@dpdk.org
>> Cc: i.maximets@samsung.com; heetae82.ahn@samsung.com; Ivan Dyukov;
>> stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH] vhost: fix checking of device features
>>
>> To compare enabled features in current device we must use bit
>> mask instead of bit position.
>>
>> CC: stable@dpdk.org
>> Fixes: c843af3aa13e ("vhost: access header only")
>>
>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>> ---
>> lib/librte_vhost/virtio_net.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
>> index ebfda1c..4fae4c1 100644
>> --- a/lib/librte_vhost/virtio_net.c
>> +++ b/lib/librte_vhost/virtio_net.c
>> @@ -601,9 +601,11 @@ static inline bool
>> virtio_net_with_host_offload(struct virtio_net *dev)
>> {
>> if (dev->features &
>> - (VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
>> - VIRTIO_NET_F_HOST_TSO4 |
>> VIRTIO_NET_F_HOST_TSO6 |
>> - VIRTIO_NET_F_HOST_UFO))
>> + ((1ULL << VIRTIO_NET_F_CSUM) |
>> + (1ULL << VIRTIO_NET_F_HOST_ECN) |
>> + (1ULL << VIRTIO_NET_F_HOST_TSO4) |
>> + (1ULL << VIRTIO_NET_F_HOST_TSO6) |
>> + (1ULL << VIRTIO_NET_F_HOST_UFO)))
>
> Another problem in this piece of code, we don't support VIRTIO_NET_F_HOST_ECN and VIRTIO_NET_F_HOST_UFO in vhost-user. We might consider to remove those two lines?
It is not really a problem as the feature is never negotiated as not
supported, it would just be a clean-up.
I think we should stick with this version as it targets also -stable.
Another patch could be sent on top to remove these unsupported feature
bits.
Thanks,
Maxime
> Thanks,
> Jianfeng
>
>
>> return true;
>>
>> return false;
>> --
>> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-stable] [PATCH] vhost: fix checking of device features
2017-06-29 7:21 ` Maxime Coquelin
@ 2017-06-29 7:37 ` Maxime Coquelin
0 siblings, 0 replies; 9+ messages in thread
From: Maxime Coquelin @ 2017-06-29 7:37 UTC (permalink / raw)
To: Ivan Dyukov, yliu, dev; +Cc: i.maximets, heetae82.ahn, stable
On 06/29/2017 09:21 AM, Maxime Coquelin wrote:
>
>
> On 06/29/2017 08:07 AM, Ivan Dyukov wrote:
>> On 06/28/2017 03:54 PM, Maxime Coquelin wrote:
>>>
>>>
>>> On 06/28/2017 02:40 PM, Ivan Dyukov wrote:
>>>> To compare enabled features in current device we must use bit
>>>> mask instead of bit position.
>>>>
>>>> CC: stable@dpdk.org
>>>> Fixes: c843af3aa13e ("vhost: access header only")
>>>>
>>>> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
>>>> ---
>>>> lib/librte_vhost/virtio_net.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> Thanks for the fix Ivan, and sorry for introducing this bug.
>>> Out of curiosity, did you noticed it because it broke offloading,
>>> or just by code review?
>> I didn't see any breakages. It's just code review.
>
> Ok, thanks.
FYI, I just found another case in vhost.c, sending patch soon.
Cheers,
Maxime
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] vhost: fix checking of device features
2017-06-29 7:27 ` Maxime Coquelin
@ 2017-06-29 8:05 ` Tan, Jianfeng
0 siblings, 0 replies; 9+ messages in thread
From: Tan, Jianfeng @ 2017-06-29 8:05 UTC (permalink / raw)
To: Maxime Coquelin, Ivan Dyukov, yliu, dev; +Cc: i.maximets, heetae82.ahn, stable
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, June 29, 2017 3:27 PM
> To: Tan, Jianfeng; Ivan Dyukov; yliu@fridaylinux.org; dev@dpdk.org
> Cc: i.maximets@samsung.com; heetae82.ahn@samsung.com;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: fix checking of device features
>
>
>
> On 06/29/2017 08:13 AM, Tan, Jianfeng wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ivan Dyukov
> >> Sent: Wednesday, June 28, 2017 8:41 PM
> >> To: yliu@fridaylinux.org; maxime.coquelin@redhat.com; dev@dpdk.org
> >> Cc: i.maximets@samsung.com; heetae82.ahn@samsung.com; Ivan
> Dyukov;
> >> stable@dpdk.org
> >> Subject: [dpdk-dev] [PATCH] vhost: fix checking of device features
> >>
> >> To compare enabled features in current device we must use bit
> >> mask instead of bit position.
> >>
> >> CC: stable@dpdk.org
> >> Fixes: c843af3aa13e ("vhost: access header only")
> >>
> >> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> >> ---
> >> lib/librte_vhost/virtio_net.c | 8 +++++---
> >> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> >> index ebfda1c..4fae4c1 100644
> >> --- a/lib/librte_vhost/virtio_net.c
> >> +++ b/lib/librte_vhost/virtio_net.c
> >> @@ -601,9 +601,11 @@ static inline bool
> >> virtio_net_with_host_offload(struct virtio_net *dev)
> >> {
> >> if (dev->features &
> >> - (VIRTIO_NET_F_CSUM | VIRTIO_NET_F_HOST_ECN |
> >> - VIRTIO_NET_F_HOST_TSO4 |
> >> VIRTIO_NET_F_HOST_TSO6 |
> >> - VIRTIO_NET_F_HOST_UFO))
> >> + ((1ULL << VIRTIO_NET_F_CSUM) |
> >> + (1ULL << VIRTIO_NET_F_HOST_ECN) |
> >> + (1ULL << VIRTIO_NET_F_HOST_TSO4) |
> >> + (1ULL << VIRTIO_NET_F_HOST_TSO6) |
> >> + (1ULL << VIRTIO_NET_F_HOST_UFO)))
> >
> > Another problem in this piece of code, we don't support
> VIRTIO_NET_F_HOST_ECN and VIRTIO_NET_F_HOST_UFO in vhost-user. We
> might consider to remove those two lines?
>
> It is not really a problem as the feature is never negotiated as not
> supported, it would just be a clean-up.
Yes, it's a clean-up to avoid confusion.
>
> I think we should stick with this version as it targets also -stable.
>
> Another patch could be sent on top to remove these unsupported feature
> bits.
Agreed.
Thanks,
Jianfeng
>
> Thanks,
> Maxime
>
> > Thanks,
> > Jianfeng
> >
> >
> >> return true;
> >>
> >> return false;
> >> --
> >> 2.7.4
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-stable] [PATCH] vhost: fix checking of device features
2017-06-28 12:40 ` [dpdk-stable] [PATCH] vhost: fix checking of device features Ivan Dyukov
2017-06-28 12:54 ` Maxime Coquelin
2017-06-29 6:13 ` [dpdk-stable] [dpdk-dev] " Tan, Jianfeng
@ 2017-07-01 23:36 ` Yuanhan Liu
2 siblings, 0 replies; 9+ messages in thread
From: Yuanhan Liu @ 2017-07-01 23:36 UTC (permalink / raw)
To: Ivan Dyukov; +Cc: maxime.coquelin, dev, i.maximets, heetae82.ahn, stable
On Wed, Jun 28, 2017 at 03:40:31PM +0300, Ivan Dyukov wrote:
> To compare enabled features in current device we must use bit
> mask instead of bit position.
>
> CC: stable@dpdk.org
> Fixes: c843af3aa13e ("vhost: access header only")
>
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
Applied to dpdk-next-virtio.
Thanks.
--yliu
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-01 23:36 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20170628124049eucas1p1688178d88249ae416d653abfc19d0478@eucas1p1.samsung.com>
2017-06-28 12:40 ` [dpdk-stable] [PATCH] vhost: fix checking of device features Ivan Dyukov
2017-06-28 12:54 ` Maxime Coquelin
2017-06-29 6:07 ` Ivan Dyukov
2017-06-29 7:21 ` Maxime Coquelin
2017-06-29 7:37 ` Maxime Coquelin
2017-06-29 6:13 ` [dpdk-stable] [dpdk-dev] " Tan, Jianfeng
2017-06-29 7:27 ` Maxime Coquelin
2017-06-29 8:05 ` Tan, Jianfeng
2017-07-01 23:36 ` [dpdk-stable] " Yuanhan Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).