Re: RFC: PERCPU_LIST to fix PR kern/52515

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

Hi,

Thank you for your quick response.

On 2017/09/19 14:36, Taylor R Campbell wrote:
>> Date: Tue, 19 Sep 2017 13:38:15 +0900
>> From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
>>
>> To fix PR kern/52515(*), in particular psref(9), I implement
>> PERCPU_LIST which is dedicated for percpu_alloc'ed percpu struct.
>> Here is the patch which consists of PERCULI_LIST implementation and
>> applying to subr_psref.c.
>>     http://www.NetBSD.org/~knakahara/percpu-list/percpu-list.patch
>>
>> (*) http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=52515
> 
> Derp.  That's an embarrassing bug!  Especially since I knew that about
> percpu(9) and didn't connect the dots...
> 
> First, what you propose is a reasonable strategy for working around
> the problem, but I'm reluctant to commit to a new style of list
> abstraction for just this purpose.  Can you confine it to subr_psref.c
> for now, and retain only the operations that are absolutely necessary?
> E.g., no need for insert_before/after -- just insert_head, remove,
> foreach, empty.

Hmm, you are right. I thought percpu list can be use by other new
components in the future, however only psref(9) must have the
initialization dependency problem. Therefore, other components should
use pointer instead of itself in percpu(9) data like percpu_urandom_cprng
in sys/dev/rndpseudo.c.

> I've attached a small patch that might serve as a quicker stop-gap
> (untested).  It's gross, but it doesn't change the ABI, add any header
> files, &c.

I tested your patch(with tiny typo fix s/perpcu_getref/percpu_getref/),
it works fine in my workload, thanks!

> Second, we should maybe have a more robust way to handle per-CPU data
> that require nontrivial initialization or finalization so that it's
> not necessary to do this in the first place.  Maybe something like
> 
> struct percpu *percpu_create(size_t nbytes,
>     void (*init)(void *, struct cpu_info *),
>     void (*fini)(void *, struct cpu_info *));
> 
> with the callbacks invoked by percpu_cpu_enlarge.  Needs details
> filled out like can it fail to initialize or finalize and thereby
> block CPU online/offline?

I agree. I think it must be hard work...


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

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

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