DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [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-dev] [PATCH] vhost: fix checking of device features
  2017-06-28 12:40 ` [dpdk-dev] [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   ` Tan, Jianfeng
  2017-07-01 23:36   ` 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-dev] [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-dev] [PATCH] vhost: fix checking of device features
  2017-06-28 12:40 ` [dpdk-dev] [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   ` 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-dev] [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-dev] [PATCH] vhost: fix checking of device features
  2017-06-29  6:13   ` 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-dev] [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-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-dev] [PATCH] vhost: fix checking of device features
  2017-06-28 12:40 ` [dpdk-dev] [PATCH] vhost: fix checking of device features Ivan Dyukov
  2017-06-28 12:54   ` Maxime Coquelin
  2017-06-29  6:13   ` 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-dev] [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   ` Tan, Jianfeng
2017-06-29  7:27     ` Maxime Coquelin
2017-06-29  8:05       ` Tan, Jianfeng
2017-07-01 23:36   ` 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).