diff mbox

[dpdk-dev] vhost: fix checking of device features

Message ID 1498653631-15307-1-git-send-email-i.dyukov@samsung.com (mailing list archive)
State Accepted, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Ivan Dyukov June 28, 2017, 12:40 p.m. UTC 
  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(-)

Comments

Maxime Coquelin June 28, 2017, 12:54 p.m. UTC | #1 
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;
>
Ivan Dyukov June 29, 2017, 6:07 a.m. UTC | #2 
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;
>>
>
>
>
>
Jianfeng Tan June 29, 2017, 6:13 a.m. UTC | #3 
> -----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
Maxime Coquelin June 29, 2017, 7:21 a.m. UTC | #4 
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
Maxime Coquelin June 29, 2017, 7:27 a.m. UTC | #5 
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
>
Maxime Coquelin June 29, 2017, 7:37 a.m. UTC | #6 
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
Jianfeng Tan June 29, 2017, 8:05 a.m. UTC | #7 
> -----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

> >
Yuanhan Liu July 1, 2017, 11:36 p.m. UTC | #8 
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
diff mbox

Patch

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;