From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <jfreimann@redhat.com>
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
by dpdk.org (Postfix) with ESMTP id 85FA7A49
for <dev@dpdk.org>; Fri, 21 Sep 2018 14:37:37 +0200 (CEST)
Received: from smtp.corp.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com
[10.5.11.26])
(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
(No client certificate requested)
by mx1.redhat.com (Postfix) with ESMTPS id D0DD6C050E00;
Fri, 21 Sep 2018 12:37:36 +0000 (UTC)
Received: from localhost (dhcp-192-209.str.redhat.com [10.33.192.209])
by smtp.corp.redhat.com (Postfix) with ESMTPS id AA03A3091385;
Fri, 21 Sep 2018 12:37:33 +0000 (UTC)
Date: Fri, 21 Sep 2018 14:37:32 +0200
From: Jens Freimann <jfreimann@redhat.com>
To: Tiwei Bie <tiwei.bie@intel.com>
Cc: dev@dpdk.org, maxime.coquelin@redhat.com, Gavin.Hu@arm.com,
zhihong.wang@intel.com
Message-ID: <20180921123732.ftt7weewxxe753o2@jenstp.localdomain>
References: <20180921103308.16357-1-jfreimann@redhat.com>
<20180921103308.16357-7-jfreimann@redhat.com>
<20180921122658.GA24906@debian>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20180921122658.GA24906@debian>
User-Agent: NeoMutt/20180716
X-Scanned-By: MIMEDefang 2.84 on 10.5.11.26
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16
(mx1.redhat.com [10.5.110.32]); Fri, 21 Sep 2018 12:37:36 +0000 (UTC)
Subject: Re: [dpdk-dev] [PATCH v6 06/11] net/virtio: implement transmit path
for packed queues
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 21 Sep 2018 12:37:37 -0000
On Fri, Sep 21, 2018 at 08:26:58PM +0800, Tiwei Bie wrote:
>On Fri, Sep 21, 2018 at 12:33:03PM +0200, Jens Freimann wrote:
>[...]
>>
>> static inline int
>> -desc_is_used(struct vring_desc_packed *desc, struct vring *vr)
>> +_desc_is_used(struct vring_desc_packed *desc)
>> {
>> uint16_t used, avail;
>>
>> used = !!(desc->flags & VRING_DESC_F_USED(1));
>> avail = !!(desc->flags & VRING_DESC_F_AVAIL(1));
>>
>> - return used == avail && used == vr->used_wrap_counter;
>> + return used == avail;
>> +
>> +}
>> +
>> +static inline int
>> +desc_is_used(struct vring_desc_packed *desc, struct vring *vr)
>> +{
>> + uint16_t used;
>> +
>> + used = !!(desc->flags & VRING_DESC_F_USED(1));
>> +
>> + return _desc_is_used(desc) && used == vr->used_wrap_counter;
>> }
>>
>> /* The standard layout for the ring is a continuous chunk of memory which
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index eb891433e..ea6300563 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -38,6 +38,7 @@
>> #define VIRTIO_DUMP_PACKET(m, len) do { } while (0)
>> #endif
>>
>> +
>> int
>> virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
>> {
>> @@ -165,6 +166,31 @@ virtqueue_dequeue_rx_inorder(struct virtqueue *vq,
>> #endif
>>
>> /* Cleanup from completed transmits. */
>> +static void
>> +virtio_xmit_cleanup_packed(struct virtqueue *vq)
>> +{
>> + uint16_t idx;
>> + uint16_t size = vq->vq_nentries;
>> + struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
>> + struct vq_desc_extra *dxp;
>> +
>> + idx = vq->vq_used_cons_idx;
>> + while (_desc_is_used(&desc[idx]) &&
>
>We can't just compare the AVAIL bit and USED bit to
>check whether a desc is used.
We can't compare with the current wrap counter value as well
because it won't match the flags in descriptors. So check against
used_wrap_counter ^= 1 then?
>
>> + vq->vq_free_cnt < size) {
>> + dxp = &vq->vq_descx[idx];
>
>The code is still assuming the descs will be written
>back by device in order. The vq->vq_descx[] needs to
>be managed e.g. as a list to support the out-of-order
>processing. IOW, we can't assume vq->vq_descx[idx]
>is corresponding to desc[idx] when device may write
>back the descs out of order.
I changed it to not assume this in other spots but missed this one. I
will check more carefully and add code to make vq_descx entries a list.
Thanks for the review!
regards,
Jens