Re: [PATCH v2] x86/gart/kcore: Exclude GART aperture from kcore

[Kairui Song]

On Tue, Feb 19, 2019 at 4:00 PM Kairui Song <kasong@xxxxxxxxxx> wrote:
>
> On Thu, Jan 24, 2019 at 10:17 AM Baoquan He <bhe@xxxxxxxxxx> wrote:
> >
> > On 01/23/19 at 10:50pm, Kairui Song wrote:
> > > > >  int fix_aperture __initdata = 1;
> > > > >
> > > > > -#ifdef CONFIG_PROC_VMCORE
> > > > > +#if defined(CONFIG_PROC_VMCORE) || defined(CONFIG_PROC_KCORE)
> > > > >  /*
> > > > >   * If the first kernel maps the aperture over e820 RAM, the kdump kernel will
> > > > >   * use the same range because it will remain configured in the northbridge.
> > > > > @@ -66,7 +67,7 @@ int fix_aperture __initdata = 1;
> > > > >   */
> > > > >  static unsigned long aperture_pfn_start, aperture_page_count;
> > > > >
> > > > > -static int gart_oldmem_pfn_is_ram(unsigned long pfn)
> > > > > +static int gart_mem_pfn_is_ram(unsigned long pfn)
> > > > >  {
> > > > >       return likely((pfn < aperture_pfn_start) ||
> > > > >                     (pfn >= aperture_pfn_start + aperture_page_count));
> > > > > @@ -76,7 +77,12 @@ static void exclude_from_vmcore(u64 aper_base, u32 aper_order)
> > > >
> > > > Shouldn't this function name be changed? It's not only handling vmcore
> > > > stuff any more, but also kcore. And this function is not excluding, but
> > > > resgistering.
> > > >
> > > > Other than this, it looks good to me.
> > > >
> > > > Thanks
> > > > Baoquan
> > > >
> > >
> > > Good suggestion, it's good to change this function name too to avoid
> > > any misleading. This patch hasn't got any other reviews recently, I'll
> > > update it shortly.
> >
> > There's more.
> >
> > These two are doing the same thing:
> >   register_mem_pfn_is_ram
> >   register_oldmem_pfn_is_ram
> >
> > Need remove one of them and put it in a right place. Furthermore, may
> > need see if there's existing function which is used to register a
> > function to a hook.
> >
> > Secondly, exclude_from_vmcore() is not excluding anthing, it's only
> > registering a function which is used to judge if oldmem/pfn is ram. Need
> > rename it.
> >
> > Thanks
> > Baoquan
>

Hi Baoquan, after second thought, vmcore and kcore are doing similar
thing but still quite independent of each, didn't see any simple way
to share the logic.
And for the following naming issue I think considering the context
there is no problem, "exclude_from_vmcore(aper_alloc, aper_order)" is
clearly doing what it literally means, excluding the aperture from
vmcore.

Let me know if anything is wrong, will send V4 later reuse this approach.

--
Best Regards,
Kairui Song