On Thu, Feb 20, 2020 at 3:18 PM Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx> wrote:
>
> Hi all,
>
> On Sun, 16 Feb 2020 22:45:35 -0800 Arjun Roy <arjunroy@xxxxxxxxxx> wrote:
> >
> > On Sun, Feb 16, 2020 at 8:12 PM Arjun Roy <arjunroy@xxxxxxxxxx> wrote:
> > >
> > > On Sun, Feb 16, 2020 at 7:57 PM Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > After merging the akpm tree, today's linux-next build (sparc64 defconfig)
> > > > failed like this:
> > > >
> > > > mm/memory.c: In function 'insert_pages':
> > > > mm/memory.c:1523:56: error: macro "pte_index" requires 2 arguments, but only 1 given
> > > > remaining_pages_total, PTRS_PER_PTE - pte_index(addr));
> > > > ^
> > > >
> > > > Caused by commit
> > > >
> > > > 366142f0b000 ("mm/memory.c: add vm_insert_pages()")
> > > >
> > > > This is the first use of pte_index() outside arch specific code and the
> > > > sparc64 version of pte_index() nas an extra argument.
> > >
> > > Looks like this happens for sparc, and also metag. Other platforms
> > > just take the addr parameter based on a quick search.
> >
> > And actually I guess there's no metag anyways now.
> > Looking further, then, it looks like in every non-sparc pte_index() is
> > an actual numerical index, while on sparc it goes a step further to
> > yield a pte_t *.
> > As far as I can tell, the sparc incarnation of this is only used by
> > the pte_offset_(kernel/map) macros.
> >
> > So I think a possibly sane way to fix this would be:
> > 1. Define pte_index() to be a numerical index, like the other architectures,
> > 2. Define something like pte_entry() that uses pte_index(), and
> > 3. Have pte_offset_(kernel/map) be defined as pte_entry() instead.
> >
> > Then pte_index would be operating on just an address for all
> > platforms, and the reverted patchset would work without any changes.
> >
> > If this sounds acceptable, I can send a patch.
> >
> > > > I have reverted these commits for today:
> > > >
> > > > 219ae14a9686 ("net-zerocopy-use-vm_insert_pages-for-tcp-rcv-zerocopy-fix")
> > > > cb912fdf96bf ("net-zerocopy: use vm_insert_pages() for tcp rcv zerocopy")
> > > > 72c684430b94 ("add missing page_count() check to vm_insert_pages().")
> > > > dbd9553775f3 ("mm-add-vm_insert_pages-fix")
> > > > 366142f0b000 ("mm/memory.c: add vm_insert_pages()")
> > > >
> > >
> > > In terms of fixing this; passing in an appropriate dir parameter is
> > > not really a problem, but what is concerning that it seems messy to
> > > have a per-platform ifdef to pass it either two arguments or one in
> > > this case. But it seems like either that would be one way to fix it,
> > > or having some arch method across all arches that takes two arguments
> > > (and ignores one of them for most arches).
> > >
> > > Is there a general preference for the right way forward, in this case?
>
> Has there been any progress with this? I am still reverting the above
> commits, so they are not getting any linux-next testing ...
>
I have a possible solution in mind, but it would involve a slight
change in the SPARC macro (to be more inline with the semantics of the
other platforms).
If you're open to such a change, I can send it out.
Thanks,
-Arjun
> --
> Cheers,
> Stephen Rothwell
