From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5C01CA2EFC for ; Tue, 15 Oct 2019 20:44:37 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 45FCD1ECED; Tue, 15 Oct 2019 20:44:36 +0200 (CEST) Received: from sysclose.org (smtp.sysclose.org [69.164.214.230]) by dpdk.org (Postfix) with ESMTP id 5D8841ECDD for ; Tue, 15 Oct 2019 20:44:34 +0200 (CEST) Received: by sysclose.org (Postfix, from userid 5001) id 29DC66795; Tue, 15 Oct 2019 18:45:00 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 sysclose.org 29DC66795 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sysclose.org; s=201903; t=1571165100; bh=nUuaK93Cl4sftMFdq4F9Ek+QtxLRt+IT4dAqysnG5IE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=kEZIe1wZFsDTN+8mprj5GPBC2oxlYw46aEioAtKSrIZJlTSnj4vEUiwBt/ms2iWc2 QWVWiSMYV+q8K1Do3AbLYhbVkFqJoVnYp9MoYY6ZCBCdb+ZqRsuoQ3EDfBl0P3/Yaf X1BlyQ/uJDJsibWC5edRTCdT9ZfJRSUE/VaRA2KjWg+f4zNBU64DoV9jVZwumZfNtb gL3vgV2GBSu2rVH8ATgANVOHq4PjSVLGDYKYW7nJIFiNzxYFaWtTMGMY0e455jFqr/ cCSdmSddMR4dLtg5L5lY5cOUzjIvxct/jlSHiSnUPeL0RYjr+vrJNfaAyFi/i4GNUc d69xajD/gKbhg== X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on mail.sysclose.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from p50.lan (unknown [177.183.215.210]) by sysclose.org (Postfix) with ESMTPSA id B7B116509; Tue, 15 Oct 2019 18:44:57 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 sysclose.org B7B116509 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sysclose.org; s=201903; t=1571165099; bh=nUuaK93Cl4sftMFdq4F9Ek+QtxLRt+IT4dAqysnG5IE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=XI46y4uzHvLbZ1xWCoeu1qIhggHSPdIpquCC8DXUmWQXDK2VJM4+/3BrgATnZG9OZ 5dV91mT4tZidgL+ckhU7pKiq2lKY6DbPhirPemnAF9nWteuv7LUrDYbOQTyaIGUrkf ovsskBNyuvDyU9sKHXgKiBEhPIzF9HnNM8WDWajB/WplN8X8ac28QG28bG0gJHcFHO VvsRqjlVIlDPwCbnbZ9Juge8p0pMoTt1NrFz/mfOm1pPifF4cqCqQ+S+rhJKV2A0eG 9wtDTQo2/qCD4R2Q7hHuBb5fAxknDrmbWetwm4X4mydCqgGYHmCAzL521ag0cKkCFb qOKeNuP99wu9g== Date: Tue, 15 Oct 2019 15:44:25 -0300 From: Flavio Leitner To: Ilya Maximets Cc: dev@dpdk.org, Maxime Coquelin , Shahaf Shuler , David Marchand , Tiwei Bie , Obrembski MichalX , Stokes Ian Message-ID: <20191015154425.75beb9f8@p50.lan> In-Reply-To: References: <20191011170947.30656-1-fbl@sysclose.org> <20191015161727.32570-1-fbl@sysclose.org> X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4] vhost: add support for large buffers 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, 15 Oct 2019 19:41:52 +0200 Ilya Maximets 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