From: Tiwei Bie <tiwei.bie@intel.com>
To: Jens Freimann <jfreimann@redhat.com>
Cc: maxime.coquelin@redhat.com, zhihong.wang@intel.com, dev@dpdk.org
Subject: Re: [PATCH 04/10] net/virtio: optimize flags update for packed ring
Date: Tue, 19 Mar 2019 17:37:34 +0800 [thread overview]
Message-ID: <20190319093734.GA24818@dpdk-tbie.sh.intel.com> (raw)
In-Reply-To: <20190319085403.gnamedd5pz7jzwvj@jenstp.localdomain>
On Tue, Mar 19, 2019 at 09:54:03AM +0100, Jens Freimann wrote:
> On Tue, Mar 19, 2019 at 02:43:06PM +0800, Tiwei Bie wrote:
> > Cache the AVAIL, USED and WRITE bits to avoid calculating
> > them as much as possible. Note that, the WRITE bit isn't
> > cached for control queue.
> >
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> > drivers/net/virtio/virtio_ethdev.c | 35 ++++++++++++++----------------
> > drivers/net/virtio/virtio_rxtx.c | 31 ++++++++++----------------
> > drivers/net/virtio/virtqueue.h | 8 +++----
> > 3 files changed, 32 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index ff16fb63e..9060b6b33 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -149,7 +149,7 @@ virtio_send_command_packed(struct virtnet_ctl *cvq,
> > int head;
> > struct vring_packed_desc *desc = vq->ring_packed.desc_packed;
> > struct virtio_pmd_ctrl *result;
> > - bool avail_wrap_counter;
> > + uint16_t flags;
> > int sum = 0;
> > int nb_descs = 0;
> > int k;
> > @@ -161,14 +161,15 @@ virtio_send_command_packed(struct virtnet_ctl *cvq,
> > * One RX packet for ACK.
> > */
> > head = vq->vq_avail_idx;
> > - avail_wrap_counter = vq->avail_wrap_counter;
> > + flags = vq->cached_flags;
> > desc[head].addr = cvq->virtio_net_hdr_mem;
> > desc[head].len = sizeof(struct virtio_net_ctrl_hdr);
> > vq->vq_free_cnt--;
> > nb_descs++;
> > if (++vq->vq_avail_idx >= vq->vq_nentries) {
> > vq->vq_avail_idx -= vq->vq_nentries;
> > - vq->avail_wrap_counter ^= 1;
> > + vq->cached_flags ^=
> > + VRING_DESC_F_AVAIL(1) | VRING_DESC_F_USED(1);
>
> Maybe name it avail_used_flags instead of cached flags. Also we could
> use a constant value.
It also contains the WRITE bit (not just AVAIL and USED bits)
for Rx path. That's why I didn't name it as avail_used_flags.
>
> > }
> >
> > for (k = 0; k < pkt_num; k++) {
> > @@ -177,34 +178,31 @@ virtio_send_command_packed(struct virtnet_ctl *cvq,
> > + sizeof(ctrl->status) + sizeof(uint8_t) * sum;
> > desc[vq->vq_avail_idx].len = dlen[k];
> > desc[vq->vq_avail_idx].flags = VRING_DESC_F_NEXT |
> > - VRING_DESC_F_AVAIL(vq->avail_wrap_counter) |
> > - VRING_DESC_F_USED(!vq->avail_wrap_counter);
> > + vq->cached_flags;
> > sum += dlen[k];
> > vq->vq_free_cnt--;
> > nb_descs++;
> > if (++vq->vq_avail_idx >= vq->vq_nentries) {
> > vq->vq_avail_idx -= vq->vq_nentries;
> > - vq->avail_wrap_counter ^= 1;
> > + vq->cached_flags ^=
> > + VRING_DESC_F_AVAIL(1) | VRING_DESC_F_USED(1);
> > }
> > }
> >
> > desc[vq->vq_avail_idx].addr = cvq->virtio_net_hdr_mem
> > + sizeof(struct virtio_net_ctrl_hdr);
> > desc[vq->vq_avail_idx].len = sizeof(ctrl->status);
> > - desc[vq->vq_avail_idx].flags = VRING_DESC_F_WRITE |
> > - VRING_DESC_F_AVAIL(vq->avail_wrap_counter) |
> > - VRING_DESC_F_USED(!vq->avail_wrap_counter);
> > + desc[vq->vq_avail_idx].flags = VRING_DESC_F_WRITE | vq->cached_flags;
> > vq->vq_free_cnt--;
> > nb_descs++;
> > if (++vq->vq_avail_idx >= vq->vq_nentries) {
> > vq->vq_avail_idx -= vq->vq_nentries;
> > - vq->avail_wrap_counter ^= 1;
> > + vq->cached_flags ^=
> > + VRING_DESC_F_AVAIL(1) | VRING_DESC_F_USED(1);
> > }
> >
> > virtio_wmb(vq->hw->weak_barriers);
> > - desc[head].flags = VRING_DESC_F_NEXT |
> > - VRING_DESC_F_AVAIL(avail_wrap_counter) |
> > - VRING_DESC_F_USED(!avail_wrap_counter);
> > + desc[head].flags = VRING_DESC_F_NEXT | flags;
> >
> > virtio_wmb(vq->hw->weak_barriers);
> > virtqueue_notify(vq);
> > @@ -226,12 +224,12 @@ virtio_send_command_packed(struct virtnet_ctl *cvq,
> > PMD_INIT_LOG(DEBUG, "vq->vq_free_cnt=%d\n"
> > "vq->vq_avail_idx=%d\n"
> > "vq->vq_used_cons_idx=%d\n"
> > - "vq->avail_wrap_counter=%d\n"
> > + "vq->cached_flags=0x%x\n"
> > "vq->used_wrap_counter=%d\n",
> > vq->vq_free_cnt,
> > vq->vq_avail_idx,
> > vq->vq_used_cons_idx,
> > - vq->avail_wrap_counter,
> > + vq->cached_flags,
> > vq->used_wrap_counter);
> >
> > result = cvq->virtio_net_hdr_mz->addr;
> > @@ -491,11 +489,10 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
> > vq->vq_nentries = vq_size;
> > vq->event_flags_shadow = 0;
> > if (vtpci_packed_queue(hw)) {
> > - vq->avail_wrap_counter = 1;
> > vq->used_wrap_counter = 1;
> > - vq->avail_used_flags =
> > - VRING_DESC_F_AVAIL(vq->avail_wrap_counter) |
> > - VRING_DESC_F_USED(!vq->avail_wrap_counter);
> > + vq->cached_flags = VRING_DESC_F_AVAIL(1);
> > + if (queue_type == VTNET_RQ)
> > + vq->cached_flags |= VRING_DESC_F_WRITE;
> > }
> >
> > /*
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index 771d3c3f6..3c354baef 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -431,7 +431,7 @@ virtqueue_enqueue_recv_refill_packed(struct virtqueue *vq,
> > struct rte_mbuf **cookie, uint16_t num)
> > {
> > struct vring_packed_desc *start_dp = vq->ring_packed.desc_packed;
> > - uint16_t flags = VRING_DESC_F_WRITE | vq->avail_used_flags;
> > + uint16_t flags = vq->cached_flags;
> > struct virtio_hw *hw = vq->hw;
> > struct vq_desc_extra *dxp;
> > uint16_t idx;
> > @@ -460,11 +460,9 @@ virtqueue_enqueue_recv_refill_packed(struct virtqueue *vq,
> > start_dp[idx].flags = flags;
> > if (++vq->vq_avail_idx >= vq->vq_nentries) {
> > vq->vq_avail_idx -= vq->vq_nentries;
> > - vq->avail_wrap_counter ^= 1;
> > - vq->avail_used_flags =
> > - VRING_DESC_F_AVAIL(vq->avail_wrap_counter) |
> > - VRING_DESC_F_USED(!vq->avail_wrap_counter);
> > - flags = VRING_DESC_F_WRITE | vq->avail_used_flags;
> > + vq->cached_flags ^=
> > + VRING_DESC_F_AVAIL(1) | VRING_DESC_F_USED(1);
> > + flags = vq->cached_flags;
>
> same here. it's not really cached, it's pre-calculated. And here we
> could also use a pre-calculated constand/define.
For pre-calculated constant/define, do you mean VRING_DESC_F_AVAIL(1)
and VRING_DESC_F_USED(1)? There is still little code in virtio-user
using them without constantly passing 1. I planned to fully get rid
of them in a separate patch later (but I can do it in this series if
anyone wants).
>
> Otherwise looks good! Did you see any performance improvements?
Yeah, I saw slightly better performance in a quick test.
Thanks,
Tiwei
>
>
> regards,
> Jens
next prev parent reply other threads:[~2019-03-19 9:37 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-19 6:43 [PATCH 00/10] net/virtio: cleanups and fixes for packed/split ring Tiwei Bie
2019-03-19 6:43 ` [PATCH 01/10] net/virtio: fix typo in packed ring init Tiwei Bie
2019-03-19 8:39 ` Jens Freimann
2019-03-19 12:46 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 02/10] net/virtio: fix interrupt helper for packed ring Tiwei Bie
2019-03-19 12:48 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 03/10] net/virtio: add missing barrier in interrupt enable Tiwei Bie
2019-03-19 12:49 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 04/10] net/virtio: optimize flags update for packed ring Tiwei Bie
2019-03-19 8:54 ` Jens Freimann
2019-03-19 9:37 ` Tiwei Bie [this message]
2019-03-19 10:11 ` Jens Freimann
2019-03-19 12:50 ` Maxime Coquelin
2019-03-19 12:58 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 05/10] net/virtio: refactor virtqueue structure Tiwei Bie
2019-03-19 9:44 ` Jens Freimann
2019-03-19 10:09 ` Tiwei Bie
2019-03-19 13:28 ` Maxime Coquelin
2019-03-19 13:47 ` Jens Freimann
2019-03-19 13:50 ` Maxime Coquelin
2019-03-19 14:59 ` Kevin Traynor
2019-03-20 4:40 ` Tiwei Bie
2019-03-20 17:50 ` Stephen Hemminger
2019-03-21 14:18 ` Maxime Coquelin
2019-03-20 4:35 ` Tiwei Bie
2019-03-19 13:28 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 06/10] net/virtio: drop redundant suffix in packed ring structure Tiwei Bie
2019-03-19 9:47 ` Jens Freimann
2019-03-19 13:29 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 07/10] net/virtio: drop unused field in Tx region structure Tiwei Bie
2019-03-19 9:51 ` Jens Freimann
2019-03-19 13:33 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 08/10] net/virtio: add interrupt helper for split ring Tiwei Bie
2019-03-19 9:53 ` Jens Freimann
2019-03-19 13:34 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 09/10] net/virtio: add ctrl vq " Tiwei Bie
2019-03-19 9:54 ` Jens Freimann
2019-03-19 13:54 ` Maxime Coquelin
2019-03-19 6:43 ` [PATCH 10/10] net/virtio: improve batching in standard Rx path Tiwei Bie
2019-03-19 10:04 ` Jens Freimann
2019-03-19 10:28 ` Tiwei Bie
2019-03-19 11:08 ` Maxime Coquelin
2019-03-19 14:15 ` Maxime Coquelin
2019-03-20 7:35 ` [PATCH 00/10] net/virtio: cleanups and fixes for packed/split ring Maxime Coquelin
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=20190319093734.GA24818@dpdk-tbie.sh.intel.com \
--to=tiwei.bie@intel.com \
--cc=dev@dpdk.org \
--cc=jfreimann@redhat.com \
--cc=maxime.coquelin@redhat.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.