Re: [PATCH v2] mm: Introduce new function vm_insert_kmem_page

[Miguel Ojeda]

On Sat, Oct 6, 2018 at 7:11 AM Souptick Joarder <jrdr.linux@xxxxxxxxx> wrote:
>
> On Fri, Oct 5, 2018 at 11:39 PM Miguel Ojeda
> <miguel.ojeda.sandonis@xxxxxxxxx> wrote:
> > They are not supposed to be "steps". You did it with 70+ commits (!!)
> > over the course of several months. Why a tree wasn't created, stuff
> > developed there, and when done, submitted it for review?
>
> Because we already have a plan for entire vm_fault_t migration and
> the * instruction * was to send one patch per driver.

The instruction?

> >
> > Fine, but you haven't answered to the other parts of my email: you
> > don't explain why you choose one alternative over the others, you
> > simply keep changing the approach.
>
> We are going in circles here. That you want to convert vm_insert_page
> to vmf_insert_page for the PF case is fine and understood. However,
> you don't *need* to introduce a new name for the remaining non-PF
> cases if the function is going to be the exact same thing as before.
> You say "The final goal is to remove vm_insert_page", but you haven't
> justified *why* you need to remove that name.
>
> I think I have given that answer. If we don't remove vm_insert_page,
> future #PF caller will have option to use it. But those should be
> restricted. How are we going to restrict vm_insert_page in one half
> of kernel when other half is still using it  ?? Is there any way ? ( I don't
> know)

Ah, so that is what you are concerned about: future misuses. Well, I
don't really see the problem. There are only ~18 calls to
vm_insert_page() in the entire kernel: checking if people is using it
properly for a while should be easy. As long as the new behavior is
documented properly, it should be fine. If you are really concerned
about mistakes being made, then fine, we can rename it as I suggested.

Now, the new vm_insert_range() is another topic. It simplifies a few
of the callers and buys us the rename at the same time, so I am also
OK with it.

As you see, I am not against the changes -- it is just that they
should clearly justified. :-) It wasn't clear what your problem with
the current vm_insert_page() is.

Cheers,
Miguel