> On Nov 5, 2020, at 1:50 PM, Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx> wrote:
>
> On Thu, Nov 05, 2020 at 01:18:02PM -0800, Linus Torvalds wrote:
>> On Thu, Nov 5, 2020 at 1:13 PM Luc Van Oostenryck
>> <luc.vanoostenryck@xxxxxxxxx> wrote:
>>>
>>> Annotations can't fix anything here. The patch [1] changes htab_lock_bucket(),
>>> into conditionally taking a lock while before it took it unconditionally.
>>> So, each point where its value is tested now becomes the confluence of 2
>>> distinct paths: one with the lock taken and one with the lock not taken.
>>> This is what is meant by 'context imbalance'.
>>
>> I think the point Song makes is that if sparse did a few more
>> optimizations, it would see the context imbalance go away, because the
>> value it tests would become constant.
>
> It's not how I understood it, but yes, I agree. It's what I tried to
> briefly explain the in the last paragraph.
>
> From what I know from these situations, in most cases, simple
> 'expression' optimizations are not enough but simple forms of
> code hoisting would often anything that's needed.
Hi Luc and Linus,
Thanks for your comments!
I know very little about sparse internal. But I feel this is more
like a minor bug.
Here is the simplified inlined code of htab_map_delete_elem()
if (some_condition) {
ret = -EBUSY;
} else {
spin_lock();
ret = 0;
}
if (ret)
return ret;
+ ret = 0;
[...]
if (some_other_condition)
ret = -ENOENT;
spin_unlock();
return ret;
When we reach the point of "+ ret = 0;", we will do spin_unlock()
regardless of the value of ret. So whether we know ret is definitely
equal to zero should not matter at all?
It feels like sparse knows there are two paths before "if (ret)":
1) ret == 0, we did spin_lock()
2) ret == -EBUSY, we didn't do spin_lock()
Then, after the "if (ret)", even without explicit "ret = 0;", it is
possible to prune path 2), but I guess sparse is not doing it, which
is unfortunate but not a bug. However, it seems explicit "ret = 0;"
helps sparse prune path 2), which doesn't seem right. Explicit "ret = 0"
should only overwrite the value of ret, but not prune any path.
Does this make sense?
Thanks,
Song
