Re: [PATCH 0/5] arm64: vdso: cleanups

From: Will Deacon <will@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: catalin.marinas@arm.com, vincenzo.frascino@arm.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 0/5] arm64: vdso: cleanups
Date: Tue, 14 Apr 2020 16:52:48 +0100	[thread overview]
Message-ID: <20200414155247.GA30881@willie-the-truck> (raw)
In-Reply-To: <20200414154319.GK2486@C02TD0UTHF1T.local>

On Tue, Apr 14, 2020 at 04:43:19PM +0100, Mark Rutland wrote:
> On Tue, Apr 14, 2020 at 04:20:58PM +0100, Will Deacon wrote:
> > On Tue, Apr 14, 2020 at 11:42:47AM +0100, Mark Rutland wrote:
> > > While attempting to review an arm64 vdso patch, I noticed some of the existing
> > > code was somewhat baroque, making it harder than necessary to understand and
> > > extend. These patches attempt to improve this by making the code more
> > > consistent and avoiding unnecessary duplication.
> > > 
> > > The first patch in the series fixes a bug in a boot time error path. This bug
> > > was made obvious during the refactoring but I've moved it to the start so that
> > > it can be backported more easily.
> > > 
> > > The series is based on v5.7-rc1 and can be found in my arm64/vdso-cleanup
> > > branch [1].
> > 
> > Cheers, this looks really good to me. The only thing I'm slightly confused
> > by is that we still have something like this in __vdso_init():
> > 
> > 	/* Allocate the vDSO pagelist, plus a page for the data. */
> > 	vdso_pagelist = kcalloc(vdso_lookup[arch_index].vdso_pages + 1,
> > 				sizeof(struct page *),
> > 				GFP_KERNEL);
> > 
> > But I don't see why this needs to be dynamic, and don't we leak the
> > allocation on failure? Not a big deal, but seems silly if we could just
> > have a couple of static page * arrays.
> 
> Unfortunately it has to be dynamic as the number of vdso code pages
> isn't known until the vdso is linked into the kernel proper. The only
> way to allocate that at build time would be as part of the linker
> script, and I think that'd be far more confusing.

I was wondering whether we could extend gen_vdso_offsets.sh to emit this
information. Why isn't looking at the shared object enough? That said,
it does get grotty so maybe it's not worth doing.

> We calculate the number of pages from the bounds of the code:
> 
> |         vdso_info[abi].vdso_pages = (
> |                         vdso_info[abi].vdso_code_end -
> |                         vdso_info[abi].vdso_code_start) >>
> |                         PAGE_SHIFT;
> | 
> |         /* Allocate the vDSO pagelist, plus a page for the data. */
> |         vdso_pagelist = kcalloc(vdso_info[abi].vdso_pages + 1,
> |                                 sizeof(struct page *),
> |                                 GFP_KERNEL);
> 
> ... that said, conflating the data page list and code page list is
> harming legibility, and I reckon that's worth cleaning up. The data page
> list can be statically allocate given it's a single page.
> 
> There is a leak if we fail to allocate the compat vdso pages, but I
> don't see a nice way of cleaning that up. It looks like
> do_one_initcall() will WARN() in that case as we'll return a non-zero
> error code.

Yeah, I'm not really worried about that since I think we only fail if a
subsequent allocation fails, it's just that the dynamic allocation seemed
a bit OTT on first glance.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel