Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1346000 > unrolled thread

[PATCH] exit: clear TIF_MEMDIE after exit_task_work

Back to article view | Back to linux.kernel

Contents

  [PATCH] exit: clear TIF_MEMDIE after exit_task_work Vladimir Davydov <vdavydov@virtuozzo.com> - 2016-02-29 18:10 +0100
    Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Michal Hocko <mhocko@kernel.org> - 2016-02-29 19:30 +0100
      Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Michal Hocko <mhocko@kernel.org> - 2016-02-29 19:50 +0100
    Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Michal Hocko <mhocko@kernel.org> - 2016-03-01 17:00 +0100
      Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work "Michael S. Tsirkin" <mst@redhat.com> - 2016-03-01 17:00 +0100
        Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Michal Hocko <mhocko@kernel.org> - 2016-03-01 17:10 +0100
          Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work "Michael S. Tsirkin" <mst@redhat.com> - 2016-03-01 17:20 +0100
          Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work "Michael S. Tsirkin" <mst@redhat.com> - 2016-03-01 17:30 +0100
            Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Michal Hocko <mhocko@kernel.org> - 2016-03-01 17:40 +0100
              Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work "Michael S. Tsirkin" <mst@redhat.com> - 2016-03-01 17:50 +0100
                Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Michal Hocko <mhocko@kernel.org> - 2016-03-01 18:20 +0100
                  Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work "Michael S. Tsirkin" <mst@redhat.com> - 2016-03-01 18:30 +0100

#1346000 — [PATCH] exit: clear TIF_MEMDIE after exit_task_work

An mm_struct may be pinned by a file. An example is vhost-net device
created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
vhost_dev_set_owner). If such process gets OOM-killed, the reference to
its mm_struct will only be released from exit_task_work -> ____fput ->
__fput -> vhost_net_release -> vhost_dev_cleanup, which is called after
exit_mmap, where TIF_MEMDIE is cleared. As a result, we can start
selecting the next victim before giving the last one a chance to free
its memory. In practice, this leads to killing several VMs along with
the fattest one.

Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 kernel/exit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index fd90195667e1..cc50e12165f7 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -434,8 +434,6 @@ static void exit_mm(struct task_struct *tsk)
 	task_unlock(tsk);
 	mm_update_next_owner(mm);
 	mmput(mm);
-	if (test_thread_flag(TIF_MEMDIE))
-		exit_oom_victim(tsk);
 }
 
 static struct task_struct *find_alive_thread(struct task_struct *p)
@@ -746,6 +744,8 @@ void do_exit(long code)
 		disassociate_ctty(1);
 	exit_task_namespaces(tsk);
 	exit_task_work(tsk);
