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

[Stephen Hemminger <stephen@networkplumber.org>]

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>