All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] virtio: enable indirect descriptors feature
@ 2016-09-02 15:55 Pierre Pfister (ppfister)
  2016-09-05  2:20 ` Yuanhan Liu
  0 siblings, 1 reply; 11+ messages in thread
From: Pierre Pfister (ppfister) @ 2016-09-02 15:55 UTC (permalink / raw)
  To: dev; +Cc: Pierre Pfister (ppfister)

Indirect descriptors support was disabled by commit 59d593f472a7,
presumably by accident as it was correctly supported.

This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
the supported features bit mask, hence enabling the use of
indirect descriptors when the feature is negociated with the
device.

Signed-off-by: Pierre Pfister <ppfister@cisco.com>
---
 drivers/net/virtio/virtio_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index 2ecec6e..31c91a5 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -63,6 +63,7 @@
         1u << VIRTIO_NET_F_CTRL_RX       |     \
         1u << VIRTIO_NET_F_CTRL_VLAN     |     \
         1u << VIRTIO_NET_F_MRG_RXBUF     |     \
+        1u << VIRTIO_RING_F_INDIRECT_DESC |    \
         1ULL << VIRTIO_F_VERSION_1)

 /*
--
2.7.4 (Apple Git-66)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] virtio: enable indirect descriptors feature
  2016-09-02 15:55 [PATCH] virtio: enable indirect descriptors feature Pierre Pfister (ppfister)
@ 2016-09-05  2:20 ` Yuanhan Liu
  2016-09-05  6:49   ` Pierre Pfister (ppfister)
  2016-09-05  6:52   ` [PATCH v2] " Pierre Pfister (ppfister)
  0 siblings, 2 replies; 11+ messages in thread
From: Yuanhan Liu @ 2016-09-05  2:20 UTC (permalink / raw)
  To: Pierre Pfister (ppfister); +Cc: dev

On Fri, Sep 02, 2016 at 03:55:24PM +0000, Pierre Pfister (ppfister) wrote:
> Indirect descriptors support was disabled by commit 59d593f472a7,

Hi,

I failed to find this commit:

    $ git show 59d593f472a7
    fatal: ambiguous argument '59d593f472a7': unknown revision or path not in the working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'

	--yliu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] virtio: enable indirect descriptors feature
  2016-09-05  2:20 ` Yuanhan Liu
@ 2016-09-05  6:49   ` Pierre Pfister (ppfister)
  2016-09-05  6:52   ` [PATCH v2] " Pierre Pfister (ppfister)
  1 sibling, 0 replies; 11+ messages in thread
From: Pierre Pfister (ppfister) @ 2016-09-05  6:49 UTC (permalink / raw)
  To: Yuanhan Liu; +Cc: dev


> Le 5 sept. 2016 à 04:20, Yuanhan Liu <yuanhan.liu@linux.intel.com> a écrit :
> 
> On Fri, Sep 02, 2016 at 03:55:24PM +0000, Pierre Pfister (ppfister) wrote:
>> Indirect descriptors support was disabled by commit 59d593f472a7,
> 
> Hi,
> 
> I failed to find this commit:
> 
>    $ git show 59d593f472a7
>    fatal: ambiguous argument '59d593f472a7': unknown revision or path not in the working tree.
>    Use '--' to separate paths from revisions, like this:
>    'git <command> [<revision>...] -- [<file>...]'

Oops !
I don't know where this one comes from.
I will fix this and push a v2.

Cheers,

- Pierre

> 
> 	--yliu


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] virtio: enable indirect descriptors feature
  2016-09-05  2:20 ` Yuanhan Liu
  2016-09-05  6:49   ` Pierre Pfister (ppfister)
@ 2016-09-05  6:52   ` Pierre Pfister (ppfister)
  2016-09-05 14:24     ` Maxime Coquelin
  1 sibling, 1 reply; 11+ messages in thread
From: Pierre Pfister (ppfister) @ 2016-09-05  6:52 UTC (permalink / raw)
  To: Yuanhan Liu; +Cc: dev

Indirect descriptors support was disabled by commit 4a92b67151be11,
presumably by accident as it was correctly supported.

This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
the supported features bit mask, hence enabling the use of
indirect descriptors when the feature is negociated with the
device.

Signed-off-by: Pierre Pfister <ppfister@cisco.com>
---
 drivers/net/virtio/virtio_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index 2ecec6e..31c91a5 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -63,6 +63,7 @@
         1u << VIRTIO_NET_F_CTRL_RX       |     \
         1u << VIRTIO_NET_F_CTRL_VLAN     |     \
         1u << VIRTIO_NET_F_MRG_RXBUF     |     \
+        1u << VIRTIO_RING_F_INDIRECT_DESC |    \
         1ULL << VIRTIO_F_VERSION_1)

 /*
--
2.7.4 (Apple Git-66)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] virtio: enable indirect descriptors feature
  2016-09-05  6:52   ` [PATCH v2] " Pierre Pfister (ppfister)
