On Tue, 15 Sep 2020 at 01:43, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
On Mon, Sep 14, 2020 at 3:24 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
Ard and Herbert added to participants: see
chacha20poly1305_crypt_sg_inplace(), which does
flags = SG_MITER_TO_SG;
if (!preemptible())
flags |= SG_MITER_ATOMIC;
introduced in commit d95312a3ccc0 ("crypto: lib/chacha20poly1305 -
reimplement crypt_from_sg() routine").
As far as I can tell, the only reason for this all is to try to use
"kmap()" rather than "kmap_atomic()".
And kmap() actually has the much more complex "might_sleep()" tests,
and apparently the "preemptible()" check wasn't even the proper full
debug check, it was just a complete hack to catch the one that
triggered.
This was not driven by a failing check.
The documentation of kmap_atomic() states the following:
* The use of kmap_atomic/kunmap_atomic is discouraged - kmap/kunmap
* gives a more generic (and caching) interface. But kmap_atomic can
* be used in IRQ contexts, so in some (very limited) cases we need
* it.
so if this is no longer accurate, perhaps we should fix it?
But another reason I tried to avoid kmap_atomic() is that it disables
preemption unconditionally, even on 64-bit architectures where HIGHMEM
is irrelevant. So using kmap_atomic() here means that the bulk of
WireGuard packet encryption runs with preemption disabled, essentially
for legacy reasons.
From a quick look, that code should probably just get rid of
SG_MITER_ATOMIC entirely, and alwayse use kmap_atomic().
kmap_atomic() is actually the faster and proper interface to use
anyway (never mind that any of this matters on any sane hardware). The
old kmap() and kunmap() interfaces should generally be avoided like
the plague - yes, they allow sleeping in the middle and that is
sometimes required, but if you don't need that, you should never ever
use them.
We used to have a very nasty kmap_atomic() that required people to be
very careful and know exactly which atomic entry to use, and that was
admitedly quite nasty.
So it _looks_ like this code started using kmap() - probably back when
kmap_atomic() was so cumbersome to use - and was then converted
(conditionally) to kmap_atomic() rather than just changed whole-sale.
Is there actually something that wants to use those sg_miter functions
and sleep?
Because if there is, that choice should come from the outside, not
from inside lib/scatterlist.c trying to make some bad guess based on
the wrong thing entirely.
Linus
