2023-08-18, 18:46:48 -0700, Jakub Kicinski wrote:

On Thu, 17 Aug 2023 17:07:03 +0200 Sabrina Dubroca wrote:

quoted

Commit ab046a5d4be4 ("net: macsec: preserve ingress frame ordering")
tried to solve an issue caused by MACsec's use of asynchronous crypto
operations, but introduced a large performance regression in cases
where async crypto isn't causing reordering of packets.

This patch introduces a per-netns sysctl that administrators can set
to allow new SAs to use async crypto, such as aesni. Existing SAs
won't be modified.

By setting default_async_crypto=1 and reconfiguring macsec, a single
netperf instance jumps from 1.4Gbps to 4.4Gbps.

Can we not fix the ordering problem?
Queue the packets locally if they get out of order?

Actually, looking into the crypto API side, I don't see how they can
get out of order since commit 81760ea6a95a ("crypto: cryptd - Add
helpers to check whether a tfm is queued"):

    [...] ensure that no reordering is introduced because of requests
    queued in cryptd with respect to requests being processed in
    softirq context.

And cryptd_aead_queued() is used by AESNI (via simd_aead_decrypt()) to
decide whether to process the request synchronously or not.

So I really don't get what commit ab046a5d4be4 was trying to fix. I've
never been able to reproduce that issue, I guess commit 81760ea6a95a
explains why.

I'd suggest to revert commit ab046a5d4be4, but it feels wrong to
revert it without really understanding what problem Scott hit and why
81760ea6a95a didn't solve it.

What do you think?

-- 
Sabrina