Re: [Xen-devel] [PATCH v2 6/9] spinlock: Introduce spin_lock_cb()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.04.17 at 14:32, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 04/18/2017 02:49 AM, Jan Beulich wrote:
>>>>> On 13.04.17 at 18:55, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 04/13/2017 11:46 AM, Jan Beulich wrote:
>>>>>>> On 03.04.17 at 18:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>> While waiting for a lock we may want to periodically run some
>>>>> code. We could use spin_trylock() but since it doesn't take lock
>>>>> ticket it may take a long time until the lock is taken.
>>>>>
>>>>> Add spin_lock_cb() that allows us to execute a callback while waiting.
>>>> You don't add any user(s) of this new interface and you also don't
>>>> outline under what conditions you think using this might be a good
>>>> idea. On that basis I don't think this makes much sense. I am
>>>> particularly worried of undue latencies use of this function may
>>>> incur.
>>> There is (currently) only one user of this interface and it is
>>> introduced in the next patch.
>>>
>>> If you don't think explanation above is sufficient I can add
>>>
>>>     "This code may, for example, allow the caller to release resources
>>>       held by it that are no longer needed in the critical section protected
>>>       by the lock."
>>>
>>> after the first sentence.
>>>
>>> As for latency, the fast path is not affected, it's only if the lock is
>>> already taken do we make the callback.
>> That's a rather relevant aspect, which I think needs calling out
>> explicitly. As you may have noticed, my initial understanding of
>> the basic idea here was that the callback would be invoked while
>> spinning (i.e. to use to spinning time to do something useful),
>> not while holding the lock.
> 
> The callback *is* invoked when we are spinning waiting for the lock. I
> probably should have said "only if the lock is already taken by someone
> else". However, on the fast path, where noone is holding the lock and
> the caller can grab it right away, the callback is not invoked.

Oh, so back to what I was originally understanding, and back to my
latency concerns. Yes, releasing a resource ought to not incur much
latency, but as you know once we have a certain mechanism, other
less clear use cases may appear. Therefore while I'm not outright
objecting to the idea, I'm not really convinced of it either (the more
that the try-lock approach still exists as a possible alternative). At
least a very clear warning needs to be placed next to the function
declaration and/or definition.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel