DPDK patches and discussions
 help / color / mirror / Atom feed
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
Subject: Re: [dpdk-dev] [PATCH v6 06/11] net/virtio: implement transmit path for packed queues
Date: Fri, 21 Sep 2018 14:37:32 +0200	[thread overview]
Message-ID: <20180921123732.ftt7weewxxe753o2@jenstp.localdomain> (raw)
In-Reply-To: <20180921122658.GA24906@debian>

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 

  reply	other threads:[~2018-09-21 12:37 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-21 10:32 [dpdk-dev] [PATCH v6 00/11] implement packed virtqueues Jens Freimann
2018-09-21 10:32 ` [dpdk-dev] [PATCH v6 01/11] net/virtio: vring init for packed queues Jens Freimann
2018-09-21 10:32 ` [dpdk-dev] [PATCH v6 02/11] net/virtio: add packed virtqueue defines Jens Freimann
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 03/11] net/virtio: add packed virtqueue helpers Jens Freimann
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 04/11] net/virtio: flush packed receive virtqueues Jens Freimann
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 05/11] net/virtio: dump packed virtqueue data Jens Freimann
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 06/11] net/virtio: implement transmit path for packed queues Jens Freimann
2018-09-21 12:26   ` Tiwei Bie
2018-09-21 12:37     ` Jens Freimann [this message]
2018-09-21 12:49       ` Tiwei Bie
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 07/11] net/virtio: implement receive " Jens Freimann
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 08/11] net/virtio: add support for mergeable buffers with packed virtqueues Jens Freimann
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 09/11] net/virtio: add virtio send command packed queue support Jens Freimann
2018-09-21 12:37   ` Tiwei Bie
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 10/11] net/virtio-user: add option to use packed queues Jens Freimann
2018-09-21 10:33 ` [dpdk-dev] [PATCH v6 11/11] net/virtio: enable packed virtqueues by default Jens Freimann
2018-09-21 12:32 ` [dpdk-dev] [PATCH v6 00/11] implement packed virtqueues Tiwei Bie
2018-09-21 14:06   ` Jens Freimann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180921123732.ftt7weewxxe753o2@jenstp.localdomain \
    --to=jfreimann@redhat.com \
    --cc=Gavin.Hu@arm.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=tiwei.bie@intel.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).