Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging
On Mon 06-03-17 11:40:41, Daniel Vetter wrote: > On Mon, Mar 06, 2017 at 08:42:59AM +0100, Michal Hocko wrote: > > On Fri 03-03-17 09:37:55, Laura Abbott wrote: > > > On 03/03/2017 05:29 AM, Michal Hocko wrote: > > > > On Thu 02-03-17 13:44:32, Laura Abbott wrote: > > > >> Hi, > > > >> > > > >> There's been some recent discussions[1] about Ion-like frameworks. > > > >> There's > > > >> apparently interest in just keeping Ion since it works reasonablly > > > >> well. > > > >> This series does what should be the final clean ups for it to possibly > > > >> be > > > >> moved out of staging. > > > >> > > > >> This includes the following: > > > >> - Some general clean up and removal of features that never got a lot > > > >> of use > > > >> as far as I can tell. > > > >> - Fixing up the caching. This is the series I proposed back in > > > >> December[2] > > > >> but never heard any feedback on. It will certainly break existing > > > >> applications that rely on the implicit caching. I'd rather make an > > > >> effort > > > >> to move to a model that isn't going directly against the > > > >> establishement > > > >> though. > > > >> - Fixing up the platform support. The devicetree approach was never > > > >> well > > > >> recieved by DT maintainers. The proposal here is to think of Ion > > > >> less as > > > >> specifying requirements and more of a framework for exposing memory > > > >> to > > > >> userspace. > > > >> - CMA allocations now happen without the need of a dummy device > > > >> structure. > > > >> This fixes a bunch of the reasons why I attempted to add devicetree > > > >> support before. > > > >> > > > >> I've had problems getting feedback in the past so if I don't hear any > > > >> major > > > >> objections I'm going to send out with the RFC dropped to be picked up. > > > >> The only reason there isn't a patch to come out of staging is to > > > >> discuss any > > > >> other changes to the ABI people might want. Once this comes out of > > > >> staging, > > > >> I really don't want to mess with the ABI. > > > > > > > > Could you recapitulate concerns preventing the code being merged > > > > normally rather than through the staging tree and how they were > > > > addressed? > > > > > > > > > > Sorry, I'm really not understanding your question here, can you > > > clarify? > > > > There must have been a reason why this code ended up in the staging > > tree, right? So my question is what those reasons were and how they were > > handled in order to move the code from the staging subtree. > > No one gave a thing about android in upstream, so Greg KH just dumped it > all into staging/android/. We've discussed ION a bunch of times, recorded > anything we'd like to fix in staging/android/TODO, and Laura's patch > series here addresses a big chunk of that. Thanks for the TODO reference. I was looking exactly at something like that in drivers/staging/android/ion/. To bad I didn't look one directory up. Thanks for the clarification! -- Michal Hocko SUSE Labs
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging
On Fri 03-03-17 09:37:55, Laura Abbott wrote: > On 03/03/2017 05:29 AM, Michal Hocko wrote: > > On Thu 02-03-17 13:44:32, Laura Abbott wrote: > >> Hi, > >> > >> There's been some recent discussions[1] about Ion-like frameworks. There's > >> apparently interest in just keeping Ion since it works reasonablly well. > >> This series does what should be the final clean ups for it to possibly be > >> moved out of staging. > >> > >> This includes the following: > >> - Some general clean up and removal of features that never got a lot of use > >> as far as I can tell. > >> - Fixing up the caching. This is the series I proposed back in December[2] > >> but never heard any feedback on. It will certainly break existing > >> applications that rely on the implicit caching. I'd rather make an effort > >> to move to a model that isn't going directly against the establishement > >> though. > >> - Fixing up the platform support. The devicetree approach was never well > >> recieved by DT maintainers. The proposal here is to think of Ion less as > >> specifying requirements and more of a framework for exposing memory to > >> userspace. > >> - CMA allocations now happen without the need of a dummy device structure. > >> This fixes a bunch of the reasons why I attempted to add devicetree > >> support before. > >> > >> I've had problems getting feedback in the past so if I don't hear any major > >> objections I'm going to send out with the RFC dropped to be picked up. > >> The only reason there isn't a patch to come out of staging is to discuss > >> any > >> other changes to the ABI people might want. Once this comes out of staging, > >> I really don't want to mess with the ABI. > > > > Could you recapitulate concerns preventing the code being merged > > normally rather than through the staging tree and how they were > > addressed? > > > > Sorry, I'm really not understanding your question here, can you > clarify? There must have been a reason why this code ended up in the staging tree, right? So my question is what those reasons were and how they were handled in order to move the code from the staging subtree. -- Michal Hocko SUSE Labs
Re: [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging
On Thu 02-03-17 13:44:32, Laura Abbott wrote: > Hi, > > There's been some recent discussions[1] about Ion-like frameworks. There's > apparently interest in just keeping Ion since it works reasonablly well. > This series does what should be the final clean ups for it to possibly be > moved out of staging. > > This includes the following: > - Some general clean up and removal of features that never got a lot of use > as far as I can tell. > - Fixing up the caching. This is the series I proposed back in December[2] > but never heard any feedback on. It will certainly break existing > applications that rely on the implicit caching. I'd rather make an effort > to move to a model that isn't going directly against the establishement > though. > - Fixing up the platform support. The devicetree approach was never well > recieved by DT maintainers. The proposal here is to think of Ion less as > specifying requirements and more of a framework for exposing memory to > userspace. > - CMA allocations now happen without the need of a dummy device structure. > This fixes a bunch of the reasons why I attempted to add devicetree > support before. > > I've had problems getting feedback in the past so if I don't hear any major > objections I'm going to send out with the RFC dropped to be picked up. > The only reason there isn't a patch to come out of staging is to discuss any > other changes to the ABI people might want. Once this comes out of staging, > I really don't want to mess with the ABI. Could you recapitulate concerns preventing the code being merged normally rather than through the staging tree and how they were addressed? -- Michal Hocko SUSE Labs
Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
On Wed 19-10-16 10:23:55, Dave Hansen wrote: > On 10/19/2016 10:01 AM, Michal Hocko wrote: > > The question I had earlier was whether this has to be an explicit FOLL > > flag used by g-u-p users or we can just use it internally when mm != > > current->mm > > The reason I chose not to do that was that deferred work gets run under > a basically random 'current'. If we just use 'mm != current->mm', then > the deferred work will sometimes have pkeys enforced and sometimes not, > basically randomly. OK, I see (async_pf_execute and ksm ). It makes more sense to me. Thanks for the clarification. -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
On Wed 19-10-16 09:49:43, Dave Hansen wrote:
> On 10/19/2016 02:07 AM, Michal Hocko wrote:
> > On Wed 19-10-16 09:58:15, Lorenzo Stoakes wrote:
> >> On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
> >>> I am wondering whether we can go further. E.g. it is not really clear to
> >>> me whether we need an explicit FOLL_REMOTE when we can in fact check
> >>> mm != current->mm and imply that. Maybe there are some contexts which
> >>> wouldn't work, I haven't checked.
> >>
> >> This flag is set even when /proc/self/mem is used. I've not looked deeply
> >> into
> >> this flag but perhaps accessing your own memory this way can be considered
> >> 'remote' since you're not accessing it directly. On the other hand,
> >> perhaps this
> >> is just mistaken in this case?
> >
> > My understanding of the flag is quite limited as well. All I know it is
> > related to protection keys and it is needed to bypass protection check.
> > See arch_vma_access_permitted. See also 1b2ee1266ea6 ("mm/core: Do not
> > enforce PKEY permissions on remote mm access").
>
> Yeah, we need the flag to tell us when PKEYs should be applied or not.
> The current task's PKRU (pkey rights register) should really only be
> used to impact access to the task's memory, but has no bearing on how a
> given task should access remote memory.
The question I had earlier was whether this has to be an explicit FOLL
flag used by g-u-p users or we can just use it internally when mm !=
current->mm
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
On Wed 19-10-16 09:58:15, Lorenzo Stoakes wrote:
> On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
> > I am wondering whether we can go further. E.g. it is not really clear to
> > me whether we need an explicit FOLL_REMOTE when we can in fact check
> > mm != current->mm and imply that. Maybe there are some contexts which
> > wouldn't work, I haven't checked.
>
> This flag is set even when /proc/self/mem is used. I've not looked deeply into
> this flag but perhaps accessing your own memory this way can be considered
> 'remote' since you're not accessing it directly. On the other hand, perhaps
> this
> is just mistaken in this case?
My understanding of the flag is quite limited as well. All I know it is
related to protection keys and it is needed to bypass protection check.
See arch_vma_access_permitted. See also 1b2ee1266ea6 ("mm/core: Do not
enforce PKEY permissions on remote mm access").
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
On Wed 19-10-16 09:59:03, Jan Kara wrote: > On Thu 13-10-16 01:20:18, Lorenzo Stoakes wrote: > > This patch removes the write parameter from __access_remote_vm() and > > replaces it > > with a gup_flags parameter as use of this function previously _implied_ > > FOLL_FORCE, whereas after this patch callers explicitly pass this flag. > > > > We make this explicit as use of FOLL_FORCE can result in surprising > > behaviour > > (and hence bugs) within the mm subsystem. > > > > Signed-off-by: Lorenzo Stoakes > > So I'm not convinced this (and the following two patches) is actually > helping much. By grepping for FOLL_FORCE we will easily see that any caller > of access_remote_vm() gets that semantics and can thus continue search I am really wondering. Is there anything inherent that would require FOLL_FORCE for access_remote_vm? I mean FOLL_FORCE is a really non-trivial thing. It doesn't obey vma permissions so we should really minimize its usage. Do all of those users really need FOLL_FORCE? Anyway I would rather see the flag explicit and used at more places than hidden behind a helper function. -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
On Wed 19-10-16 10:06:46, Lorenzo Stoakes wrote: > On Wed, Oct 19, 2016 at 10:52:05AM +0200, Michal Hocko wrote: > > yes this is the desirable and expected behavior. > > > > > wonder if this is desirable behaviour or whether this ought to be limited > > > to > > > ptrace system calls. Regardless, by making the flag more visible it makes > > > it > > > easier to see that this is happening. > > > > mem_open already enforces PTRACE_MODE_ATTACH > > Ah I missed this, that makes a lot of sense, thanks! > > I still wonder whether other invocations of access_remote_vm() in > fs/proc/base.c > (the principle caller of this function) need FOLL_FORCE, for example the > various > calls that simply read data from other processes, so I think the point stands > about keeping this explicit. I do agree. Making them explicit will help to clean them up later, should there be a need. -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
On Wed 19-10-16 09:40:45, Lorenzo Stoakes wrote: > On Wed, Oct 19, 2016 at 10:13:52AM +0200, Michal Hocko wrote: > > On Wed 19-10-16 09:59:03, Jan Kara wrote: > > > On Thu 13-10-16 01:20:18, Lorenzo Stoakes wrote: > > > > This patch removes the write parameter from __access_remote_vm() and > > > > replaces it > > > > with a gup_flags parameter as use of this function previously _implied_ > > > > FOLL_FORCE, whereas after this patch callers explicitly pass this flag. > > > > > > > > We make this explicit as use of FOLL_FORCE can result in surprising > > > > behaviour > > > > (and hence bugs) within the mm subsystem. > > > > > > > > Signed-off-by: Lorenzo Stoakes > > > > > > So I'm not convinced this (and the following two patches) is actually > > > helping much. By grepping for FOLL_FORCE we will easily see that any > > > caller > > > of access_remote_vm() gets that semantics and can thus continue search > > > > I am really wondering. Is there anything inherent that would require > > FOLL_FORCE for access_remote_vm? I mean FOLL_FORCE is a really > > non-trivial thing. It doesn't obey vma permissions so we should really > > minimize its usage. Do all of those users really need FOLL_FORCE? > > I wonder about this also, for example by accessing /proc/self/mem you trigger > access_remote_vm() and consequently get_user_pages_remote() meaning FOLL_FORCE > is implied and you can use /proc/self/mem to override any VMA permissions. I yes this is the desirable and expected behavior. > wonder if this is desirable behaviour or whether this ought to be limited to > ptrace system calls. Regardless, by making the flag more visible it makes it > easier to see that this is happening. mem_open already enforces PTRACE_MODE_ATTACH -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
On Thu 13-10-16 01:20:10, Lorenzo Stoakes wrote:
> This patch series adjusts functions in the get_user_pages* family such that
> desired FOLL_* flags are passed as an argument rather than implied by flags.
>
> The purpose of this change is to make the use of FOLL_FORCE explicit so it is
> easier to grep for and clearer to callers that this flag is being used. The
> use
> of FOLL_FORCE is an issue as it overrides missing VM_READ/VM_WRITE flags for
> the
> VMA whose pages we are reading from/writing to, which can result in surprising
> behaviour.
>
> The patch series came out of the discussion around commit 38e0885, which
> addressed a BUG_ON() being triggered when a page was faulted in with PROT_NONE
> set but having been overridden by FOLL_FORCE. do_numa_page() was run on the
> assumption the page _must_ be one marked for NUMA node migration as an actual
> PROT_NONE page would have been dealt with prior to this code path, however
> FOLL_FORCE introduced a situation where this assumption did not hold.
>
> See https://marc.info/?l=linux-mm&m=147585445805166 for the patch proposal.
I like this cleanup. Tracking FOLL_FORCE users was always a nightmare
and the flag behavior is really subtle so we should better be explicit
about it. I haven't gone through each patch separately but rather
applied the whole series and checked the resulting diff. This all seems
OK to me and feel free to add
Acked-by: Michal Hocko
I am wondering whether we can go further. E.g. it is not really clear to
me whether we need an explicit FOLL_REMOTE when we can in fact check
mm != current->mm and imply that. Maybe there are some contexts which
wouldn't work, I haven't checked.
Then I am also wondering about FOLL_TOUCH behavior.
__get_user_pages_unlocked has only few callers which used to be
get_user_pages_unlocked before 1e9877902dc7e ("mm/gup: Introduce
get_user_pages_remote()"). To me a dropped FOLL_TOUCH seems
unintentional. Now that get_user_pages_unlocked has gup_flags argument I
guess we might want to get rid of the __g-u-p-u version altogether, no?
__get_user_pages is quite low level and imho shouldn't be exported. It's
only user - kvm - should rather pull those two functions to gup instead
and export them. There is nothing really KVM specific in them.
I also cannot say I would be entirely thrilled about get_user_pages_locked,
we only have one user which can simply do lock g-u-p unlock AFAICS.
I guess there is more work in that area and I do not want to impose all
that work on you, but I couldn't resist once I saw you playing in that
area ;) Definitely a good start!
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
