On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
> From: Peter Rosin <[email protected]>
>
> Hi Greg,
>
> Philipp found problems in v14 with using a mutex for locking that was
> the outcome of the review for v13, so I'm now using a semaphore instead
> of the rwsem that was in v13. That at least got rid of the scary call
> to downgrade_write. However, I'm still unsure about what you actually
> meant with your comment about lack of sparse markings [1]. I did add
> __must_check to the funcs that selects the mux, but I've got this
> feeling that this is not what you meant?
I thought there was a way to mark a function as requiring a lock be held
when it is being called. Does sparse not support that anymore?
thanks,
greg k-h
On Sat, Jun 03, 2017 at 07:26:27PM +0900, Greg Kroah-Hartman wrote:
> On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
> > From: Peter Rosin <[email protected]>
> >
> > Hi Greg,
> >
> > Philipp found problems in v14 with using a mutex for locking that was
> > the outcome of the review for v13, so I'm now using a semaphore instead
> > of the rwsem that was in v13. That at least got rid of the scary call
> > to downgrade_write. However, I'm still unsure about what you actually
> > meant with your comment about lack of sparse markings [1]. I did add
> > __must_check to the funcs that selects the mux, but I've got this
> > feeling that this is not what you meant?
>
> I thought there was a way to mark a function as requiring a lock be held
> when it is being called. Does sparse not support that anymore?
Anyway, not a big deal. I still worry about the calls blocking when
people are not expecting them to, but it is just the nature of th api I
guess.
All now queued up, nice work, thanks for sticking with this.
greg k-h
On Sat, Jun 3, 2017 at 12:26 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
>> From: Peter Rosin <[email protected]>
>>
>> Hi Greg,
>>
>> Philipp found problems in v14 with using a mutex for locking that was
>> the outcome of the review for v13, so I'm now using a semaphore instead
>> of the rwsem that was in v13. That at least got rid of the scary call
>> to downgrade_write. However, I'm still unsure about what you actually
>> meant with your comment about lack of sparse markings [1]. I did add
>> __must_check to the funcs that selects the mux, but I've got this
>> feeling that this is not what you meant?
>
> I thought there was a way to mark a function as requiring a lock be held
> when it is being called. Does sparse not support that anymore?
sparse still support these annotations, of course.
In this case, I suppose you're talking about '__must_hold()' which
*must* be used instead of a pair of '__releases()' + '__acquires()'
when the lock is help on function entry and exit.
Cheers,
-- Luc Van Oostenryck
On Sat, Jun 03, 2017 at 08:37:21PM +0200, Luc Van Oostenryck wrote:
> On Sat, Jun 3, 2017 at 12:26 PM, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
> >> From: Peter Rosin <[email protected]>
> >>
> >> Hi Greg,
> >>
> >> Philipp found problems in v14 with using a mutex for locking that was
> >> the outcome of the review for v13, so I'm now using a semaphore instead
> >> of the rwsem that was in v13. That at least got rid of the scary call
> >> to downgrade_write. However, I'm still unsure about what you actually
> >> meant with your comment about lack of sparse markings [1]. I did add
> >> __must_check to the funcs that selects the mux, but I've got this
> >> feeling that this is not what you meant?
> >
> > I thought there was a way to mark a function as requiring a lock be held
> > when it is being called. Does sparse not support that anymore?
>
> sparse still support these annotations, of course.
> In this case, I suppose you're talking about '__must_hold()' which
> *must* be used instead of a pair of '__releases()' + '__acquires()'
> when the lock is help on function entry and exit.
Ah, yes, that's what I was thinking of. I don't know if sparse can
track things like this across an exported symbol, so I doubt it really
will help here. Sorry for the noise.
thanks,
greg k-h
On Sat, Jun 3, 2017 at 9:34 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Sat, Jun 03, 2017 at 08:37:21PM +0200, Luc Van Oostenryck wrote:
>> On Sat, Jun 3, 2017 at 12:26 PM, Greg Kroah-Hartman
>> <[email protected]> wrote:
>> > On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
>> >> From: Peter Rosin <[email protected]>
>> >>
>> >> Hi Greg,
>> >>
>> >> Philipp found problems in v14 with using a mutex for locking that was
>> >> the outcome of the review for v13, so I'm now using a semaphore instead
>> >> of the rwsem that was in v13. That at least got rid of the scary call
>> >> to downgrade_write. However, I'm still unsure about what you actually
>> >> meant with your comment about lack of sparse markings [1]. I did add
>> >> __must_check to the funcs that selects the mux, but I've got this
>> >> feeling that this is not what you meant?
>> >
>> > I thought there was a way to mark a function as requiring a lock be held
>> > when it is being called. Does sparse not support that anymore?
>>
>> sparse still support these annotations, of course.
>> In this case, I suppose you're talking about '__must_hold()' which
>> *must* be used instead of a pair of '__releases()' + '__acquires()'
>> when the lock is help on function entry and exit.
>
> Ah, yes, that's what I was thinking of. I don't know if sparse can
> track things like this across an exported symbol, so I doubt it really
> will help here. Sorry for the noise.
No problem, I'm glad to help for sparse related things.
I didn't saw the code in question because the lkml.org link Peter
gave didn't work for me and I don't know much about exported symbols
(but I think the sole effect is to add some data in some symbol table).
But these annotations just work based on the declarations, very much
like type checking. So if you have something in scope like the following:
void do_stuff_locked(struct s *ptr) __must_hold(*ptr);
...
void do_stuff_unlocked(struct s *ptr)
{
...
do_stuff_locked(ptr); // will warn
...
}
You will have a warning from sparse unless the code preceding and following
the call to do_stuff_locked() lock & then unlock 'ptr', generaly
indirectly by a pair
of functions, the one before with an '__acquires()' in its declaration
the one after
with a '__releases()' in its declaration:
void lock_stuff(struct s *ptr) __acquires(*ptr);
void unlock_stuff(struct s *ptr) __releases(*ptr);
void do_stuff_unlocked(struct s *ptr)
{
lock_stuff(ptr);
do_stuff_locked(ptr); // won't warn
unlock_stuff(ptr);
}
Regards,
-- Luc
On 2017-06-03 22:26, Luc Van Oostenryck wrote:
> On Sat, Jun 3, 2017 at 9:34 PM, Greg Kroah-Hartman
> <[email protected]> wrote:
>> On Sat, Jun 03, 2017 at 08:37:21PM +0200, Luc Van Oostenryck wrote:
>>> On Sat, Jun 3, 2017 at 12:26 PM, Greg Kroah-Hartman
>>> <[email protected]> wrote:
>>>> On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
>>>>> From: Peter Rosin <[email protected]>
>>>>>
>>>>> Hi Greg,
>>>>>
>>>>> Philipp found problems in v14 with using a mutex for locking that was
>>>>> the outcome of the review for v13, so I'm now using a semaphore instead
>>>>> of the rwsem that was in v13. That at least got rid of the scary call
>>>>> to downgrade_write. However, I'm still unsure about what you actually
>>>>> meant with your comment about lack of sparse markings [1]. I did add
>>>>> __must_check to the funcs that selects the mux, but I've got this
>>>>> feeling that this is not what you meant?
>>>>
>>>> I thought there was a way to mark a function as requiring a lock be held
>>>> when it is being called. Does sparse not support that anymore?
>>>
>>> sparse still support these annotations, of course.
>>> In this case, I suppose you're talking about '__must_hold()' which
>>> *must* be used instead of a pair of '__releases()' + '__acquires()'
>>> when the lock is help on function entry and exit.
>>
>> Ah, yes, that's what I was thinking of. I don't know if sparse can
>> track things like this across an exported symbol, so I doubt it really
>> will help here. Sorry for the noise.
>
> No problem, I'm glad to help for sparse related things.
>
> I didn't saw the code in question because the lkml.org link Peter
> gave didn't work for me and I don't know much about exported symbols
> (but I think the sole effect is to add some data in some symbol table).
> But these annotations just work based on the declarations, very much
> like type checking. So if you have something in scope like the following:
>
> void do_stuff_locked(struct s *ptr) __must_hold(*ptr);
>
> ...
>
> void do_stuff_unlocked(struct s *ptr)
> {
> ...
> do_stuff_locked(ptr); // will warn
> ...
> }
>
> You will have a warning from sparse unless the code preceding and following
> the call to do_stuff_locked() lock & then unlock 'ptr', generaly
> indirectly by a pair
> of functions, the one before with an '__acquires()' in its declaration
> the one after
> with a '__releases()' in its declaration:
>
> void lock_stuff(struct s *ptr) __acquires(*ptr);
> void unlock_stuff(struct s *ptr) __releases(*ptr);
>
> void do_stuff_unlocked(struct s *ptr)
> {
> lock_stuff(ptr);
> do_stuff_locked(ptr); // won't warn
> unlock_stuff(ptr);
> }
Ok, thanks for the explanation! The above was what I gathered when I
looked around, and since it didn't really fit the usage pattern of the
mux api I was stomped. When comparing the mux code with the above,
mux_control_select would be an __acquires (albeit a conditional one,
but let's not muddy the waters unnecessarily) and mux_control_deselect
would be a __releases.
But for long time mux consumers, like the video mux, it must be OK to
only acquire the mux, and not release it right away in the same context,
which I assume will be very hard for sparse to handle sanely? E.g. I
think sparse also complains if there are unbalanced __acquires and
__releases in some context, no?
Cheers,
peda
BTW, the core mux code is at the below link if the lkml link continues
to fail:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-testing&id=a3b02a9c6591ce154cd44e2383406390a45b530c
On 2017-06-03 12:31, Greg Kroah-Hartman wrote:
> On Sat, Jun 03, 2017 at 07:26:27PM +0900, Greg Kroah-Hartman wrote:
>> On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
>>> From: Peter Rosin <[email protected]>
>>>
>>> Hi Greg,
>>>
>>> Philipp found problems in v14 with using a mutex for locking that was
>>> the outcome of the review for v13, so I'm now using a semaphore instead
>>> of the rwsem that was in v13. That at least got rid of the scary call
>>> to downgrade_write. However, I'm still unsure about what you actually
>>> meant with your comment about lack of sparse markings [1]. I did add
>>> __must_check to the funcs that selects the mux, but I've got this
>>> feeling that this is not what you meant?
>>
>> I thought there was a way to mark a function as requiring a lock be held
>> when it is being called. Does sparse not support that anymore?
>
> Anyway, not a big deal. I still worry about the calls blocking when
> people are not expecting them to, but it is just the nature of th api I
> guess.
Yeah, first come first serve. I don't know what else I can do, except maybe
follow up with a timed version of mux_control_select()...
> All now queued up, nice work, thanks for sticking with this.
*big sigh of relief*
I was getting pretty fed up with the series to be honest :-), so thanks
a bunch for taking it!
Cheers,
peda
On Sat, Jun 3, 2017 at 11:29 PM, Peter Rosin <[email protected]> wrote:
> On 2017-06-03 22:26, Luc Van Oostenryck wrote:
>> On Sat, Jun 3, 2017 at 9:34 PM, Greg Kroah-Hartman
>> <[email protected]> wrote:
>>> On Sat, Jun 03, 2017 at 08:37:21PM +0200, Luc Van Oostenryck wrote:
>>>> On Sat, Jun 3, 2017 at 12:26 PM, Greg Kroah-Hartman
>>>> <[email protected]> wrote:
>>>>> On Sun, May 14, 2017 at 09:51:03PM +0200, Peter Rosin wrote:
>>>>>> From: Peter Rosin <[email protected]>
>>>>>>
>>>>>> Hi Greg,
>>>>>>
>>>>>> Philipp found problems in v14 with using a mutex for locking that was
>>>>>> the outcome of the review for v13, so I'm now using a semaphore instead
>>>>>> of the rwsem that was in v13. That at least got rid of the scary call
>>>>>> to downgrade_write. However, I'm still unsure about what you actually
>>>>>> meant with your comment about lack of sparse markings [1]. I did add
>>>>>> __must_check to the funcs that selects the mux, but I've got this
>>>>>> feeling that this is not what you meant?
>>>>>
>>>>> I thought there was a way to mark a function as requiring a lock be held
>>>>> when it is being called. Does sparse not support that anymore?
>>>>
>>>> sparse still support these annotations, of course.
>>>> In this case, I suppose you're talking about '__must_hold()' which
>>>> *must* be used instead of a pair of '__releases()' + '__acquires()'
>>>> when the lock is help on function entry and exit.
>>>
>>> Ah, yes, that's what I was thinking of. I don't know if sparse can
>>> track things like this across an exported symbol, so I doubt it really
>>> will help here. Sorry for the noise.
>>
>> No problem, I'm glad to help for sparse related things.
>>
>> I didn't saw the code in question because the lkml.org link Peter
>> gave didn't work for me and I don't know much about exported symbols
>> (but I think the sole effect is to add some data in some symbol table).
>> But these annotations just work based on the declarations, very much
>> like type checking. So if you have something in scope like the following:
>>
>> void do_stuff_locked(struct s *ptr) __must_hold(*ptr);
>>
>> ...
>>
>> void do_stuff_unlocked(struct s *ptr)
>> {
>> ...
>> do_stuff_locked(ptr); // will warn
>> ...
>> }
>>
>> You will have a warning from sparse unless the code preceding and following
>> the call to do_stuff_locked() lock & then unlock 'ptr', generaly
>> indirectly by a pair
>> of functions, the one before with an '__acquires()' in its declaration
>> the one after
>> with a '__releases()' in its declaration:
>>
>> void lock_stuff(struct s *ptr) __acquires(*ptr);
>> void unlock_stuff(struct s *ptr) __releases(*ptr);
>>
>> void do_stuff_unlocked(struct s *ptr)
>> {
>> lock_stuff(ptr);
>> do_stuff_locked(ptr); // won't warn
>> unlock_stuff(ptr);
>> }
>
> Ok, thanks for the explanation! The above was what I gathered when I
> looked around, and since it didn't really fit the usage pattern of the
> mux api I was stomped. When comparing the mux code with the above,
> mux_control_select would be an __acquires (albeit a conditional one,
> but let's not muddy the waters unnecessarily) and mux_control_deselect
> would be a __releases.
>
> But for long time mux consumers, like the video mux, it must be OK to
> only acquire the mux, and not release it right away in the same context,
> which I assume will be very hard for sparse to handle sanely? E.g. I
> think sparse also complains if there are unbalanced __acquires and
> __releases in some context, no?
Yes, there are some limitations.
You can do conditional locks, it's what spin_trylock() and
{read,write}_trylock()
do. something like the following will be OK:
void do_stuff_unlocked(struct s *ptr)
{
if (trylock_stuff(ptr)) {
do_stuff_locked(ptr); // won't warn
unlock_stuff(ptr);
}
}
sparse won't complain because everything is correct within each block.
But something that sparse will complain about would be:
void foobar(struct s *ptr)
{
if (some condition)
lock_stuff(ptr);
// will warn
... some code ...
if (same condition) {
do_stuff_locked(ptr);
unlock_stuff(ptr);
}
}
While correct from the point of view of locking, sparse won't be able
to take into account
that indeed, the two ifs are fro the same condition. sparse will only
see that after the first
if there will be one path where the lock will be held and another one
where it won't and
will this issue a 'different lock contexts for basic block' warning.
For this mux code, for example, the function mux_control_select() will
have a problem
because depending on the return value of __mux_control_select() you will want to
take back the lock or not, so the exit state of this function is not
well defined regarding
locking. It's the same for mux_control_try_select().
I hope this render the possibilities a bit clearer.
Cheers,
-- Luc
> > All now queued up, nice work, thanks for sticking with this.
>
> *big sigh of relief*
I can imagine. Great work, Peda! Congrats.