Re: Introducing localcount(9)

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

On 2017/05/12 7:04, Paul Goyette wrote:
> On Thu, 11 May 2017, Taylor R Campbell wrote:
> 
>>>>    (1) Why splsoftserial() is required instead of kpreempt_disable()?
>>>>        localcount_drain() uses xc_broadcast(0, ...), that is, it uses
>>>>        low priority xcall. Low priority xcall would be done by kthread
>>>>        context, so I think kpreempt_disable() would be sufficient to
>>>>        prevent localcount_drain() xcall running.
>>>
>>> I think you are correct.  Taylor, do you agree?
>>
>> Yes, I think this is fine.  I probably chose splsoftserial because I
>> was thinking of pserialize(9).
> 
> Committed.

Thank you for your commit. I believe this modification make it
more efficient. :)


>>>>    (2) Why both "localcount_adjust(lc, -1)" and "*lc->lc_totalp -= 1" are
>>>>        necessary in "lc->lc_totalp != NULL" case?
>>>>        I am afraid of below situation
>>>>            [1] CPU#A: mutex_enter(interlock)
>>>>            [2] CPU#A: call localcount_drain()
>>>>            [3] CPU#A: mutex_exit(interlock) before xc_broadcast
>>>>            [4] CPU#B: call localcount_release()
>>>>            [5] CPU#B: mutex_enter(interlock)
>>>>            [6] CPU#B: localcount_adjust(lc, -1)
>>>>            [7] CPU#B: *lc->lc_totalp -= 1
>>>>            [8] CPU#B: done localcount_release()
>>>>            [9] CPU#A: call xc_broadcast()
>>>>            [10]CPU#B: begin localcount_xc() and done
>>>>            [11]CPU#A: resume localcount_drain()
>>>>            [12]CPU#A: mutex_enter(interlock)
>>>>        Can "lc->lc_totalp" be -1 at this point?
>>>
>>> localcount_adjust() updates the local CPU's reference counter, while
>>> *lc->lc_totalp is the global sum that we are accumulating.  We want to
>>> keep them synchronized.
>>
>> What knakahara is worried about -- rightly, I think -- is the
>> difference between the following two scenarios:
>>
>> 1. CPU A: localcount_drain      (total=0, a=0, b=1)
>>   CPU B: localcount_xc         (total=1, a=0, b=1)
>>   CPU B: localcount_release    (total=0, a=0, b=0)
>>
>> 2. CPU A: localcount_drain      (total=0, a=0, b=1)
>>   CPU B: localcount_release    (total=-1, a=0, b=0)
>>   CPU B: localcount_xc         (total=-1, a=0, b=0)
>>
>> If localcount_xc has already run on the local CPU, then we do want to
>> keep them in sync in localcount_release, which is why I wrote the
>> logic I did.  But if it has not yet run, we don't want to double-count
>> the release in the global and the local counts, which is what
>> knakahara observed.
> 
> Ah, yes, got it!

I am sorry for lack of words...
Thank you for more clearly explanation, riastradh@n.o!


>> The problem is that in localcount_release, we don't know whether the
>> total includes the local count yet, and thus whether when subtracting
>> one from the local count we also need to subtract one from the total
>> count.
>>
>> One possible way to remedy this -- haven't thought this through yet,
>> obviously needs more attention -- is to change localcount_xc to
>> *transfer* rather than add the local references,
>>
>>        mutex_enter(interlock);
>>        localp = percpu_getref(lc->lc_percpu);
>>        *lc->lc_totalp += *localp;
>>        *localp -= *localp;   /* i.e., *localp = 0 */
>>        percpu_putref(lc->lc_percpu);
>>        mutex_exit(interlock);
>>
>> and to change localcount_release to decrement only the total count
>> when it is available:
>>
>>        mutex_enter(interlock);
>>        if (--*lc->lc_totalp == 0)
>>                cv_broadcast(cv);
>>        mutex_exit(interlock);
>>
>> Then the scenarios become:
>>
>> 1'. CPU A: localcount_drain     (total=0, a=0, b=1)
>>    CPU B: localcount_xc        (total=1, a=0, b=0)
>>    CPU B: localcount_release   (total=0, a=0, b=0)
>>
>> 2'. CPU A: localcount_drain     (total=0, a=0, b=1)
>>    CPU B: localcount_release   (total=-1, a=0, b=1)
>>    CPU B: localcount_xc        (total=0, a=0, b=0)
> 
> Makes sense.  I've made appropriate changes, and will rebuild and 
> re-test.

LGTM, too. Thank you for your fixing.


Thanks,
 
-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>