> -----Original Message-----
> From: Dave Watson [mailto:davejwat...@fb.com]
> Sent: Monday, July 23, 2018 10:05 PM
> To: David Miller <da...@davemloft.net>
> Cc: Vakul Garg <vakul.g...@nxp.com>; netdev@vger.kernel.org;
> bor...@mellanox.com; avia...@mellanox.com; Doron Roberts-Kedes
> <doro...@fb.com>
> Subject: Re: [net-next v5 3/3] net/tls: Remove redundant array allocation.
>
> On 07/21/18 07:25 PM, David Miller wrote:
> > From: Vakul Garg <vakul.g...@nxp.com>
> > Date: Thu, 19 Jul 2018 21:56:13 +0530
> >
> > > In function decrypt_skb(), array allocation in case when sgout is
> > > NULL is unnecessary. Instead, local variable sgin_arr[] can be used.
> > >
> > > Signed-off-by: Vakul Garg <vakul.g...@nxp.com>
> >
> > Hmmm...
> >
> > Dave, can you take a look at this? Do you think there might have been
> > a reason you felt that you needed to dynamically allocate the
> > scatterlists when you COW and skb and do in-place decryption?
> >
> > I guess this change is ok, nsg can only get smaller when the SKB is
> > COW'd.
> >
>
> > > memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
> > > if (!sgout) {
> > > nsg = skb_cow_data(skb, 0, &unused) + 1;
> > > - sgin = kmalloc_array(nsg, sizeof(*sgin),
> > > sk->sk_allocation);
> > > sgout = sgin;
> > > }
>
> 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'?
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.)
