> Le 5 sept. 2016 ? 23:08, Stephen Hemminger <stephen at networkplumber.org> a
> ?crit :
>
> On Mon, 5 Sep 2016 16:24:13 +0200
> Maxime Coquelin <maxime.coquelin at 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 at 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 at 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 at networkplumber.org>
> Acked-by: Yuanhan Liu <yuanhan.liu at linux.intel.com>
> Acked-by: Huawei Xie <huawei.xie at intel.com>
>
>
