Re: [dpdk-dev] [PATCH v4] vhost: add support for large buffers

[Flavio Leitner <fbl@sysclose.org>]

On Tue, 15 Oct 2019 19:41:52 +0200
Ilya Maximets <i.maximets@ovn.org> wrote:

> Hi.
> Not a full review.  Few comments inline.
> 
> Best regards, Ilya Maximets.

Thanks for reviewing Ilya, see below.

[...]
> > @@ -870,6 +878,8 @@ rte_vhost_driver_register(const char *path,
> > uint64_t flags) goto out_free;
> >   	}
> >   	vsocket->dequeue_zero_copy = flags &
> > RTE_VHOST_USER_DEQUEUE_ZERO_COPY;
> > +	vsocket->extbuf = flags & RTE_VHOST_USER_EXTBUF_SUPPORT;
> > +	vsocket->linearbuf = flags &
> > RTE_VHOST_USER_LINEARBUF_SUPPORT; 
> >   	/*
> >   	 * Set the supported features correctly for the builtin
> > vhost-user @@ -894,6 +904,18 @@ rte_vhost_driver_register(const
> > char *path, uint64_t flags)
> >   	 * not compatible with postcopy.
> >   	 */
> >   	if (vsocket->dequeue_zero_copy) {
> > +		if (vsocket->extbuf) {
> > +			RTE_LOG(ERR, VHOST_CONFIG,
> > +			"error: zero copy is incompatible with
> > external buffers\n");
> > +			ret = -1;
> > +			goto out_free;  
> 
> There should be 'out_mutex'.

Good catch, going to fix that.

> 
> > +		}
> > +		if (vsocket->linearbuf) {
> > +			RTE_LOG(ERR, VHOST_CONFIG,
> > +			"error: zero copy is incompatible with
> > linear buffers\n");
> > +			ret = -1;
> > +			goto out_free;  
> 
> Ditto.

Yeah

[...]
> > +/*
> > + * Allocate a host supported pktmbuf.
> > + */
> > +static __rte_always_inline struct rte_mbuf *
> > +virtio_dev_pktmbuf_alloc(struct virtio_net *dev, struct
> > rte_mempool *mp,
> > +			 uint32_t data_len)
> > +{
> > +	struct rte_mbuf *pkt = rte_pktmbuf_alloc(mp);
> > +
> > +	if (unlikely(pkt == NULL))
> > +		return NULL;
> > +
> > +	if (rte_pktmbuf_tailroom(pkt) >= data_len)
> > +		return pkt;
> > +
> > +	/* attach an external buffer if supported */
> > +	if (dev->extbuf && !virtio_dev_extbuf_alloc(pkt, data_len))
> > +		return pkt;
> > +
> > +	/* check if chained buffers are allowed */
> > +	if (!dev->linearbuf)
> > +		return pkt;  
> 
> I guess, checking of the 'linearbuf' should go before checking the
> 'extbuf'. The usecase is that allocation of several buffers from the
> memory pool is, probably, faster than rte_malloc() + memory
> attaching.  So, if the 'linearbuf' is not requested, it might be
> faster to use chained mbufs.

The speed seems to depend on the length of the packet and it is not
just the allocation, but the overall overhead to deal with multiple
segments. If the preferred way is chained buffers, then just don't
pass any of the new flags because that's the default behavior today.
 
> BTW, I'm not sure if we really need 2 separate options for this.
> i.e. 1. +linear +extbuf --> extbuf allocated
>       2. +linear -extbuf --> buffer dropped (is this case is useful
> at all?) 3. -linear +extbuf --> chained mbufs might be preferred (see
> above) 4. -linear -extbuf --> chained mbufs
> 
> Case 4 is a default case. 

Yes, in this case if the packet length fits into the pktmbuf, a single
linear mbuf is returned otherwise it uses chained mbufs.

>  Case 1 is our target case for supporting
> large buffers. 

Right, here it enables external buffers, but we don't want chained
mbufs.

> Case 3 might makes no much sense as the result is
> equal to case 4.

Well case #3 either uses a single mbuf from mpool or use an external
buffer for big packets while in case #4 there is no support for big
packets at all.

> Case 2 might be not interesting for as at all,

Case #2 says the app only supports what fits into a single mbuf and
no external buffer nor chained buffers is supported.

> because it will lead to random packet drops depending on their size.

Either that or wrong processing.

> But, if only cases 1 and 4 are valid and interesting to us, we could
> easily merge both flags.

For OvS use-case today yes, but I created different flags because they
seemed different useful features to me and changing flags later can get
the API very confusing.

Thanks!
fbl