Messages in this thread | | | Date | Mon, 15 Jul 2013 21:40:33 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v5 01/12] spinlock: A new lockref structure for lockless update of refcount |
| |
On 07/15/2013 07:47 PM, Thomas Gleixner wrote: > On Mon, 15 Jul 2013, Waiman Long wrote: >> On 07/15/2013 10:41 AM, Thomas Gleixner wrote: >>> On Mon, 8 Jul 2013, Waiman Long wrote: >>> 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. > And what's the point of that? I just explained it to you that you do > not need the ARCH=y and GENERIC=y at all.
As I said in my previous mail, I can remove the ARCH+GENERIC option.
>>> > 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 > I asked you several times now to explain and document the whole thing > with numbers instead of your handwaving "slightly better performance" > arguments.
I will provide performance data for 1 and 2 retries in my next patch version.
> >> 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. > No, it's not acceptable at all if you are not able to provide data for > 1,2,4,8 socket machines (from single core to your precious big > boxes). It's that simple. We are not accepting patches which optimize > for a single use case and might negatively affect 99,9999% of the > existing users which have no access to this kind of hardware unless > proven otherwise.
I did provide performance data for 1,2,4 and 8 socket configurations in my commit message. I used numactl to simulate different socket configuration by forcing the code to use only a subset of total number of sockets. I know that is not ideal, but I think it should be close enough. I will provide performance data on a more common 2 socket test machine that I have.
Yes, I don't provide data for single-thread use case. I will also provide that data in my next version by measuring the average time for doing low-level reference count update using lock and lockless update like what I had done for the qrwlock patch. For single thread case, I don't believe any real workload will show any appreciable difference in performance due to the differing reference count update mechanisms.
>>> 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. > And I asked for a step by step approach in the first review, but you > decided to ignore that. And now you think that it's accetable for you > as long as you get what you want. That's not the way it works, really.
I am trying to provide what you are asking for while at the same time meet my own need.
>>>>>> + 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? >>>>> > q> > > > 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. > Did you even try to understand what I wrote? I did not ask you to > remove instrumentation. I asked you to use useful instrumentation > instead of some totally useless crap.
I am not that familiar with using the tracepoints instrumentation for timing measurement. I will try to use that in the code for that purpose.
Regards, Longman
| |