+	if (test_thread_flag(TIF_MEMDIE))
+		exit_oom_victim(tsk);
 	exit_thread();
 
 	/*
-- 
2.1.4

[toc] | [next] | [standalone]

#1346068

On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> An mm_struct may be pinned by a file. An example is vhost-net device
> created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> vhost_dev_set_owner). If such process gets OOM-killed, the reference to
> its mm_struct will only be released from exit_task_work -> ____fput ->
> __fput -> vhost_net_release -> vhost_dev_cleanup, which is called after
> exit_mmap, where TIF_MEMDIE is cleared. As a result, we can start
> selecting the next victim before giving the last one a chance to free
> its memory. In practice, this leads to killing several VMs along with
> the fattest one.

I am wondering why our PF_EXITING protection hasn't fired up. This is
not done in the mmotm tree but I guess you have seen the issue with the
linus tree, right? Do you have a log with oom reports available?

To be honest I do not feel very comfortable about moving the
exit_oom_victim even further down in do_exit path behind even less clear
locking or other dependencies.

Let's see if we can do any better for this particular case. 

> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> ---
>  kernel/exit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index fd90195667e1..cc50e12165f7 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -434,8 +434,6 @@ static void exit_mm(struct task_struct *tsk)
>  	task_unlock(tsk);
>  	mm_update_next_owner(mm);
>  	mmput(mm);
> -	if (test_thread_flag(TIF_MEMDIE))
> -		exit_oom_victim(tsk);
>  }
>  
>  static struct task_struct *find_alive_thread(struct task_struct *p)
> @@ -746,6 +744,8 @@ void do_exit(long code)
>  		disassociate_ctty(1);
>  	exit_task_namespaces(tsk);
>  	exit_task_work(tsk);
> +	if (test_thread_flag(TIF_MEMDIE))
> +		exit_oom_victim(tsk);
>  	exit_thread();
>  
>  	/*
> -- 
> 2.1.4

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1346087

On Mon 29-02-16 19:21:31, Michal Hocko wrote:
> On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > An mm_struct may be pinned by a file. An example is vhost-net device
> > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > vhost_dev_set_owner). If such process gets OOM-killed, the reference to
> > its mm_struct will only be released from exit_task_work -> ____fput ->
> > __fput -> vhost_net_release -> vhost_dev_cleanup, which is called after
> > exit_mmap, where TIF_MEMDIE is cleared. As a result, we can start
> > selecting the next victim before giving the last one a chance to free
> > its memory. In practice, this leads to killing several VMs along with
> > the fattest one.
> 
> I am wondering why our PF_EXITING protection hasn't fired up.

OK, I guess I can see it. exit_mm has done tsk->mm = NULL and so we are
skipping over that task because oom_scan_process_thread hasn't checked
PF_EXITING. I will try to think about this some more tomorrow with a
fresh brain.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1346713

[CCing vhost-net maintainer]

On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> An mm_struct may be pinned by a file. An example is vhost-net device
> created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> vhost_dev_set_owner).

The more I think about that the more I am wondering whether this is
actually OK and correct. Why does the driver have to pin the address
space? Nothing really prevents from parallel tearing down of the address
space anyway so the code cannot expect all the vmas to stay. Would it be
enough to pin the mm_struct only?

I am not sure I understand the code properly but what prevents from
the situation when a VHOST_SET_OWNER caller dies without calling
VHOST_RESET_OWNER and so the mm would be pinned indefinitely?

[Keeping the reset of the email for reference]

> If such process gets OOM-killed, the reference to
> its mm_struct will only be released from exit_task_work -> ____fput ->
> __fput -> vhost_net_release -> vhost_dev_cleanup, which is called after
> exit_mmap, where TIF_MEMDIE is cleared. As a result, we can start
> selecting the next victim before giving the last one a chance to free
> its memory. In practice, this leads to killing several VMs along with
> the fattest one.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> ---
>  kernel/exit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index fd90195667e1..cc50e12165f7 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -434,8 +434,6 @@ static void exit_mm(struct task_struct *tsk)
>  	task_unlock(tsk);
>  	mm_update_next_owner(mm);
>  	mmput(mm);
> -	if (test_thread_flag(TIF_MEMDIE))
> -		exit_oom_victim(tsk);
>  }
>  
>  static struct task_struct *find_alive_thread(struct task_struct *p)
> @@ -746,6 +744,8 @@ void do_exit(long code)
>  		disassociate_ctty(1);
>  	exit_task_namespaces(tsk);
>  	exit_task_work(tsk);
> +	if (test_thread_flag(TIF_MEMDIE))
> +		exit_oom_victim(tsk);
>  	exit_thread();
>  
>  	/*
> -- 
> 2.1.4

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1346716

On Tue, Mar 01, 2016 at 04:52:12PM +0100, Michal Hocko wrote:
> [CCing vhost-net maintainer]
> 
> On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > An mm_struct may be pinned by a file. An example is vhost-net device
> > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > vhost_dev_set_owner).
> 
> The more I think about that the more I am wondering whether this is
> actually OK and correct. Why does the driver have to pin the address
> space? Nothing really prevents from parallel tearing down of the address
> space anyway so the code cannot expect all the vmas to stay. Would it be
> enough to pin the mm_struct only?

I'll need to research this. It's a fact that as long as the
device is not stopped, vhost can attempt to access
the address space.

> I am not sure I understand the code properly but what prevents from
> the situation when a VHOST_SET_OWNER caller dies without calling
> VHOST_RESET_OWNER and so the mm would be pinned indefinitely?
> 
> [Keeping the reset of the email for reference]

We have:

static const struct file_operations vhost_net_fops = {
        .owner          = THIS_MODULE,
        .release        = vhost_net_release,
...
};

When caller dies and after fds are closed,
vhost_net_release calls vhost_dev_cleanup and that
drops the mm reference.

> > If such process gets OOM-killed, the reference to
> > its mm_struct will only be released from exit_task_work -> ____fput ->
> > __fput -> vhost_net_release -> vhost_dev_cleanup, which is called after
> > exit_mmap, where TIF_MEMDIE is cleared. As a result, we can start
> > selecting the next victim before giving the last one a chance to free
> > its memory. In practice, this leads to killing several VMs along with
> > the fattest one.
> > 
> > Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> > ---
> >  kernel/exit.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index fd90195667e1..cc50e12165f7 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -434,8 +434,6 @@ static void exit_mm(struct task_struct *tsk)
> >  	task_unlock(tsk);
> >  	mm_update_next_owner(mm);
> >  	mmput(mm);
> > -	if (test_thread_flag(TIF_MEMDIE))
> > -		exit_oom_victim(tsk);
> >  }
> >  
> >  static struct task_struct *find_alive_thread(struct task_struct *p)
> > @@ -746,6 +744,8 @@ void do_exit(long code)
> >  		disassociate_ctty(1);
> >  	exit_task_namespaces(tsk);
> >  	exit_task_work(tsk);
> > +	if (test_thread_flag(TIF_MEMDIE))
> > +		exit_oom_victim(tsk);
> >  	exit_thread();
> >  
> >  	/*
> > -- 
> > 2.1.4
> 
> -- 
> Michal Hocko
> SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1346718

On Tue 01-03-16 17:57:04, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2016 at 04:52:12PM +0100, Michal Hocko wrote:
> > [CCing vhost-net maintainer]
> > 
> > On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > > An mm_struct may be pinned by a file. An example is vhost-net device
> > > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > > vhost_dev_set_owner).
> > 
> > The more I think about that the more I am wondering whether this is
> > actually OK and correct. Why does the driver have to pin the address
> > space? Nothing really prevents from parallel tearing down of the address
> > space anyway so the code cannot expect all the vmas to stay. Would it be
> > enough to pin the mm_struct only?
> 
> I'll need to research this. It's a fact that as long as the
> device is not stopped, vhost can attempt to access
> the address space.

But does it expect any specific parts of the address space to be mapped?
E.g. proc needs to keep the mm allocated as well for some files but it
doesn't pin the address space (mm_users) but rather mm_count (see
proc_mem_open).

> > I am not sure I understand the code properly but what prevents from
> > the situation when a VHOST_SET_OWNER caller dies without calling
> > VHOST_RESET_OWNER and so the mm would be pinned indefinitely?
> > 
> > [Keeping the reset of the email for reference]
> 
> We have:
> 
> static const struct file_operations vhost_net_fops = {
>         .owner          = THIS_MODULE,
>         .release        = vhost_net_release,
> ...
> };
> 
> When caller dies and after fds are closed,
> vhost_net_release calls vhost_dev_cleanup and that
> drops the mm reference.

Can another process have the device open as well and prevent from
destruction?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1346731

On Tue, Mar 01, 2016 at 05:08:13PM +0100, Michal Hocko wrote:
> On Tue 01-03-16 17:57:04, Michael S. Tsirkin wrote:
> > On Tue, Mar 01, 2016 at 04:52:12PM +0100, Michal Hocko wrote:
> > > [CCing vhost-net maintainer]
> > > 
> > > On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > > > An mm_struct may be pinned by a file. An example is vhost-net device
> > > > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > > > vhost_dev_set_owner).
> > > 
> > > The more I think about that the more I am wondering whether this is
> > > actually OK and correct. Why does the driver have to pin the address
> > > space? Nothing really prevents from parallel tearing down of the address
> > > space anyway so the code cannot expect all the vmas to stay. Would it be
> > > enough to pin the mm_struct only?
> > 
> > I'll need to research this. It's a fact that as long as the
> > device is not stopped, vhost can attempt to access
> > the address space.
> 
> But does it expect any specific parts of the address space to be mapped?
> E.g. proc needs to keep the mm allocated as well for some files but it
> doesn't pin the address space (mm_users) but rather mm_count (see
> proc_mem_open).

As I said, I need to research this.

> > > I am not sure I understand the code properly but what prevents from
> > > the situation when a VHOST_SET_OWNER caller dies without calling
> > > VHOST_RESET_OWNER and so the mm would be pinned indefinitely?
> > > 
> > > [Keeping the reset of the email for reference]
> > 
> > We have:
> > 
> > static const struct file_operations vhost_net_fops = {
> >         .owner          = THIS_MODULE,
> >         .release        = vhost_net_release,
> > ...
> > };
> > 
> > When caller dies and after fds are closed,
> > vhost_net_release calls vhost_dev_cleanup and that
> > drops the mm reference.
> 
> Can another process have the device open as well and prevent from
> destruction?

Yes.

> -- 
> Michal Hocko
> SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1346732

On Tue, Mar 01, 2016 at 05:08:13PM +0100, Michal Hocko wrote:
> On Tue 01-03-16 17:57:04, Michael S. Tsirkin wrote:
> > On Tue, Mar 01, 2016 at 04:52:12PM +0100, Michal Hocko wrote:
> > > [CCing vhost-net maintainer]
> > > 
> > > On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > > > An mm_struct may be pinned by a file. An example is vhost-net device
> > > > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > > > vhost_dev_set_owner).
> > > 
> > > The more I think about that the more I am wondering whether this is
> > > actually OK and correct. Why does the driver have to pin the address
> > > space? Nothing really prevents from parallel tearing down of the address
> > > space anyway so the code cannot expect all the vmas to stay. Would it be
> > > enough to pin the mm_struct only?
> > 
> > I'll need to research this. It's a fact that as long as the
> > device is not stopped, vhost can attempt to access
> > the address space.
> 
> But does it expect any specific parts of the address space to be mapped?
> E.g. proc needs to keep the mm allocated as well for some files but it
> doesn't pin the address space (mm_users) but rather mm_count (see
> proc_mem_open).

At a quick glance, it seems that it's needed: it calls
get_user_pages(mm) and that looks like it will not DTRT (or even fail
gracefully) if mm->mm_users == 0 and exit_mmap/etc was already called
(or is in progress).

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1346741

On Tue 01-03-16 18:22:32, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2016 at 05:08:13PM +0100, Michal Hocko wrote:
> > On Tue 01-03-16 17:57:04, Michael S. Tsirkin wrote:
> > > On Tue, Mar 01, 2016 at 04:52:12PM +0100, Michal Hocko wrote:
> > > > [CCing vhost-net maintainer]
> > > > 
> > > > On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > > > > An mm_struct may be pinned by a file. An example is vhost-net device
> > > > > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > > > > vhost_dev_set_owner).
> > > > 
> > > > The more I think about that the more I am wondering whether this is
> > > > actually OK and correct. Why does the driver have to pin the address
> > > > space? Nothing really prevents from parallel tearing down of the address
> > > > space anyway so the code cannot expect all the vmas to stay. Would it be
> > > > enough to pin the mm_struct only?
> > > 
> > > I'll need to research this. It's a fact that as long as the
> > > device is not stopped, vhost can attempt to access
> > > the address space.
> > 
> > But does it expect any specific parts of the address space to be mapped?
> > E.g. proc needs to keep the mm allocated as well for some files but it
> > doesn't pin the address space (mm_users) but rather mm_count (see
> > proc_mem_open).
> 
> At a quick glance, it seems that it's needed: it calls
> get_user_pages(mm) and that looks like it will not DTRT (or even fail
> gracefully) if mm->mm_users == 0 and exit_mmap/etc was already called
> (or is in progress).

yes it will fail gracefully but what does prevent from munmap now? The
VMA can go away and get_user_pages would fail as well.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1346746

On Tue, Mar 01, 2016 at 05:35:37PM +0100, Michal Hocko wrote:
> On Tue 01-03-16 18:22:32, Michael S. Tsirkin wrote:
> > On Tue, Mar 01, 2016 at 05:08:13PM +0100, Michal Hocko wrote:
> > > On Tue 01-03-16 17:57:04, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 01, 2016 at 04:52:12PM +0100, Michal Hocko wrote:
> > > > > [CCing vhost-net maintainer]
> > > > > 
> > > > > On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > > > > > An mm_struct may be pinned by a file. An example is vhost-net device
> > > > > > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > > > > > vhost_dev_set_owner).
> > > > > 
> > > > > The more I think about that the more I am wondering whether this is
> > > > > actually OK and correct. Why does the driver have to pin the address
> > > > > space? Nothing really prevents from parallel tearing down of the address
> > > > > space anyway so the code cannot expect all the vmas to stay. Would it be
> > > > > enough to pin the mm_struct only?
> > > > 
> > > > I'll need to research this. It's a fact that as long as the
> > > > device is not stopped, vhost can attempt to access
> > > > the address space.
> > > 
> > > But does it expect any specific parts of the address space to be mapped?
> > > E.g. proc needs to keep the mm allocated as well for some files but it
> > > doesn't pin the address space (mm_users) but rather mm_count (see
> > > proc_mem_open).
> > 
> > At a quick glance, it seems that it's needed: it calls
> > get_user_pages(mm) and that looks like it will not DTRT (or even fail
> > gracefully) if mm->mm_users == 0 and exit_mmap/etc was already called
> > (or is in progress).
> 
> yes it will fail gracefully


What makes get_user_pages fail gracefully in this case,
if it races with task exiting?

> but what does prevent from munmap now? The
> VMA can go away and get_user_pages would fail as well.
> -- 
> Michal Hocko
> SUSE Labs

That's user error -> user gets -EFAULT.

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1346778

On Tue 01-03-16 18:46:38, Michael S. Tsirkin wrote:
> On Tue, Mar 01, 2016 at 05:35:37PM +0100, Michal Hocko wrote:
> > On Tue 01-03-16 18:22:32, Michael S. Tsirkin wrote:
> > > On Tue, Mar 01, 2016 at 05:08:13PM +0100, Michal Hocko wrote:
> > > > On Tue 01-03-16 17:57:04, Michael S. Tsirkin wrote:
> > > > > On Tue, Mar 01, 2016 at 04:52:12PM +0100, Michal Hocko wrote:
> > > > > > [CCing vhost-net maintainer]
> > > > > > 
> > > > > > On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > > > > > > An mm_struct may be pinned by a file. An example is vhost-net device
> > > > > > > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > > > > > > vhost_dev_set_owner).
> > > > > > 
> > > > > > The more I think about that the more I am wondering whether this is
> > > > > > actually OK and correct. Why does the driver have to pin the address
> > > > > > space? Nothing really prevents from parallel tearing down of the address
> > > > > > space anyway so the code cannot expect all the vmas to stay. Would it be
> > > > > > enough to pin the mm_struct only?
> > > > > 
> > > > > I'll need to research this. It's a fact that as long as the
> > > > > device is not stopped, vhost can attempt to access
> > > > > the address space.
> > > > 
> > > > But does it expect any specific parts of the address space to be mapped?
> > > > E.g. proc needs to keep the mm allocated as well for some files but it
> > > > doesn't pin the address space (mm_users) but rather mm_count (see
> > > > proc_mem_open).
> > > 
> > > At a quick glance, it seems that it's needed: it calls
> > > get_user_pages(mm) and that looks like it will not DTRT (or even fail
> > > gracefully) if mm->mm_users == 0 and exit_mmap/etc was already called
> > > (or is in progress).
> > 
> > yes it will fail gracefully
> 
> 
> What makes get_user_pages fail gracefully in this case,
> if it races with task exiting?

Sorry, I could have been more verbose... The code would have to make sure
that the mm is still alive before calling g-u-p by
atomic_inc_not_zero(&mm->mm_users) and fail if the user count dropped to
0 in the mean time. See how fs/proc/task_mmu.c does that (proc_mem_open
+ m_start + m_stop.

The biggest advanatage would be that the mm address space pin would be
only for the particular operation. Not sure whether that is possible in
the driver though. Anyway pinning the mm for a potentially unbounded
amount of time doesn't sound too nice.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1346788

On Tue, Mar 01, 2016 at 06:17:58PM +0100, Michal Hocko wrote:
> On Tue 01-03-16 18:46:38, Michael S. Tsirkin wrote:
> > On Tue, Mar 01, 2016 at 05:35:37PM +0100, Michal Hocko wrote:
> > > On Tue 01-03-16 18:22:32, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 01, 2016 at 05:08:13PM +0100, Michal Hocko wrote:
> > > > > On Tue 01-03-16 17:57:04, Michael S. Tsirkin wrote:
> > > > > > On Tue, Mar 01, 2016 at 04:52:12PM +0100, Michal Hocko wrote:
> > > > > > > [CCing vhost-net maintainer]
> > > > > > > 
> > > > > > > On Mon 29-02-16 20:02:09, Vladimir Davydov wrote:
> > > > > > > > An mm_struct may be pinned by a file. An example is vhost-net device
> > > > > > > > created by a qemu/kvm (see vhost_net_ioctl -> vhost_net_set_owner ->
> > > > > > > > vhost_dev_set_owner).
> > > > > > > 
> > > > > > > The more I think about that the more I am wondering whether this is
> > > > > > > actually OK and correct. Why does the driver have to pin the address
> > > > > > > space? Nothing really prevents from parallel tearing down of the address
> > > > > > > space anyway so the code cannot expect all the vmas to stay. Would it be
> > > > > > > enough to pin the mm_struct only?
> > > > > > 
> > > > > > I'll need to research this. It's a fact that as long as the
> > > > > > device is not stopped, vhost can attempt to access
> > > > > > the address space.
> > > > > 
> > > > > But does it expect any specific parts of the address space to be mapped?
> > > > > E.g. proc needs to keep the mm allocated as well for some files but it
> > > > > doesn't pin the address space (mm_users) but rather mm_count (see
> > > > > proc_mem_open).
> > > > 
> > > > At a quick glance, it seems that it's needed: it calls
> > > > get_user_pages(mm) and that looks like it will not DTRT (or even fail
> > > > gracefully) if mm->mm_users == 0 and exit_mmap/etc was already called
> > > > (or is in progress).
> > > 
> > > yes it will fail gracefully
> > 
> > 
> > What makes get_user_pages fail gracefully in this case,
> > if it races with task exiting?
> 
> Sorry, I could have been more verbose... The code would have to make sure
> that the mm is still alive before calling g-u-p by
> atomic_inc_not_zero(&mm->mm_users) and fail if the user count dropped to
> 0 in the mean time. See how fs/proc/task_mmu.c does that (proc_mem_open
> + m_start + m_stop.
> 
> The biggest advanatage would be that the mm address space pin would be
> only for the particular operation. Not sure whether that is possible in
> the driver though. Anyway pinning the mm for a potentially unbounded
> amount of time doesn't sound too nice.
> -- 
> Michal Hocko
> SUSE Labs

Hmm that would be another atomic on data path ...
I'd have to explore that.

-- 
MST

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web