On Sat, Nov 7, 2020 at 1:23 PM Luc Van Oostenryck
<luc.vanoostenryck@xxxxxxxxx> wrote:
>
> Yes, but the branch simplifications depend on the simplifications
> that can be made on the condition. For the moment there is a focus
> on select because it often happens when the condition is simple but
> for these context imbalance the general case is something like:
> stuff
> if (arbitrary condition)
> take lock
> do other stuff
> if (same arbitrary condition)
> release lock
>
> and the problem is (at least) twofold (but partially related):
> 1) recognize that the second condition is the same as the first one
> 2) avoid that partial optimizations of the first condition 'leak'
> into the common part because this often inhibits doing 1)
> [Sorry, if this is not very clear. I need to find some small
> examples to illustrate this].
>
> When the condition is very simple, it is converted into a select
> but very often it's more complex, involves a memory access and/or
> some function calls or some asm.
Note that if the condition is complex and involves memory etc, it's
generally actually a bug in the kernel.
Because if it could possibly change between the lock taking and
releasing, you are now broken.
So I wouldn't want to accept code like that _anyway_. Any complex
conditional needs to be written as
lock = complex_conditional;
if (lock)
take lock
do other stuff
if (lock)
release lock
and if sparse handles _that_ case well, then all is ok.
If sparse complains about the fragile "repeat complex conditional that
couldn't be simplified to show that it was obviously identical", then
that's actually a _good_ thing.
Because we want to complain about stuff where it's not obviously clear
that the conditional is 100% the same.
Now, even in the above, if we'd want sparse to say that is ok, then
we'd actually need to make the context tracking itself able to deal
with "conditional on this pseudo", which it doesn't do right now.
So right now it requires that much simpler case where you actually end
up not ever having that shared case in the middle at all, and the
simplification really ends up showing that the real path is really
error = complex_conditional;
if (error)
return error;
take lock
do other stuff
release lock
which apparently sparse now - with your simplification - can see that
the code actually ends up doing .
Which is even better.
I'd much rather have the lock context tracking show that locking is
simple, and that we don't have any code that sometimes is called
locked, and sometimes is not.
Because even if you can prove - with some kind of conditional context
tracking - that the context at least _nests_ correctly (ie you always
properly unlock something you locked), that isn't actually the big
problem. The big problem really is "why did that 'do-other-stuff'
sometimes run without locking, and sometimes with locking?"
So the lock context tracking is fairly inflexible on purpose. It's
_meant_ to not just do "unlock matches a lock" matching. It's meant to
also track "ok, there is no odd 'sometimes locked, sometimes not'
code".
Linus
