From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 85FA7A49 for ; 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 To: Tiwei Bie 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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