Hi David,
> -----Original Message-----
> From: David Marchand <david.march...@redhat.com>
> Sent: Thursday, June 16, 2022 10:40 PM
> To: Ding, Xuan <xuan.d...@intel.com>
> Cc: Maxime Coquelin <maxime.coque...@redhat.com>; Xia, Chenbo
> <chenbo....@intel.com>; dev <dev@dpdk.org>; Hu, Jiayu
> <jiayu...@intel.com>; Jiang, Cheng1 <cheng1.ji...@intel.com>; Pai G, Sunil
> <sunil.pa...@intel.com>; lian...@liangbit.com; Wang, YuanX
> <yuanx.w...@intel.com>; Mcnamara, John <john.mcnam...@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.march...@redhat.com> wrote:
> >
> > On Mon, May 16, 2022 at 1:16 PM <xuan.d...@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.d...@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.d...@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
