linux-kernel - Re: [PATCH 09/10] percpu: implement new dynamic percpu allocator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200902192221.52835.rusty@rustcorp.com.au>
Date:	Thu, 19 Feb 2009 22:21:51 +1030
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Tejun Heo <tj@...nel.org>
Cc:	tglx@...utronix.de, x86@...nel.org, linux-kernel@...r.kernel.org,
	hpa@...or.com, jeremy@...p.org, cpw@....com, mingo@...e.hu,
	tony.luck@...el.com
Subject: Re: [PATCH 09/10] percpu: implement new dynamic percpu allocator

On Wednesday 18 February 2009 22:34:35 Tejun Heo wrote:
> Impact: new scalable dynamic percpu allocator which allows dynamic
>         percpu areas to be accessed the same way as static ones
> 
> Implement scalable dynamic percpu allocator which can be used for both
> static and dynamic percpu areas.  This will allow static and dynamic
> areas to share faster direct access methods.  This feature is optional
> and enabled only when CONFIG_HAVE_DYNAMIC_PER_CPU_AREA is defined by
> arch.  Please read comment on top of mm/percpu.c for details.

Hi Tejun,

   One question.  Are you thinking that to be defined by every SMP arch
long-term?  Because there are benefits in having &<percpuvar> == valid
percpuptr, such as passing them around as parameters.  If so, IA64
will want a dedicated per-cpu area for statics (tho it can probably just
map it somehow, but it has to be 64k).

   It'd also be nice to use your generalised module_percpu allocator for the
!CONFIG_HAVE_DYNAMIC_PER_CPU_AREA case, but doesn't really matter if that's
temporary anyway.

Direct comments follow:

> +static int __pcpu_unit_pages_shift = PCPU_MIN_UNIT_PAGES_SHIFT;
> +static int __pcpu_unit_pages;
> +static int __pcpu_unit_shift;
> +static int __pcpu_unit_size;
> +static int __pcpu_chunk_size;
> +static int __pcpu_nr_slots;
> +
> +/* currently everything is power of two, there's no hard dependency on it tho */
> +#define PCPU_UNIT_PAGES_SHIFT	((int)__pcpu_unit_pages_shift)
> +#define PCPU_UNIT_PAGES		((int)__pcpu_unit_pages)
> +#define PCPU_UNIT_SHIFT		((int)__pcpu_unit_shift)
> +#define PCPU_UNIT_SIZE		((int)__pcpu_unit_size)
> +#define PCPU_CHUNK_SIZE		((int)__pcpu_chunk_size)
> +#define PCPU_NR_SLOTS		((int)__pcpu_nr_slots)

These pseudo-constants seem like a really weird thing to do to me.

And AFAICT you have the requirement that PCPU_UNIT_PAGES*PAGE_SIZE >=
sizeof(.data.percpu).  Should probably note that somewhere.

> +static DEFINE_MUTEX(pcpu_mutex);		/* one mutex to rule them all */
> +static struct list_head *pcpu_slot;		/* chunk list slots */
> +static struct rb_root pcpu_addr_root = RB_ROOT;	/* chunks by address */

rbtree might be overkill on first cut.  I'm bearing in mind that Christoph L
had a nice patch to use dynamic percpu allocation in the sl*b allocators;
which would mean this needs to only use get_free_page.

Ah, I see akpm has responded.  I'll stop now and chain onto his comments
in the morning.

Thanks!
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/