on 15/05/2011 19:24 Andriy Gapon said the following: > on 15/05/2011 19:09 Max Laier said the following: >> >> I don't think we ever intended to synchronize the local teardown part, and I >> believe that is the correct behavior for this API. >> >> This version is sufficiently close to what I have, so I am resonably sure that >> it will work for us. It seems, however, that if we move to check to after >> picking up the lock anyway, the generation approach has even less impact and I >> am starting to prefer that solution. >> >> Andriy, is there any reason why you'd prefer your approach over the generation >> version? > > No reason. And I even haven't said that I prefer it :-) > I just wanted to show and explain it as apparently there was some > misunderstanding about it. I think that generation count approach could even > have a little bit better performance while perhaps being a tiny bit less obvious. Resurrecting this conversation. Here's a link to the thread on gmane for your convenience: http://article.gmane.org/gmane.os.freebsd.current/132682 It seems that we haven't fully accounted for the special case of master CPU behavior in neither my proposed fix that added a new wait count nor in the committed fix that has added a generation count. This has been pointed out by kib at the recent mini-DevSummit in Kyiv. The problem can be illustrated by the following example: - the teardown function is a "real" function; that is, not NULL and not smp_rendevous_no_ barrier (sic) - the arg parameter is a non-NULL pointer - the teardown function actually accesses memory pointed to by the arg - the master CPU deallocates memory pointed by the arg right after smp_rendezvous() call returns (either via free(9) or by stack management, etc) Quoting the code: MPASS(generation == smp_rv_generation); atomic_add_int(&smp_rv_waiters[2], 1); if (local_teardown_func != smp_no_rendevous_barrier) { while (smp_rv_waiters[2] < smp_rv_ncpus && generation == smp_rv_generation) cpu_spinwait(); if (local_teardown_func != NULL) local_teardown_func(local_func_arg); } Generation count helps slave CPUs to not get stuck at the while loop, but it doesn't prevent stale/different arg being passed to the teardown function. So here is a patch, which is a variation of my earlier proposal, that should fix this issue: http://people.freebsd.org/~avg/smp_rendezvous.diff The patch is not tested yet. The patch undoes the smp_rv_generation change and introduces a new smp_rv_waiters[] element. Perhaps the code could be optimized by making waiting on smp_rv_waiters[3] conditional on some combination of values for the teardown and the arg, but I decided to follow the KISS principle here to make the code a little bit more robust. -- Andriy GaponReceived on Mon Jul 18 2011 - 09:34:15 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:15 UTC