From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id BC3911041 for ; Wed, 12 Sep 2018 10:26:02 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 479494023840; Wed, 12 Sep 2018 08:26:02 +0000 (UTC) Received: from [10.36.112.35] (ovpn-112-35.ams2.redhat.com [10.36.112.35]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7B46A202322B; Wed, 12 Sep 2018 08:25:59 +0000 (UTC) To: Jens Freimann , dev@dpdk.org Cc: tiwei.bie@intel.com References: <20180906181947.20646-1-jfreimann@redhat.com> <20180906181947.20646-4-jfreimann@redhat.com> From: Maxime Coquelin Message-ID: <3206e0ea-8c5c-60cc-6cab-6024b304da73@redhat.com> Date: Wed, 12 Sep 2018 10:25:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180906181947.20646-4-jfreimann@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Wed, 12 Sep 2018 08:26:02 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Wed, 12 Sep 2018 08:26:02 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v5 03/11] net/virtio: add packed virtqueue helpers 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: Wed, 12 Sep 2018 08:26:03 -0000 On 09/06/2018 08:19 PM, Jens Freimann wrote: > Add helper functions to set/clear and check descriptor flags. > > Signed-off-by: Jens Freimann > --- > drivers/net/virtio/virtio_ring.h | 26 ++++++++++++++++++++++++++ > drivers/net/virtio/virtqueue.h | 19 +++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h > index e2c597434..f3b23f419 100644 > --- a/drivers/net/virtio/virtio_ring.h > +++ b/drivers/net/virtio/virtio_ring.h > @@ -78,6 +78,8 @@ struct vring_packed_desc_event { > > struct vring { > unsigned int num; > + unsigned int avail_wrap_counter; > + unsigned int used_wrap_counter; > union { > struct vring_desc_packed *desc_packed; > struct vring_desc *desc; > @@ -92,6 +94,30 @@ struct vring { > }; > }; > > +static inline void > +_set_desc_avail(struct vring_desc_packed *desc, int wrap_counter) > +{ > + desc->flags |= VRING_DESC_F_AVAIL(wrap_counter) | > + VRING_DESC_F_USED(!wrap_counter); It implies the avail and used bits to be cleared beforehand. Maybe it would be safer to clear them in the helper? > +} > + > +static inline void > +set_desc_avail(struct vring *vr, struct vring_desc_packed *desc) > +{ > + _set_desc_avail(desc, vr->avail_wrap_counter); > +} > + > +static inline int > +desc_is_used(struct vring_desc_packed *desc, struct vring *vr) > +{ > + 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; > +} > + > /* The standard layout for the ring is a continuous chunk of memory which > * looks like this. We assume num is a power of 2. > * > diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h > index d2a0b651a..53fce61b4 100644 > --- a/drivers/net/virtio/virtqueue.h > +++ b/drivers/net/virtio/virtqueue.h > @@ -245,6 +245,25 @@ struct virtio_tx_region { > __attribute__((__aligned__(16))); > }; > > +static inline uint16_t > +increment_pq_index(uint16_t idx, size_t ring_size) > +{ > + return ++idx >= ring_size ? 0 : idx; > +} Not sure this helper is really useful, as it ends up doing two checks in a row. > + > +static inline uint16_t > +update_pq_avail_index(struct virtqueue *vq) > +{ > + uint16_t idx; > + > + idx = increment_pq_index(vq->vq_avail_idx, vq->vq_nentries); > + if (idx == 0) > + vq->vq_ring.avail_wrap_counter ^= 1; > + vq->vq_avail_idx = idx; > + > + return vq->vq_avail_idx; > +} So it could be simplified to: { if (++vq->vq_avail_idx >= vq->vq_entries) { vq->vq_avail_idx = 0; vq->vq_ring.avail_wrap_counter ^= 1; } return vq->vq_avail_idx; } > + > static inline void > vring_desc_init_packed(struct vring *vr, int n) > { >