Messages in this thread | | | Date | Wed, 22 Apr 2020 22:23:57 -0700 | From | Andrei Vagin <> | Subject | Re: [PATCH 3/6] arm64/vdso: Add time napespace page |
| |
On Thu, Apr 16, 2020 at 11:45:27AM +0100, Mark Rutland wrote: > Hi Andrei, > > On Wed, Apr 15, 2020 at 10:26:15PM -0700, Andrei Vagin wrote: > > diff --git a/arch/arm64/include/asm/vdso.h b/arch/arm64/include/asm/vdso.h > > index 07468428fd29..351c145d3808 100644 > > --- a/arch/arm64/include/asm/vdso.h > > +++ b/arch/arm64/include/asm/vdso.h ... > > +#ifdef CONFIG_TIME_NS > > +static __always_inline const struct vdso_data *__arch_get_timens_vdso_data(void) > > +{ > > + const struct vdso_data *ret; > > + > > + ret = _timens_data; > > + OPTIMIZER_HIDE_VAR(ret); > > + > > + return ret; > > +} > > +#endif > > Sorry for the confusion here, but please either: > > * Add a preparatory patch making __arch_get_vdso_data() use > OPTIMIZER_HIDE_VAR(), and use OPTIMIZER_HIDE_VAR() here. > > * Use the same assembly as __arch_get_vdso_data() currently does. > > ... and either way add a comment here: > > /* See __arch_get_vdso_data() */ > > ... so taht the rationale is obvious. > > [...] > > > +enum vvar_pages { > > + VVAR_DATA_PAGE_OFFSET = 0, > > +#ifdef CONFIG_TIME_NS > > + VVAR_TIMENS_PAGE_OFFSET = 1, > > +#endif /* CONFIG_TIME_NS */ > > + VVAR_NR_PAGES = __VVAR_PAGES, > > +}; > > Pet peeve, but we don't need the initializers here, as enums start from > zero. The last element shouldn't have a trailing comma as we don't > expect to add elements after it in future. > > Rather than assigning to VVAR_NR_PAGES, it'd be better to use a > BUILD_BUG_ON() to verify that it is the number we expect: > > enum vvar_pages { > VVAR_DATA_PAGE, > #ifdef CONFIG_TIME_NS > VVAR_TIMENS_PAGE, > #endif > VVAR_NR_PAGES > }; > > BUILD_BUG_ON(VVAR_NR_PAGES != __VVAR_PAGES);
Hi Mark,
Thank you for the review. I have sent a fixed version of this patch in replay to the origin patch.
Thanks, Andrei
| |