@ 2016-09-05 14:24     ` Maxime Coquelin
  2016-09-05 21:08       ` Stephen Hemminger
  0 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2016-09-05 14:24 UTC (permalink / raw)
  To: Pierre Pfister (ppfister), Yuanhan Liu; +Cc: dev

Thanks Pierre for sending the fix.

Minor comments below:

On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote:
> Indirect descriptors support was disabled by commit 4a92b67151be11,
> presumably by accident as it was correctly supported.
>
> This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
> the supported features bit mask, hence enabling the use of
> indirect descriptors when the feature is negociated with the
> device.
>

You should add the below line:
Fixes: 4a92b671 ("virtio: clarify feature bit handling")

Also, maybe we should consider add stable@dpdk.org in cc:,
because the regression was introduced before v16.07 final tag.
But the problem is that all the final validation has been done
without this feature enabled, and it impact quite a few lines of
code in Virtio PMD.

Other than that, you can add:
Reviewed-by: Maxime Coquelin <maxime.coquelin@¶edhat.com>

> Signed-off-by: Pierre Pfister <ppfister@cisco.com>
> ---
>  drivers/net/virtio/virtio_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index 2ecec6e..31c91a5 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -63,6 +63,7 @@
>          1u << VIRTIO_NET_F_CTRL_RX       |     \
>          1u << VIRTIO_NET_F_CTRL_VLAN     |     \
>          1u << VIRTIO_NET_F_MRG_RXBUF     |     \
> +        1u << VIRTIO_RING_F_INDIRECT_DESC |    \
>          1ULL << VIRTIO_F_VERSION_1)
>
>  /*
> --
> 2.7.4 (Apple Git-66)
>


One off-topic question, do you measure some improvement in perfs using
the feature? If yes, could you describe the use-case, and the figures?
I ask because I have implemented TX indirect descriptor in vhost lib
(see http://dpdk.org/dev/patchwork/patch/14797/), and failed to see
some use-case benefiting of it.

Regards,
Maxime

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] virtio: enable indirect descriptors feature
  2016-09-05 14:24     ` Maxime Coquelin
@ 2016-09-05 21:08       ` Stephen Hemminger
  2016-09-06  6:49         ` Pierre Pfister (ppfister)
  2016-09-06  7:44         ` [PATCH v2] " Maxime Coquelin
  0 siblings, 2 replies; 11+ messages in thread
From: Stephen Hemminger @ 2016-09-05 21:08 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: Pierre Pfister (ppfister), Yuanhan Liu, dev

On Mon, 5 Sep 2016 16:24:13 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> Thanks Pierre for sending the fix.
> 
> Minor comments below:
> 
> On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote:
> > Indirect descriptors support was disabled by commit 4a92b67151be11,
> > presumably by accident as it was correctly supported.
> >
> > This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
> > the supported features bit mask, hence enabling the use of
> > indirect descriptors when the feature is negociated with the
> > device.
> >  
> 
> You should add the below line:
> Fixes: 4a92b671 ("virtio: clarify feature bit handling")
> 
> Also, maybe we should consider add stable@dpdk.org in cc:,
> because the regression was introduced before v16.07 final tag.
> But the problem is that all the final validation has been done
> without this feature enabled, and it impact quite a few lines of
> code in Virtio PMD.
> 
> Other than that, you can add:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@¶edhat.com>

The patch is correct, but it doesn't fix a regression.

The original virtio DPDK did not support INDIRECT descriptors at all.
The original code in virtio_negotiate features was the inverse of what it is now.
Read carefully, the values in mask were the bits that were rejected during
guest negotiation at the time.

 static void
 virtio_negotiate_features(struct virtio_hw *hw)
 {
-	uint32_t host_features, mask;
-
-	/* checksum offload not implemented */
-	mask = VIRTIO_NET_F_CSUM | VIRTIO_NET_F_GUEST_CSUM;
-
-	/* TSO and LRO are only available when their corresponding
-	 * checksum offload feature is also negotiated.
-	 */
-	mask |= VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NET_F_HOST_ECN;
-	mask |= VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_NET_F_GUEST_ECN;
-	mask |= VTNET_LRO_FEATURES;
-
-	/* not negotiating INDIRECT descriptor table support */
-	mask |= VIRTIO_RING_F_INDIRECT_DESC;
+	uint32_t host_features;
 
 	/* Prepare guest_features: feature that driver wants to support */
