Re: [PATCH PATCH net-next v4 8/8] tls: Enable batch async decryption in read_sock

2026-03-24, 09:17:57 -0400, Chuck Lever wrote:
> On 3/23/26 7:08 PM, Sabrina Dubroca wrote:
> > 2026-03-23, 11:04:16 -0400, Chuck Lever wrote:
> >>
> >> On Mon, Mar 23, 2026, at 10:14 AM, Sabrina Dubroca wrote:
> >>> 2026-03-17, 11:04:21 -0400, Chuck Lever wrote:
> >>>> +/* Bound on concurrent async AEAD submissions per read_sock
> >>>> + * call.  Chosen to fill typical hardware crypto pipelines
> >>>> + * without excessive memory consumption (each in-flight record
> >>>> + * holds one cleartext skb plus its AEAD request context).
> >>>> + */
> >>>> +#define TLS_READ_SOCK_BATCH	16
> >>>
> >>> I suspect that at some point, we'll have a request to make this
> >>> configurable (maybe system-wide, maybe by socket?).
> >>
> >> I appreciate your careful and close review. The series has
> >> improved significantly.
> >>
> >> I will admit that the current value (16) is arbitrary. I agree
> >> that someone might want to modify this value. At this point,
> >> however, the constant is straightforward and it is still quite
> >> easy to promote to a tunable later if that proves to be needed.
> > 
> > Agreed.
> > 
> >> The right interface for this depends on kTLS consumer needs
> >> that aren't clear (to me) yet.
> > 
> > In this case (read_sock), the kTLS consumer is NVMe/TCP etc, and
> > specifically users of those features with crypto acceleration
> > cards. I'm not familiar with either.
> > 
> >> But let me know if you have a
> >> preferred API mechanism or a specific use case in mind, or if
> >> there is a netdev policy that should guide the introduction
> >> of a suitable API for this purpose.
> > 
> > Nothing specific, I just thought I'd mention it since I was replying
> > to the patch anyway. I think at this stage "it seems easy to promote
> > to a tunable later" is enough consideration (just to avoid getting
> > trapped in some API (or lack thereof) and unable to change it, but I
> > agree that it shouldn't be a problem here).
> 
> I looked into this a little more yesterday before noticing that
> async crypto was disabled for TLS 1.3.
> 
> The hardware crypto engines do not surface a concurrency limit to
> their consumers, but their ring sizes are typically much larger than
> the batch limit of 16 I've chosen here. We can't rely on them to
> tell kTLS where to set that limit.
> 
> However, the loop was structured to continue until the batch limit
> was hit or reading gets -EBUSY. Either way 16 seems to be a safe
> but fairly conservative setting. For the larger rings it might cause
> a decrypt pipeline bubble.
> 
> So the true purpose of the low limit is to constrain the amount of
> memory tls_read_sock sets aside for decryption in progress. That's
> going to be about 256KB per socket. Yes, that could be made larger
> or contingent upon TCP socket buffer sizes, for example.

If you end up resending those changes in the future (after/together
with async support for 1.3), it would be nice to record some of this
in the commit message, to justify the limit you've chosen.


About async for 1.3: I think it would be quite some work to implement,
since we don't know the record type until after we decrypt the record.
If we get a rekey, we'll have already submitted a bunch of other
records that will fail decryption, so we'll need to roll back and
resubmit everything after the rekey, once userspace gave us the new
key. I'm not sure if other non-DATA records would also be that
problematic (or maybe worse?)

Also, async for 1.2 has been a pain (see the many fixes in the past
years). 1.3 looks complex.
I don't have access to HW that would benefit from it, do you
have benchmarks on with-async vs without-async? (obviously for
non-NVMe/non-NFS use-cases, if you test that at all)

-- 
Sabrina