Re: "warning: context imbalance" in kernel/bpf/hashtab.c

[Song Liu <songliubraving@xxxxxx>]

> On Nov 6, 2020, at 9:45 AM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> On Fri, Nov 6, 2020 at 9:30 AM Song Liu <songliubraving@xxxxxx> wrote:
>> 
>> I know very little about sparse internal. But I feel this is more
>> like a minor bug.
> 
> It's not really a bug, and the pattern you show isn't actually all
> that easy to see for a compiler.
> 
>> Here is the simplified inlined code of htab_map_delete_elem()
>> 
>>        if (some_condition) {
>>                ret = -EBUSY;
>>        } else {
>>                spin_lock();
>>                ret = 0;
>>        }
> 
> So here the issue for the sparse optimizer is that while both "ret"
> and "spin_lock" is tied to "some_condition", it's not really obvious.
> 
> A compiler would have to follow all the value analysis - and good
> compilers do that, but sparse is not a "great" compiler, it's more of
> a "solid and not horribly stupid" one.
> 
>>        if (ret)
>>                return ret;
> 
> So I'd really suggest you make the code more obvious - it will be more
> obvious to humans too.
> 
> Write it as
> 
>        if (some_condition)
>                return -EBUSY;
>        spin_lock();
> 
> instead, and now sparse will see that obviously spin_lock() and
> "some_condition" are mutually incompatible.

Thanks for your suggestion! This pattern didn't trigger the warning. 
We still need some work to apply this to actual code, because the lock
part is in an helper:

inline int htab_lock_bucket()
{
	if (some_condition) 
		return -EBUSY;	
	spin_lock();
	return 0;
}

int htab_map_delete_elem()
{
	ret = htab_lock_bucket();
	if (ret)
		return ret;
	[...]
}

We can probably split htab_lock_bucket() into two helpers, like

int htab_map_delete_elem()
{
	ret = htab_lock_is_busy();  // check some_condition
	if (ret)
		return ret;

	htab_do_lock();    // calls spin_lock()

	[...]
}

I will do more experiments with this. 

Thanks again!
Song