-	hw->guest_features = VTNET_FEATURES & ~mask;
+	hw->guest_features = VIRTIO_PMD_GUEST_FEATURES;
 	PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %x",
 		hw->guest_features);
 
Therefore INDIRECT descriptors were always disabled!  Don't blame any commit.
Use of indirect descriptors by DPDK did not happen until a later change.

commit 6dc5de3a6aefba3946fe04368d93994db3f7a5fd
Author: Stephen Hemminger <stephen@networkplumber.org>
Date:   Fri Mar 4 10:19:19 2016 -0800

    virtio: use indirect ring elements
    
    The virtio ring in QEMU/KVM is usually limited to 256 entries
    and the normal way that virtio driver was queuing mbufs required
    nsegs + 1 ring elements. By using the indirect ring element feature
    if available, each packet will take only one ring slot even for
    multi-segment packets.
    
    Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
    Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
    Acked-by: Huawei Xie <huawei.xie@intel.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] virtio: enable indirect descriptors feature
  2016-09-05 21:08       ` Stephen Hemminger
@ 2016-09-06  6:49         ` Pierre Pfister (ppfister)
  2016-09-06 15:32           ` Stephen Hemminger
  2016-09-06  7:44         ` [PATCH v2] " Maxime Coquelin
  1 sibling, 1 reply; 11+ messages in thread
From: Pierre Pfister (ppfister) @ 2016-09-06  6:49 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Maxime Coquelin, Yuanhan Liu, dev


> Le 5 sept. 2016 à 23:08, Stephen Hemminger <stephen@networkplumber.org> a écrit :
> 
> On Mon, 5 Sep 2016 16:24:13 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> 
>> Thanks Pierre for sending the fix.
>> 
>> Minor comments below:
>> 
>> On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote:
>>> Indirect descriptors support was disabled by commit 4a92b67151be11,
>>> presumably by accident as it was correctly supported.
>>> 
>>> This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
>>> the supported features bit mask, hence enabling the use of
>>> indirect descriptors when the feature is negociated with the
>>> device.
>>> 
>> 
>> You should add the below line:
>> Fixes: 4a92b671 ("virtio: clarify feature bit handling")
>> 
>> Also, maybe we should consider add stable@dpdk.org in cc:,
>> because the regression was introduced before v16.07 final tag.
>> But the problem is that all the final validation has been done
>> without this feature enabled, and it impact quite a few lines of
>> code in Virtio PMD.
>> 
>> Other than that, you can add:
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@¶edhat.com>
> 
> The patch is correct, but it doesn't fix a regression.
> 
> The original virtio DPDK did not support INDIRECT descriptors at all.
> The original code in virtio_negotiate features was the inverse of what it is now.
> Read carefully, the values in mask were the bits that were rejected during
> guest negotiation at the time.
> 
> static void
> virtio_negotiate_features(struct virtio_hw *hw)
> {
> -	uint32_t host_features, mask;
> -
> -	/* checksum offload not implemented */
> -	mask = VIRTIO_NET_F_CSUM | VIRTIO_NET_F_GUEST_CSUM;
> -
> -	/* TSO and LRO are only available when their corresponding
> -	 * checksum offload feature is also negotiated.
> -	 */
> -	mask |= VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NET_F_HOST_ECN;
> -	mask |= VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_NET_F_GUEST_ECN;
> -	mask |= VTNET_LRO_FEATURES;
> -
> -	/* not negotiating INDIRECT descriptor table support */
> -	mask |= VIRTIO_RING_F_INDIRECT_DESC;
> +	uint32_t host_features;
> 
> 	/* Prepare guest_features: feature that driver wants to support */
> -	hw->guest_features = VTNET_FEATURES & ~mask;
> +	hw->guest_features = VIRTIO_PMD_GUEST_FEATURES;
> 	PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %x",
> 		hw->guest_features);
> 
> Therefore INDIRECT descriptors were always disabled!  Don't blame any commit.
> Use of indirect descriptors by DPDK did not happen until a later change.

