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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 04/18/2017 08:43 AM, Jan Beulich wrote:
>>>> 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.


I'd rather add a warning in the header file since trylock approach is
essentially unbounded, especially on large systems (where this series
becomes useful).

Alternatively, I could drop the lock and restart in alloc_heap_pages()
when buddy that is being scrubbed is selected and have the scrubber
break the it into clean and dirty part. But that will cause us lose the
high-order chunk.

-boris


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