Date: Tue, 17 Oct 2023 13:51:36 +0200
From: Sascha Hauer <sha@...gutronix.de>
To: Jens Axboe <axboe@...nel.dk>
Cc: Boris Pismenny <borisp@...dia.com>, netdev@...r.kernel.org,
	John Fastabend <john.fastabend@...il.com>,
	linux-kernel@...r.kernel.org, io-uring@...r.kernel.org,
	kernel@...gutronix.de, Jakub Kicinski <kuba@...nel.org>,
	Pavel Begunkov <asml.silence@...il.com>
Subject: Re: Problem with io_uring splice and KTLS

On Mon, Oct 16, 2023 at 07:17:23AM -0600, Jens Axboe wrote:
> On 10/16/23 1:26 AM, Sascha Hauer wrote:
> > On Fri, Oct 13, 2023 at 07:45:55AM -0600, Jens Axboe wrote:
> >> On 10/12/23 11:47 PM, Sascha Hauer wrote:
> >>> On Thu, Oct 12, 2023 at 07:45:07PM -0600, Jens Axboe wrote:
> >>>> On 10/12/23 7:34 AM, Sascha Hauer wrote:
> >>>>> In case you don't have encryption hardware you can create an
> >>>>> asynchronous encryption module using cryptd. Compile a kernel with
> >>>>> CONFIG_CRYPTO_USER_API_AEAD and CONFIG_CRYPTO_CRYPTD and start the
> >>>>> webserver with the '-c' option. /proc/crypto should then contain an
> >>>>> entry with:
> >>>>>
> >>>>>  name         : gcm(aes)
> >>>>>  driver       : cryptd(gcm_base(ctr(aes-generic),ghash-generic))
> >>>>>  module       : kernel
> >>>>>  priority     : 150
> >>>>
> >>>> I did a bit of prep work to ensure I had everything working for when
> >>>> there's time to dive into it, but starting it with -c doesn't register
> >>>> this entry. Turns out the bind() in there returns -1/ENOENT.
> >>>
> >>> Yes, that happens here as well, that's why I don't check for the error
> >>> in the bind call. Nevertheless it has the desired effect that the new
> >>> algorithm is registered and used from there on. BTW you only need to
> >>> start the webserver once with -c. If you start it repeatedly with -c a
> >>> new gcm(aes) instance is registered each time.
> >>
> >> Gotcha - I wasn't able to trigger the condition, which is why I thought
> >> perhaps I was missing something.
> >>
> >> Can you try the below patch and see if that makes a difference? I'm not
> >> quite sure why it would since you said it triggers with DEFER_TASKRUN as
> >> well, and for that kind of notification, you should never hit the paths
> >> you have detailed in the debug patch.
> > 
> > I can confirm that this patch makes it work for me. I tested with both
> > software cryptd and also with my original CAAM encryption workload.
> > IORING_SETUP_SINGLE_ISSUER | IORING_SETUP_DEFER_TASKRUN is not needed.
> > Both my simple webserver and the original C++ Webserver from our
> > customer are now working without problems.
> 
> OK, good to hear. I'm assuming you only change for
> sk_stream_wait_memory()? If you can reproduce, would be good to test.
> But i general none of them should hurt.

Yes, only the change in sk_stream_wait_memory() is needed for me. The
other two hunks do not change anything for me.

> 
> FWIW, the reason why DEFER_TASKRUN wasn't fully solving it is because
> we'd also use TIF_NOTIFY_SIGNAL for creating new io-wq workers. So while
> task_work would not be the trigger for setting that condition, we'd
> still end up doing it via io-wq worker creation.
> 
> > Do you think there is a chance getting this change upstream? I'm a bit
> > afraid the code originally uses signal_pending() instead of
> > task_sigpending() for a good reason.
> 
> The distinction between signal_pending() and task_sigpending() was
> introduced with TIF_NOTIFY_SIGNAL. This isn't a case of networking
> needing to use signal_pending(), just that this is was originally the
> only aborting condition and now it's a bit too broad for some cases
> (like this one).

Ok. I didn't realize so far that it was you who TIF_NOTIFY_SIGNAL.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Powered by blists - more mailing lists