Oops. My bad, sorry. I will remove the blame from the commit message.
It is strange though that 6dc5de3a6aef adds indirect buffer support but does not
provide a way to enable it.

> 
> commit 6dc5de3a6aefba3946fe04368d93994db3f7a5fd
> Author: Stephen Hemminger <stephen@networkplumber.org>
> Date:   Fri Mar 4 10:19:19 2016 -0800
> 
>    virtio: use indirect ring elements
> 
>    The virtio ring in QEMU/KVM is usually limited to 256 entries
>    and the normal way that virtio driver was queuing mbufs required
>    nsegs + 1 ring elements. By using the indirect ring element feature
>    if available, each packet will take only one ring slot even for
>    multi-segment packets.
> 
>    Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>    Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>    Acked-by: Huawei Xie <huawei.xie@intel.com>
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] virtio: enable indirect descriptors feature
  2016-09-05 21:08       ` Stephen Hemminger
  2016-09-06  6:49         ` Pierre Pfister (ppfister)
@ 2016-09-06  7:44         ` Maxime Coquelin
  1 sibling, 0 replies; 11+ messages in thread
From: Maxime Coquelin @ 2016-09-06  7:44 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Pierre Pfister (ppfister), Yuanhan Liu, dev



On 09/05/2016 11:08 PM, Stephen Hemminger wrote:
> On Mon, 5 Sep 2016 16:24:13 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
>> Thanks Pierre for sending the fix.
>>
>> Minor comments below:
>>
>> On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote:
>>> Indirect descriptors support was disabled by commit 4a92b67151be11,
>>> presumably by accident as it was correctly supported.
>>>
>>> This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
>>> the supported features bit mask, hence enabling the use of
>>> indirect descriptors when the feature is negociated with the
>>> device.
>>>
>>
>> You should add the below line:
>> Fixes: 4a92b671 ("virtio: clarify feature bit handling")
>>
>> Also, maybe we should consider add stable@dpdk.org in cc:,
>> because the regression was introduced before v16.07 final tag.
>> But the problem is that all the final validation has been done
>> without this feature enabled, and it impact quite a few lines of
>> code in Virtio PMD.
>>
>> Other than that, you can add:
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@¶edhat.com>
>
> The patch is correct, but it doesn't fix a regression.
>
> The original virtio DPDK did not support INDIRECT descriptors at all.
> The original code in virtio_negotiate features was the inverse of what it is now.
> Read carefully, the values in mask were the bits that were rejected during
> guest negotiation at the time.
>
>  static void
>  virtio_negotiate_features(struct virtio_hw *hw)
>  {
> -	uint32_t host_features, mask;
> -
> -	/* checksum offload not implemented */
> -	mask = VIRTIO_NET_F_CSUM | VIRTIO_NET_F_GUEST_CSUM;
> -
> -	/* TSO and LRO are only available when their corresponding
> -	 * checksum offload feature is also negotiated.
> -	 */
> -	mask |= VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NET_F_HOST_ECN;
> -	mask |= VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_NET_F_GUEST_ECN;
> -	mask |= VTNET_LRO_FEATURES;
> -
> -	/* not negotiating INDIRECT descriptor table support */
> -	mask |= VIRTIO_RING_F_INDIRECT_DESC;
> +	uint32_t host_features;
>
>  	/* Prepare guest_features: feature that driver wants to support */
> -	hw->guest_features = VTNET_FEATURES & ~mask;
> +	hw->guest_features = VIRTIO_PMD_GUEST_FEATURES;
>  	PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %x",
>  		hw->guest_features);
>
> Therefore INDIRECT descriptors were always disabled!  Don't blame any commit.
> Use of indirect descriptors by DPDK did not happen until a later change.
Oh, sorry, I read commit pointed out by Pierre quite too quickly.
So this is definitively not to land into stable tree.
>
> commit 6dc5de3a6aefba3946fe04368d93994db3f7a5fd
> Author: Stephen Hemminger <stephen@networkplumber.org>
> Date:   Fri Mar 4 10:19:19 2016 -0800
>
>     virtio: use indirect ring elements
>
>     The virtio ring in QEMU/KVM is usually limited to 256 entries
>     and the normal way that virtio driver was queuing mbufs required
>     nsegs + 1 ring elements. By using the indirect ring element feature
>     if available, each packet will take only one ring slot even for
>     multi-segment packets.
>
>     Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>     Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>     Acked-by: Huawei Xie <huawei.xie@intel.com>
>
>
I second Pierre on his question, why was it not enabled?
In current state, it just adds overhead to Tx path.
And when enabled, do you see some gain? in which use-case?

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] virtio: enable indirect descriptors feature
  2016-09-06  6:49         ` Pierre Pfister (ppfister)
