Re: [dpdk-dev] [PATCH v2] virtio: enable indirect descriptors feature

["Pierre Pfister (ppfister)" <ppfister@cisco.com>]

> 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>
> 
>