Re: [PATCH v5 01/12] spinlock: A new lockref structure for lockless update of refcount

[Waiman Long <waiman.long@hp.com>]

On 07/15/2013 10:41 AM, Thomas Gleixner wrote:
> On Mon, 8 Jul 2013, Waiman Long wrote:
>> On 07/05/2013 02:59 PM, Thomas Gleixner wrote:
>>> On Fri, 5 Jul 2013, Waiman Long wrote:
>> I think it is OK to add the GENERIC option, but I would like to make available
>> a slightly different set of options:
>> 1) Always take the lock
>> 2) Use the generic implementation with the default parameters
>> 3) Use the generic implementation with a customized set of parameters
>> 4) Use an architecture specific implementation.
>>
>> 2) set only GENERIC_SPINLOCK_REFCOUNT
>> 3) set both GENERIC_SPINLOCK_REFCOUNT and ARCH_SPINLOCK_REFCOUNT
>> 4) set only ARCH_SPINLOCK_REFCOUNT
>>
>> The customized parameters will be set in the "asm/spinlock_refcount.h" file.
>> Currently ,there is 2 parameters that can be customized for each architecture:
>> 1) How much time will the function wait until the lock is free
>> 2) How many attempts to do a lockless cmpxchg to update reference count
> Sigh. GENERIC means, that you use the generic implementation, ARCH
> means the architecture has a private implementation of that code.
>
> The generic implementation can use arch specific includes and if there
> is none we simply fallback to the asm-generic one.

I used the ARCH+GENERIC to mean using the generic code but with arch 
specific include.

>   >  Let's start with a simple version because it IS simple and easy
>>> to analyze and debug and then incrementally build improvements on it
>>> instead of creating an heuristics monster in the first place, i.e.:
>>>
>>>      if (!spin_is_locked(&lr->lock)&&   try_cmpxchg_once(lr))
>>>         return 0;
>>>      return 1;
>>>
>>> Take numbers for this on a zoo of different machines: Intel and AMD,
>>> old and new.
>>>
>>> Then you can add an incremental patch on that, which add loops and
>>> hoops. Along with numbers on the same zoo of machines.
>> I originally tried to do a cmpxchg without waiting and there was
>> practically no performance gain. I believe that as soon as
> Well, you did not see a difference on your particular machine. Still
> we don't have an idea how all of this works on a set of different
> machines. There is a world beside 8 socket servers.

I understand that. I can live with try_cmpxchg_once, though doing it 
twice will give a slightly better performance. However, without waiting 
for the lock to be free, this patch won't do much good. To keep it 
simple, I can remove the ability to do customization while doing cmpxchg 
once and wait until the lock is free. Please let me know if this is 
acceptable to you.

>> contention happens, it will force all the upcoming reference count
>> update threads to take the lock eliminating any potential
>> performance gain that we can have. To make it simple, I can change
>> the default to wait indefinitely until the lock is free instead of
>> looping a certain number of times, but I still like the option of
>> letting each architecture to decide how many times they want to
>> try. I agree that the actual waiting time even for one architecture
>> is depending on the actual CPU chip that is being used. I have done
>> some experiment on that. As long as the count is large enough,
>> increasing the loop count doesn't have any significant impact on
>> performance any more. The main reason for having a finite time is to
>> avoid having the waiting thread to wait virtually forever if the
>> lock happens to be busy for a long time.
> Again, if we make this tunable then we still want documentation for
> the behaviour on small, medium and large machines.
>
> Also what's the approach to tune that? Running some random testbench
> and monitor the results for various settings?
>
> If that's the case we better have a that as variables with generic
> initial values in the code, which can be modified by sysctl.

As I said above, I can remove the customization. I may reintroduce user 
customization using sysctl as you suggested in the a follow up patch 
after this one is merged.

>
>>>> +		getnstimeofday(&tv2);
>>>> +		ns = (tv2.tv_sec - tv1.tv_sec) * NSEC_PER_SEC +
>>>> +		     (tv2.tv_nsec - tv1.tv_nsec);
>>>> +		pr_info("lockref wait loop time = %lu ns\n", ns);
>>> Using getnstimeofday() for timestamping and spamming the logs with
>>> printouts? You can't be serious about that?
>>>
>>> Thats what tracepoints are for. Tracing is the only way to get proper
>>> numbers which take preemption, interrupts etc. into account without
>>> hurting runtime performace.
>> The _SHOW_WAIT_LOOP_TIME is for debugging and instrumentation purpose only
>> during development cycle. It is not supposed to be turned on at production
>> system. I will document that in the code.
> No, no, no! Again: That's what tracepoints are for.
>
> This kind of debugging is completely pointless. Tracepoints are
> designed to be low overhead and can be enabled on production
> systems.
>
> Your debugging depends on slow timestamps against CLOCK_REALTIME and
> an even slower output via printk. How useful is that if you want to
> really instrument the behaviour of this code?

This code is not critical and I can certainly remove it.

Regards,
Longman