@ 2016-09-06 15:32           ` Stephen Hemminger
  2016-09-06 16:09             ` [PATCH v3] " Pierre Pfister (ppfister)
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2016-09-06 15:32 UTC (permalink / raw)
  To: Pierre Pfister (ppfister); +Cc: Maxime Coquelin, Yuanhan Liu, dev

On Tue, 6 Sep 2016 06:49:26 +0000
"Pierre Pfister (ppfister)" <ppfister@cisco.com> wrote:

> > Le 5 sept. 2016 à 23:08, Stephen Hemminger <stephen@networkplumber.org> a écrit :
> > 
> > On Mon, 5 Sep 2016 16:24:13 +0200
> > Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> >   
> >> Thanks Pierre for sending the fix.
> >> 
> >> Minor comments below:
> >> 
> >> On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote:  
> >>> Indirect descriptors support was disabled by commit 4a92b67151be11,
> >>> presumably by accident as it was correctly supported.
> >>> 
> >>> This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
> >>> the supported features bit mask, hence enabling the use of
> >>> indirect descriptors when the feature is negociated with the
> >>> device.
> >>>   
> >> 
> >> You should add the below line:
> >> Fixes: 4a92b671 ("virtio: clarify feature bit handling")
> >> 
> >> Also, maybe we should consider add stable@dpdk.org in cc:,
> >> because the regression was introduced before v16.07 final tag.
> >> But the problem is that all the final validation has been done
> >> without this feature enabled, and it impact quite a few lines of
> >> code in Virtio PMD.
> >> 
> >> Other than that, you can add:
> >> Reviewed-by: Maxime Coquelin <maxime.coquelin@¶edhat.com>  
> > 
> > The patch is correct, but it doesn't fix a regression.
> > 
> > The original virtio DPDK did not support INDIRECT descriptors at all.
> > The original code in virtio_negotiate features was the inverse of what it is now.
> > Read carefully, the values in mask were the bits that were rejected during
> > guest negotiation at the time.
> > 
> > static void
> > virtio_negotiate_features(struct virtio_hw *hw)
> > {
> > -	uint32_t host_features, mask;
> > -
> > -	/* checksum offload not implemented */
> > -	mask = VIRTIO_NET_F_CSUM | VIRTIO_NET_F_GUEST_CSUM;
> > -
> > -	/* TSO and LRO are only available when their corresponding
> > -	 * checksum offload feature is also negotiated.
> > -	 */
> > -	mask |= VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NET_F_HOST_ECN;
> > -	mask |= VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_NET_F_GUEST_ECN;
> > -	mask |= VTNET_LRO_FEATURES;
> > -
> > -	/* not negotiating INDIRECT descriptor table support */
> > -	mask |= VIRTIO_RING_F_INDIRECT_DESC;
> > +	uint32_t host_features;
> > 
> > 	/* Prepare guest_features: feature that driver wants to support */
> > -	hw->guest_features = VTNET_FEATURES & ~mask;
> > +	hw->guest_features = VIRTIO_PMD_GUEST_FEATURES;
> > 	PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %x",
> > 		hw->guest_features);
> > 
> > Therefore INDIRECT descriptors were always disabled!  Don't blame any commit.
> > Use of indirect descriptors by DPDK did not happen until a later change.  
> 
> Oops. My bad, sorry. I will remove the blame from the commit message.
> It is strange though that 6dc5de3a6aef adds indirect buffer support but does not
> provide a way to enable it.

