On Wed, Feb 26, 2025 at 1:23 PM Yosry Ahmed <yosry.ahmed@xxxxxxxxx> wrote: > > On Wed, Feb 26, 2025 at 09:16:28PM +0000, Eric Biggers wrote: > > On Wed, Feb 26, 2025 at 08:32:22PM +0000, Yosry Ahmed wrote: > > > On Wed, Feb 26, 2025 at 08:00:16PM +0000, Eric Biggers wrote: > > > > On Wed, Feb 26, 2025 at 06:56:25PM +0000, Yosry Ahmed wrote: > > > > > Currently, zswap_cpu_comp_dead() calls crypto_free_acomp() while holding > > > > > the per-CPU acomp_ctx mutex. crypto_free_acomp() then holds scomp_lock > > > > > (through crypto_exit_scomp_ops_async()). > > > > > > > > > > On the other hand, crypto_alloc_acomp_node() holds the scomp_lock > > > > > (through crypto_scomp_init_tfm()), and then allocates memory. > > > > > If the allocation results in reclaim, we may attempt to hold the per-CPU > > > > > acomp_ctx mutex. > > > > > > > > The bug is in acomp. crypto_free_acomp() should never have to wait for a memory > > > > allocation. That is what needs to be fixed. > > > > > > crypto_free_acomp() does not explicitly wait for an allocation, but it > > > waits for scomp_lock (in crypto_exit_scomp_ops_async()), which may be > > > held while allocating memory from crypto_scomp_init_tfm(). > > > > > > Are you suggesting that crypto_exit_scomp_ops_async() should not be > > > holding scomp_lock? > > > > I think the solution while keeping the bounce buffer in place would be to do > > what the patch > > https://lore.kernel.org/linux-crypto/Z6w7Pz8jBeqhijut@xxxxxxxxxxxxxxxxxxx/ does, > > i.e. make the actual allocation and free happen outside the lock. > > I am fine with a solution like that if Herbert is fine with it. Although > as I mentioned, I think this patch is nice to have anyway. > > > > > > > But really the bounce buffering in acomp (which is what is causing this problem) > > > > should not exist at all. There is really no practical use case for it; it's > > > > just there because of the Crypto API's insistence on shoehorning everything into > > > > scatterlists for no reason... > > > > > > I am assuming this about scomp_scratch logic, which is what we need to > > > hold the scomp_lock for, resulting in this problem. > > > > Yes. > > > > > If this is something that can be done right away I am fine with dropping > > > this patch for an alternative fix, although it may be nice to reduce the > > > lock critical section in zswap_cpu_comp_dead() to the bare minimum > > > anyway. > > > > Well, unfortunately the whole Crypto API philosophy of having a single interface > > for software and for hardware offload doesn't really work. This is just yet > > another example of that; it's a problem caused by shoehorning software > > compression into an interface designed for hardware offload. zcomp really > > should just use the compression libs directly (like most users of compression in > > the kernel already do), and have an alternate code path specifically for > > hardware offload (using acomp) for the few people who really want that. > > zcomp is for zram, zswap does not use it. If zswap is not going to use > the crypto API we'll want something like zcomp or maybe reuse zcomp > itself. That's a problem for another day :) I'm actually thinking whether we should expose the zcomp API and use it for zswap. There are a couple of parameters for zstd I wanna play with, which zcomp/zram seems to already support, but not the crypto API (zstd level, dictionary, etc.). But yes, a different problem for another day :)
