opencrypto: Introduce crypto_dispatch_async()

[markj]

Summary

Currently consumers request async dispatch using a flag in the cryptop.
Only IPSec does this currently. I think this is a bit confusing: we
(conditionally) set cryptop flags to request async dispatch, and then
crypto_dispatch() immediately examines those flags to see if the
consumer wants async dispatch. The flag names are also confusing since
they don't specify what "async" applies to: dispatch or completion.

Add a new KPI, crypto_dispatch_async(), rather than encoding the
requested dispatch type in each cryptop. crypto_dispatch_async() falls
back to crypto_dispatch() if the session's driver provides asynchronous
dispatch. Get rid of CRYPTOP_ASYNC() and CRYPTOP_ASYNC_KEEPORDER().

Add CRYPTO_SESS_SYNC(), which can be used by consumers to determine
whether crypto requests will be dispatched synchronously. This is just
a helper macro. Use it instead of looking at cap flags directly.

Fix style in crypto_done(). Also get rid of CRYPTO_RETW_EMPTY() and
just check the relevant queues directly. This could result in some
unnecessary wakeups but I think it's very uncommon to be using more than
one queue per worker in a given workload, so checking all three queues
is a waste of cycles.

[jhb]

Perhaps update crypto_request.9 as well?

[markj]

In D28194#631525, @jhb wrote:

Perhaps update crypto_request.9 as well?

This made me realize that we also use the CRYPTO_F_BATCH flag to control scheduling of requests. The implementation looks really hokey though: instead of dispatching the request to a driver, we put it in a global queue and kick the crypto process, which then submits requests to the driver, passing CRYPTO_HINT_MORE until it reaches the last one.

I'm not sure if it's a very useful optimization. Most drivers don't implement it, but it seems its purpose is to let drivers reduce the number of interrupts generated. OTOH we end up having to switch to the crypto process, which is allowed to race with dispatch (so it might end up submitting multiple batches to the driver anyway). It also makes no sense with software crypto drivers.

The only consumer in the tree is GELI and it doesn't use it by default. So I wonder if we should keep this feature at all. Do you have any opinions? If we keep it I'd be inclined to add a crypto_dispatch_batch() which would take a queue of cryptops and dispatch them to the driver, and modify GELI to build up that queue before calling into crypto(9).

[jhb]

In D28194#631628, @markj wrote:

In D28194#631525, @jhb wrote:

Perhaps update crypto_request.9 as well?

This made me realize that we also use the CRYPTO_F_BATCH flag to control scheduling of requests. The implementation looks really hokey though: instead of dispatching the request to a driver, we put it in a global queue and kick the crypto process, which then submits requests to the driver, passing CRYPTO_HINT_MORE until it reaches the last one.

I'm not sure if it's a very useful optimization. Most drivers don't implement it, but it seems its purpose is to let drivers reduce the number of interrupts generated. OTOH we end up having to switch to the crypto process, which is allowed to race with dispatch (so it might end up submitting multiple batches to the driver anyway). It also makes no sense with software crypto drivers.

The only consumer in the tree is GELI and it doesn't use it by default. So I wonder if we should keep this feature at all. Do you have any opinions? If we keep it I'd be inclined to add a crypto_dispatch_batch() which would take a queue of cryptops and dispatch them to the driver, and modify GELI to build up that queue before calling into crypto(9).

I would be down with having a crypto_dispatch_batch(), and being able to dispatch those synchronously to async drivers would be nice.

I didn't try to "fix" this part of OCF in the refactor but did try to document the weirdness I found. I do think much of the scheduling stuff is quite weird and would like to make it a bit simpler and easier to understand. I think this review and your proposed BATCH change are steps in the right direction. I'd probably like to use a single thread pool like the async mode uses and ditch the other kthread.

[markj]

• Add crypto_dispatch_batch()
• Update the man page

[markj]

• Bump __FreeBSD_version

[jhb]

Definitely clearer and more sane.

share/man/man9/crypto_request.9

118