From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Watson Subject: Re: [net-next v5 3/3] net/tls: Remove redundant array allocation. Date: Wed, 25 Jul 2018 14:01:03 -0700 Message-ID: <20180725210103.GA74476@davejwatson-mba.dhcp.thefacebook.com> References: <20180719162613.27184-1-vakul.garg@nxp.com> <20180719162613.27184-4-vakul.garg@nxp.com> <20180721.192532.520509909556885779.davem@davemloft.net> <20180723163509.GA91424@davejwatson-mba.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: David Miller , "netdev@vger.kernel.org" , "borisp@mellanox.com" , "aviadye@mellanox.com" , Doron Roberts-Kedes To: Vakul Garg Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:60562 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730604AbeGYWOz (ORCPT ); Wed, 25 Jul 2018 18:14:55 -0400 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On 07/24/18 08:22 AM, Vakul Garg wrote: > > I don't think this patch is safe as-is. sgin_arr is a stack array of size > > MAX_SKB_FRAGS (+ overhead), while my read of skb_cow_data is that it > > walks the whole chain of skbs from skb->next, and can return any number of > > segments. Therefore we need to heap allocate. I think I copied the IPSEC > > code here. > > > > For perf though, we could use the stack array if skb_cow_data returns <= > > MAX_SKB_FRAGS. > > > > This code is slightly confusing though, since we don't heap allocate in the > > zerocopy case - what happens is that skb_to_sgvec returns -EMSGSIZE, and > > we fall back to the non-zerocopy case, and return again to this function, > > where we then hit the skb_cow_data path and heap allocate. > > Thanks for explaining. > I am missing the point why MAX_SKB_FRAGS sized local array > sgin has been used in tls_sw_recvmsg(). What is special about MAX_SKB_FRAGS so > that we used it as a size factor for 'sgin'? There is nothing special about it, in the zerocopy-fastpath if we happen to fit in MAX_SKB_FRAGS we avoid any kmalloc whatsoever though. It could be renamed MAX_SC_FOR_FASTPATH or something. > Will it be a bad idea to get rid of array 'sgin' on stack and simply kmalloc 'sgin' for > whatever the number the number of pages returned by iov_iter_npages()? > We can allocate for sgout too with the same kmalloc(). > > (Using a local array based 'sgin' is coming in the way to achieve sending multiple async > record decryption requests to the accelerator without waiting for previous one to complete.) Yes we could do this, and yes we would need to heap allocate if you want to support multiple outstanding decryption requests. I think async crypto prevents any sort of zerocopy-fastpath, however.