* [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] [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] [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-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] [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] [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).