Killing off __cond_lock()

Killing off __cond_lock()

[Peter Zijlstra]

Hi all,

So the kernel has this __cond_lock() crap pile, which is implemented
like:

#define __acquire(x)       __context__(x,1)
#define __cond_lock(x,c)   ((c) ? ({ __acquire(x); 1; }) : 0)

Now the problem with this is that people send ugly patches like:

https://lkml.org/lkml/2012/3/24/57
http://www.spinics.net/lists/mm-commits/msg80386.html

That basically wrap an existing function in an inline function just to
use __cond_lock() on the return value.

It would be ever so much nicer if we could declare such functions like:

struct anon_vma *page_lock_anon_vma(struct page *page)
	__cond_acquires(RCU) 
	__cond_acquires(page_lock_anon_vma(page)->root->lock);

Meaning that if the return value is true (non-zero), it would acquire
that lock/context. One could of course add some shorter means of
referring to the return value, but simply using the function in the
expression should be simple enough.

In order to implement this I guess we need to extend the
__attribute__((context(expr,in,out))) thing. 

Currently in,out are explicit value constants, but I guess if we make
them expressions we could evaluate them and get dynamic behaviour.

Thus allowing something like:

int spin_trylock(spinlock_t *lock)
	__attribute__((context(lock, 0, !!spin_trylock(lock));

meaning that the context would be incremented by 1 if the return value
were true.

Having only briefly looked at the sparse source, is this feasible to
implement or do we get chicken/egg problems wrt using a function before
its declaration is complete, and referring a return value before the
function is part of an expression?

If this yields problems, are there better ways of solving this issue?

Re: Killing off __cond_lock()

[Johannes Berg]

Hi,

> struct anon_vma *page_lock_anon_vma(struct page *page)
> 	__cond_acquires(RCU) 
> 	__cond_acquires(page_lock_anon_vma(page)->root->lock);
> 
> Meaning that if the return value is true (non-zero), it would acquire
> that lock/context. One could of course add some shorter means of
> referring to the return value, but simply using the function in the
> expression should be simple enough.
> 
> In order to implement this I guess we need to extend the
> __attribute__((context(expr,in,out))) thing. 
> 
> Currently in,out are explicit value constants, but I guess if we make
> them expressions we could evaluate them and get dynamic behaviour.
> 
> Thus allowing something like:
> 
> int spin_trylock(spinlock_t *lock)
> 	__attribute__((context(lock, 0, !!spin_trylock(lock));
> 
> meaning that the context would be incremented by 1 if the return value
> were true.
> 
> Having only briefly looked at the sparse source, is this feasible to
> implement or do we get chicken/egg problems wrt using a function before
> its declaration is complete, and referring a return value before the
> function is part of an expression?
> 
> If this yields problems, are there better ways of solving this issue?

I once looked at all of this (which I suspect you saw, given that you're
CC'ing me) but all my changes ended up being reverted since they broke
things so maybe I'm not the right person to ask ... :-)

I played with having a "RETURN" builtin (or something like that) to use
inside here but it didn't really work out well, I don't think that was
what ended up going upstream though.

However, I don't think using the function call etc. is a good idea, to
me that makes it look too much like you could put arbitrary code there,
but since this doesn't even exist at runtime ...

However, note that today sparse doesn't evaluate anything in the
context, it doesn't even look at the first argument. So another thing
you can't really annotate well is things like this:

struct foo_object *get_locked_object(...);

This is why I used RETURN to give the return value a name, so you could
write
	__acquires(&RETURN->lock)


But I was also trying to make sparse actually evaluate the first
argument so it could tell the difference between two locks, which you
might not even care about ... (it would be nice though I think)

johannes

Re: Killing off __cond_lock()

[Peter Zijlstra]

On Sun, 2012-03-25 at 10:48 +0200, Johannes Berg wrote:
> Hi,
> 
> > struct anon_vma *page_lock_anon_vma(struct page *page)
> > 	__cond_acquires(RCU) 
> > 	__cond_acquires(page_lock_anon_vma(page)->root->lock);
> > 
> > Meaning that if the return value is true (non-zero), it would acquire
> > that lock/context. One could of course add some shorter means of
> > referring to the return value, but simply using the function in the
> > expression should be simple enough.
> > 
> > In order to implement this I guess we need to extend the
> > __attribute__((context(expr,in,out))) thing. 
> > 
> > Currently in,out are explicit value constants, but I guess if we make
> > them expressions we could evaluate them and get dynamic behaviour.
> > 
> > Thus allowing something like:
> > 
> > int spin_trylock(spinlock_t *lock)
> > 	__attribute__((context(lock, 0, !!spin_trylock(lock));
> > 
> > meaning that the context would be incremented by 1 if the return value
> > were true.
> > 
> > Having only briefly looked at the sparse source, is this feasible to
> > implement or do we get chicken/egg problems wrt using a function before
> > its declaration is complete, and referring a return value before the
> > function is part of an expression?
> > 
> > If this yields problems, are there better ways of solving this issue?
> 
> I once looked at all of this (which I suspect you saw, given that you're
> CC'ing me) but all my changes ended up being reverted since they broke
> things so maybe I'm not the right person to ask ... :-)

Yeah, I looked at the git log of validation/context.c :-)

> I played with having a "RETURN" builtin (or something like that) to use
> inside here but it didn't really work out well, I don't think that was
> what ended up going upstream though.
> 
> However, I don't think using the function call etc. is a good idea, to
> me that makes it look too much like you could put arbitrary code there,
> but since this doesn't even exist at runtime ...

Fair enough.. the explicit reference to the return value is indeed more
convenient and as you show below makes it possible to do other things as
well.

> However, note that today sparse doesn't evaluate anything in the
> context, it doesn't even look at the first argument. So another thing
> you can't really annotate well is things like this:
> 
> struct foo_object *get_locked_object(...);
> 
> This is why I used RETURN to give the return value a name, so you could
> write
> 	__acquires(&RETURN->lock)


Right, but if it doesn't actually evaluate the expression used in the
context this is going to be problematic.

> But I was also trying to make sparse actually evaluate the first
> argument so it could tell the difference between two locks, which you
> might not even care about ... (it would be nice though I think)

Right, so what I thought we could maybe do is inject code in the
callsites of these functions.

So after the OP_CALL emit a piece of code that works like the
__context__ stmt and can reference the return value that exists at that
point.

This also makes the conditional thing quite simple to do.

Re: Killing off __cond_lock()

[Johannes Berg]

On Mon, 2012-03-26 at 10:11 +0200, Peter Zijlstra wrote:

> > However, note that today sparse doesn't evaluate anything in the
> > context, it doesn't even look at the first argument. So another thing
> > you can't really annotate well is things like this:
> > 
> > struct foo_object *get_locked_object(...);
> > 
> > This is why I used RETURN to give the return value a name, so you could
> > write
> > 	__acquires(&RETURN->lock)
> 
> 
> Right, but if it doesn't actually evaluate the expression used in the
> context this is going to be problematic.

It probably could -- but the question is what context to evaluate it in.

> > But I was also trying to make sparse actually evaluate the first
> > argument so it could tell the difference between two locks, which you
> > might not even care about ... (it would be nice though I think)
> 
> Right, so what I thought we could maybe do is inject code in the
> callsites of these functions.
> 
> So after the OP_CALL emit a piece of code that works like the
> __context__ stmt and can reference the return value that exists at that
> point.
> 
> This also makes the conditional thing quite simple to do.

Indeed, but you'd need some sort of expression rewriting. Consider

void lock_obj(obj_t *o) __acquires(&o->lock);

void lock_obj(obj_t *obj)
{
	__acquire(&obj->lock);

	...
}


void foo(void)
{
	...
	lock_obj(&f);
	...
	unlock_obj(&f);
	...
}


Now you suddenly need to replace "&o->lock" with "&(&f)->lock", when
checking the function itself it really is called "obj", not "o". Not
that sparse actually checks that the function behaviour matches the
declaration today though.

johannes


PS: Something else I had wanted to remind you of: the cond_lock thing
only works due to some sort of optimiser pass (is there such a thing?)
in sparse, sometimes it fails mysteriously because the condition isn't
the exact same condition or something.

Re: Killing off __cond_lock()

[Josh Triplett]

On Mon, Mar 26, 2012 at 10:11:34AM +0200, Peter Zijlstra wrote:
> On Sun, 2012-03-25 at 10:48 +0200, Johannes Berg wrote:
> > But I was also trying to make sparse actually evaluate the first
> > argument so it could tell the difference between two locks, which you
> > might not even care about ... (it would be nice though I think)
> 
> Right, so what I thought we could maybe do is inject code in the
> callsites of these functions.
> 
> So after the OP_CALL emit a piece of code that works like the
> __context__ stmt and can reference the return value that exists at that
> point.
> 
> This also makes the conditional thing quite simple to do.

For the general case, this seems like roughly the right solution.  That
will also handle slightly more complex conditionals, though
significantly more complex ones will just get sparse complaining that
you can reach later code with multiple contexts.

Similarly, require_context might work better as an expression executed
beforehand, which would ideally have access to the arguments.

I don't know if it makes sense to put an __attribute__((__after_expr__,
arbitrary_expression)) into sparse just to avoid doing the same thing
with a macro or static inline, though.  A macro or static inline
trivially has access to the arguments and return value, and can run
arbitrary statements/expressions either before or after the underlying
function call, including conditionals.

(Even better if sparse could just always do whole-program analysis, so
that you could include the __context__ call inline in the normal
function and have the caller see it, but that won't happen anytime
soon.)

- Josh Triplett