From: "Ding, Xuan" <xuan.ding@intel.com>
To: David Marchand <david.marchand@redhat.com>
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>,
"Xia, Chenbo" <chenbo.xia@intel.com>, dev <dev@dpdk.org>,
"Hu, Jiayu" <jiayu.hu@intel.com>,
"Jiang, Cheng1" <cheng1.jiang@intel.com>,
"Pai G, Sunil" <sunil.pai.g@intel.com>,
"liangma@liangbit.com" <liangma@liangbit.com>,
"Wang, YuanX" <yuanx.wang@intel.com>,
"Mcnamara, John" <john.mcnamara@intel.com>
Subject: RE: [PATCH v8 4/5] vhost: support async dequeue for split ring
Date: Fri, 17 Jun 2022 06:34:31 +0000 [thread overview]
Message-ID: <BN9PR11MB551354043CCECD7C56D98391E7AF9@BN9PR11MB5513.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAJFAV8xyyn7rAQ5pN994h9vda+82L2sJ-M1pjgEfUQw50zHc-w@mail.gmail.com>
Hi David,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, June 16, 2022 10:40 PM
> To: Ding, Xuan <xuan.ding@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; dev <dev@dpdk.org>; Hu, Jiayu
> <jiayu.hu@intel.com>; Jiang, Cheng1 <cheng1.jiang@intel.com>; Pai G, Sunil
> <sunil.pai.g@intel.com>; liangma@liangbit.com; Wang, YuanX
> <yuanx.wang@intel.com>; Mcnamara, John <john.mcnamara@intel.com>
> Subject: Re: [PATCH v8 4/5] vhost: support async dequeue for split ring
>
> On Thu, Jun 16, 2022 at 4:38 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Mon, May 16, 2022 at 1:16 PM <xuan.ding@intel.com> wrote:
> > > +static __rte_always_inline uint16_t
> > > +virtio_dev_tx_async_split(struct virtio_net *dev, struct vhost_virtqueue
> *vq,
> > > + struct rte_mempool *mbuf_pool, struct rte_mbuf **pkts,
> uint16_t count,
> > > + int16_t dma_id, uint16_t vchan_id, bool
> > > +legacy_ol_flags) {
> > > + static bool allocerr_warned;
> > > + bool dropped = false;
> > > + uint16_t free_entries;
> > > + uint16_t pkt_idx, slot_idx = 0;
> > > + uint16_t nr_done_pkts = 0;
> > > + uint16_t pkt_err = 0;
> > > + uint16_t n_xfer;
> > > + struct vhost_async *async = vq->async;
> > > + struct async_inflight_info *pkts_info = async->pkts_info;
> > > + struct rte_mbuf *pkts_prealloc[MAX_PKT_BURST];
> >
> > Why do we need this array?
> > Plus, see blow.
> >
> > > + uint16_t pkts_size = count;
> > > +
> > > + /**
> > > + * The ordering between avail index and
> > > + * desc reads needs to be enforced.
> > > + */
> > > + free_entries = __atomic_load_n(&vq->avail->idx,
> __ATOMIC_ACQUIRE) -
> > > + vq->last_avail_idx;
> > > + if (free_entries == 0)
> > > + goto out;
> > > +
> > > + rte_prefetch0(&vq->avail->ring[vq->last_avail_idx &
> > > + (vq->size - 1)]);
> > > +
> > > + async_iter_reset(async);
> > > +
> > > + count = RTE_MIN(count, MAX_PKT_BURST);
>
> ^^^
> Ok, my point about the overflow does not stand.
> Just the pkts_prealloc array is probably useless.
Allocating a bulk of mbufs by rte_pktmbuf_alloc_bulk() is for performance consideration.
The pkts_prealloc array is for keeping a temporary variable to orderly update async_inflight_info,
which is required in async path.
>
> > > + count = RTE_MIN(count, free_entries);
> > > + VHOST_LOG_DATA(DEBUG, "(%s) about to dequeue %u buffers\n",
> > > + dev->ifname, count);
> > > +
> > > + if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts_prealloc, count))
> >
> > 'count' is provided by the user of the vhost async dequeue public API.
> > There is no check that it is not bigger than MAX_PKT_BURST.
> >
> > Calling rte_pktmbuf_alloc_bulk on a fixed-size array pkts_prealloc,
> > allocated on the stack, it may cause a stack overflow.
>
> The rest still stands for me.
> vvv
>
> >
> >
> >
> > This code is mostly copy/pasted from the "sync" code.
> > I see a fix on the stats has been sent.
I need to explain the fix here.
The async dequeue patches and stats patches were both sent out
and merged in this release, while stats patch requires changes in both sync/async enq
and deq. Sorry for not notice this change in the stats patch that was merged first, and
that is the reason for this bug.
> > I point here another bug.
> > There are probably more...
> >
> > <grmbl>
> > I don't like how async code has been added in the vhost library by Intel.
> >
> > Maxime did a cleanup on the enqueue patch
> >
> https://patchwork.dpdk.org/project/dpdk/list/?series=20020&state=%2A&ar
> chive=both.
> > I see that the recent dequeue path additions have the same method of
> > copying/pasting code and adding some branches in a non systematic way.
> > Please clean this code and stop copy/pasting without a valid reason.
> > </grmbl>
The cleanup for the code in dequeue patch was suggested in RFC v3.
https://patchwork.dpdk.org/project/dpdk/patch/20220310065407.17145-2-xuan.ding@intel.com/
With merging copy_desc_to_mbuf and async_desc_to_mbuf in a single function, and reuse the
fill_seg function, the code can be simplified without performance degradation.
Could you help to point out where in the current code that needs to be further cleaned?
Are you referring to the common parts of async deq API and sync deq API
can be abstracted into a function, such as ARAP?
https://patchwork.dpdk.org/project/dpdk/patch/20220516111041.63914-5-xuan.ding@intel.com/
There are code duplications between async deq API and sync deq API, these parts are both needed.
But the code clean of dequeue is by no means mindless copy/pasting.
Hope to get your insights.
Thanks,
Xuan
>
>
> --
> David Marchand
next prev parent reply other threads:[~2022-06-17 6:34 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-07 15:25 [PATCH v1 0/5] vhost: support async dequeue data path xuan.ding
2022-04-07 15:25 ` [PATCH v1 1/5] vhost: prepare sync for descriptor to mbuf refactoring xuan.ding
2022-04-07 15:25 ` [PATCH v1 2/5] vhost: prepare async " xuan.ding
2022-04-07 15:25 ` [PATCH v1 3/5] vhost: merge sync and async descriptor to mbuf filling xuan.ding
2022-04-07 15:25 ` [PATCH v1 4/5] vhost: support async dequeue for split ring xuan.ding
2022-04-07 15:25 ` [PATCH v1 5/5] examples/vhost: support async dequeue data path xuan.ding
2022-04-11 10:00 ` [PATCH v2 0/5] vhost: " xuan.ding
2022-04-11 10:00 ` [PATCH v2 1/5] vhost: prepare sync for descriptor to mbuf refactoring xuan.ding
2022-04-11 10:00 ` [PATCH v2 2/5] vhost: prepare async " xuan.ding
2022-04-11 10:00 ` [PATCH v2 3/5] vhost: merge sync and async descriptor to mbuf filling xuan.ding
2022-04-11 10:00 ` [PATCH v2 4/5] vhost: support async dequeue for split ring xuan.ding
2022-04-11 10:00 ` [PATCH v2 5/5] examples/vhost: support async dequeue data path xuan.ding
2022-04-19 3:43 ` [PATCH v3 0/5] vhost: " xuan.ding
2022-04-19 3:43 ` [PATCH v3 1/5] vhost: prepare sync for descriptor to mbuf refactoring xuan.ding
2022-04-22 15:30 ` Maxime Coquelin
2022-04-19 3:43 ` [PATCH v3 2/5] vhost: prepare async " xuan.ding
2022-04-22 15:32 ` Maxime Coquelin
2022-04-19 3:43 ` [PATCH v3 3/5] vhost: merge sync and async descriptor to mbuf filling xuan.ding
2022-04-22 11:06 ` David Marchand
2022-04-22 15:46 ` Maxime Coquelin
2022-04-24 2:02 ` Ding, Xuan
2022-04-22 15:43 ` Maxime Coquelin
2022-04-19 3:43 ` [PATCH v3 4/5] vhost: support async dequeue for split ring xuan.ding
2022-04-19 3:43 ` [PATCH v3 5/5] examples/vhost: support async dequeue data path xuan.ding
2022-05-05 6:23 ` [PATCH v4 0/5] vhost: " xuan.ding
2022-05-05 6:23 ` [PATCH v4 1/5] vhost: prepare sync for descriptor to mbuf refactoring xuan.ding
2022-05-05 7:37 ` Yang, YvonneX
2022-05-05 6:23 ` [PATCH v4 2/5] vhost: prepare async " xuan.ding
2022-05-05 7:38 ` Yang, YvonneX
2022-05-05 6:23 ` [PATCH v4 3/5] vhost: merge sync and async descriptor to mbuf filling xuan.ding
2022-05-05 7:39 ` Yang, YvonneX
2022-05-05 6:23 ` [PATCH v4 4/5] vhost: support async dequeue for split ring xuan.ding
2022-05-05 7:40 ` Yang, YvonneX
2022-05-05 19:36 ` Maxime Coquelin
2022-05-05 6:23 ` [PATCH v4 5/5] examples/vhost: support async dequeue data path xuan.ding
2022-05-05 7:39 ` Yang, YvonneX
2022-05-05 19:38 ` Maxime Coquelin
2022-05-05 19:52 ` [PATCH v4 0/5] vhost: " Maxime Coquelin
2022-05-06 1:49 ` Ding, Xuan
2022-05-13 2:00 ` [PATCH v5 " xuan.ding
2022-05-13 2:00 ` [PATCH v5 1/5] vhost: prepare sync for descriptor to mbuf refactoring xuan.ding
2022-05-13 2:00 ` [PATCH v5 2/5] vhost: prepare async " xuan.ding
2022-05-13 2:00 ` [PATCH v5 3/5] vhost: merge sync and async descriptor to mbuf filling xuan.ding
2022-05-13 2:00 ` [PATCH v5 4/5] vhost: support async dequeue for split ring xuan.ding
2022-05-13 2:24 ` Stephen Hemminger
2022-05-13 2:33 ` Ding, Xuan
2022-05-13 2:00 ` [PATCH v5 5/5] examples/vhost: support async dequeue data path xuan.ding
2022-05-13 2:50 ` [PATCH v6 0/5] vhost: " xuan.ding
2022-05-13 2:50 ` [PATCH v6 1/5] vhost: prepare sync for descriptor to mbuf refactoring xuan.ding
2022-05-13 2:50 ` [PATCH v6 2/5] vhost: prepare async " xuan.ding
2022-05-13 2:50 ` [PATCH v6 3/5] vhost: merge sync and async descriptor to mbuf filling xuan.ding
2022-05-13 2:50 ` [PATCH v6 4/5] vhost: support async dequeue for split ring xuan.ding
2022-05-13 2:50 ` [PATCH v6 5/5] examples/vhost: support async dequeue data path xuan.ding
2022-05-13 3:27 ` Xia, Chenbo
2022-05-13 3:51 ` Ding, Xuan
2022-05-16 2:43 ` [PATCH v7 0/5] vhost: " xuan.ding
2022-05-16 2:43 ` [PATCH v7 1/5] vhost: prepare sync for descriptor to mbuf refactoring xuan.ding
2022-05-16 2:43 ` [PATCH v7 2/5] vhost: prepare async " xuan.ding
2022-05-16 2:43 ` [PATCH v7 3/5] vhost: merge sync and async descriptor to mbuf filling xuan.ding
2022-05-16 2:43 ` [PATCH v7 4/5] vhost: support async dequeue for split ring xuan.ding
2022-05-16 5:52 ` Hu, Jiayu
2022-05-16 6:10 ` Ding, Xuan
2022-05-16 2:43 ` [PATCH v7 5/5] examples/vhost: support async dequeue data path xuan.ding
2022-05-16 11:10 ` [PATCH v8 0/5] vhost: " xuan.ding
2022-05-16 11:10 ` [PATCH v8 1/5] vhost: prepare sync for descriptor to mbuf refactoring xuan.ding
2022-05-16 11:10 ` [PATCH v8 2/5] vhost: prepare async " xuan.ding
2022-05-16 11:10 ` [PATCH v8 3/5] vhost: merge sync and async descriptor to mbuf filling xuan.ding
2022-05-16 11:10 ` [PATCH v8 4/5] vhost: support async dequeue for split ring xuan.ding
2022-06-16 14:38 ` David Marchand
2022-06-16 14:40 ` David Marchand
2022-06-17 6:34 ` Ding, Xuan [this message]
2022-05-16 11:10 ` [PATCH v8 5/5] examples/vhost: support async dequeue data path xuan.ding
2022-05-17 13:22 ` [PATCH v8 0/5] vhost: " 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=BN9PR11MB551354043CCECD7C56D98391E7AF9@BN9PR11MB5513.namprd11.prod.outlook.com \
--to=xuan.ding@intel.com \
--cc=chenbo.xia@intel.com \
--cc=cheng1.jiang@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=jiayu.hu@intel.com \
--cc=john.mcnamara@intel.com \
--cc=liangma@liangbit.com \
--cc=maxime.coquelin@redhat.com \
--cc=sunil.pai.g@intel.com \
--cc=yuanx.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).