I agree that indirect descriptors should be negotiated. Probably the lack of
the bit is left over from testing the three ways to send.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] virtio: enable indirect descriptors feature
  2016-09-06 15:32           ` Stephen Hemminger
@ 2016-09-06 16:09             ` Pierre Pfister (ppfister)
  2016-09-07  2:57               ` Yuanhan Liu
  0 siblings, 1 reply; 11+ messages in thread
From: Pierre Pfister (ppfister) @ 2016-09-06 16:09 UTC (permalink / raw)
  To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, Stephen Hemminger

>From be1210e77f0f9072ccb8e6970552596b6780a44c Mon Sep 17 00:00:00 2001
From: Pierre Pfister <ppfister@cisco.com>
Date: Fri, 2 Sep 2016 16:24:57 +0200
Subject: [PATCH] virtio: enable indirect descriptors feature

Virtio indirect descriptors are supported by the data-path
but the feature bit is never set during feature negociation.

This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
the supported features bit mask, hence enabling the use of
indirect descriptors when the feature is negociated with the
device.

Signed-off-by: Pierre Pfister <ppfister@cisco.com>
---
 drivers/net/virtio/virtio_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index 2ecec6e..31c91a5 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -63,6 +63,7 @@
         1u << VIRTIO_NET_F_CTRL_RX       |     \
         1u << VIRTIO_NET_F_CTRL_VLAN     |     \
         1u << VIRTIO_NET_F_MRG_RXBUF     |     \
+        1u << VIRTIO_RING_F_INDIRECT_DESC |    \
         1ULL << VIRTIO_F_VERSION_1)

 /*
--
2.7.4 (Apple Git-66)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] virtio: enable indirect descriptors feature
  2016-09-06 16:09             ` [PATCH v3] " Pierre Pfister (ppfister)
@ 2016-09-07  2:57               ` Yuanhan Liu
  0 siblings, 0 replies; 11+ messages in thread
From: Yuanhan Liu @ 2016-09-07  2:57 UTC (permalink / raw)
  To: Pierre Pfister (ppfister); +Cc: dev, Maxime Coquelin, Stephen Hemminger

On Tue, Sep 06, 2016 at 04:09:39PM +0000, Pierre Pfister (ppfister) wrote:
> >From be1210e77f0f9072ccb8e6970552596b6780a44c Mon Sep 17 00:00:00 2001
> From: Pierre Pfister <ppfister@cisco.com>
> Date: Fri, 2 Sep 2016 16:24:57 +0200
> Subject: [PATCH] virtio: enable indirect descriptors feature
> 
> Virtio indirect descriptors are supported by the data-path
> but the feature bit is never set during feature negociation.
> 
> This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to
> the supported features bit mask, hence enabling the use of
> indirect descriptors when the feature is negociated with the
> device.

Applied to dpdk-next-virtio, with a patch format fix stated below.

> 
> Signed-off-by: Pierre Pfister <ppfister@cisco.com>
> ---
>  drivers/net/virtio/virtio_ethdev.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index 2ecec6e..31c91a5 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -63,6 +63,7 @@
>          1u << VIRTIO_NET_F_CTRL_RX       |     \
>          1u << VIRTIO_NET_F_CTRL_VLAN     |     \
>          1u << VIRTIO_NET_F_MRG_RXBUF     |     \
> +        1u << VIRTIO_RING_F_INDIRECT_DESC |    \
>          1ULL << VIRTIO_F_VERSION_1)

Note that DPDK uses TAB but not 8 space chars. For that reason, your
patch is not appliable.

Thanks.

	--yliu

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-09-07  2:45 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-02 15:55 [PATCH] virtio: enable indirect descriptors feature Pierre Pfister (ppfister)
2016-09-05  2:20 ` Yuanhan Liu
2016-09-05  6:49   ` Pierre Pfister (ppfister)
2016-09-05  6:52   ` [PATCH v2] " Pierre Pfister (ppfister)
2016-09-05 14:24     ` Maxime Coquelin
2016-09-05 21:08       ` Stephen Hemminger
2016-09-06  6:49         ` Pierre Pfister (ppfister)
2016-09-06 15:32           ` Stephen Hemminger
2016-09-06 16:09             ` [PATCH v3] " Pierre Pfister (ppfister)
2016-09-07  2:57               ` Yuanhan Liu
2016-09-06  7:44         ` [PATCH v2] " Maxime Coquelin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.