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

Groups > linux.kernel > #1420748 > unrolled thread

Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Michal Hocko <mhocko@kernel.org> - 2016-06-13 14:00 +0200
    Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-13 16:00 +0200
      Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work Michal Hocko <mhocko@kernel.org> - 2016-06-13 16:10 +0200
    Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work "Michael S. Tsirkin" <mst@redhat.com> - 2016-06-13 20:20 +0200

#1420748 — Re: [PATCH] exit: clear TIF_MEMDIE after exit_task_work

On Tue 07-06-16 14:50:14, Michal Hocko wrote:
> On Mon 14-03-16 17:39:43, Michal Hocko wrote:
> > On Tue 01-03-16 19:20:24, Michael S. Tsirkin wrote:
> > > On Tue, Mar 01, 2016 at 06:17:58PM +0100, Michal Hocko wrote:
> > [...]
> > > > 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.
> > > 
> > > Hmm that would be another atomic on data path ...
> > > I'd have to explore that.
> > 
> > Did you have any chance to look into this?
> 
> So this is my take to get rid of mm_users pinning for an unbounded
> amount of time. This is even not compile tested. I am not sure how to
> handle when the mm goes away while there are still work items pending.
> It seems this is not handled current anyway and only shouts with a
> warning so this shouldn't cause a new regression AFAICS. I am not
> familiar with the vnet code at all so I might be missing many things,
> though. Does the below sound even remotely reasonable to you Michael?

I have checked the vnet code and it doesn't seem to rely on
copy_from_user/get_user AFAICS. Other users of use_mm() need to copy to
the userspace only as well. So we should be perfectly safe to OOM reap
address space even when it is shared by the kthread [1] so this is
not really needed for the OOM correctness purpose. It would be much
nicer if the kthread didn't pin the mm for two long outside of the OOM
handling as well of course but that lowers the priority of the change.

[1] http://lkml.kernel.org/r/20160613112348.GC6518@dhcp22.suse.cz
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1420870

Michal Hocko wrote:
> I have checked the vnet code and it doesn't seem to rely on
> copy_from_user/get_user AFAICS. Other users of use_mm() need to copy to
> the userspace only as well. So we should be perfectly safe to OOM reap
> address space even when it is shared by the kthread [1] so this is
> not really needed for the OOM correctness purpose. It would be much
> nicer if the kthread didn't pin the mm for two long outside of the OOM
> handling as well of course but that lowers the priority of the change.
> 
> [1] http://lkml.kernel.org/r/20160613112348.GC6518@dhcp22.suse.cz

It seems to me that vhost code relies on copy from the userspace.

use_mm(dev->mm) and unuse_mm(dev->mm) are used inside vhost_worker().
work->fn(work) is initialized by vhost_work_init().
vhost_scsi_open() passes vhost_scsi_complete_cmd_work() and
vhost_scsi_evt_work() as ->fn, and both functions call __get_user().

vhost_scsi_complete_cmd_work() {
  vhost_signal() {
    vhost_notify() {
      __get_user()
    }
  }
}

vhost_scsi_evt_work() {
  vhost_scsi_do_evt_work() {
    vhost_get_vq_desc() {
      __get_user() / __copy_from_user()
      get_indirect() {
        copy_from_iter()
      }
    }
  }
}

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

#1420886

On Mon 13-06-16 22:52:43, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > I have checked the vnet code and it doesn't seem to rely on
> > copy_from_user/get_user AFAICS. Other users of use_mm() need to copy to
> > the userspace only as well. So we should be perfectly safe to OOM reap
> > address space even when it is shared by the kthread [1] so this is
> > not really needed for the OOM correctness purpose. It would be much
> > nicer if the kthread didn't pin the mm for two long outside of the OOM
> > handling as well of course but that lowers the priority of the change.
> > 
> > [1] http://lkml.kernel.org/r/20160613112348.GC6518@dhcp22.suse.cz
> 
> It seems to me that vhost code relies on copy from the userspace.
> 
> use_mm(dev->mm) and unuse_mm(dev->mm) are used inside vhost_worker().
> work->fn(work) is initialized by vhost_work_init().
> vhost_scsi_open() passes vhost_scsi_complete_cmd_work() and
> vhost_scsi_evt_work() as ->fn, and both functions call __get_user().
> 
> vhost_scsi_complete_cmd_work() {
>   vhost_signal() {
>     vhost_notify() {
>       __get_user()
>     }
>   }
> }
> 
> vhost_scsi_evt_work() {
>   vhost_scsi_do_evt_work() {
>     vhost_get_vq_desc() {
>       __get_user() / __copy_from_user()
>       get_indirect() {
>         copy_from_iter()
>       }
>     }
>   }
> }

Ahh, I've missed those. Thanks for pointing this out! Let me try to find
out whether the code is robust to see unexpected 0 when reading from the
userspace.

-- 
Michal Hocko
SUSE Labs

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

#1421087

On Mon, Jun 13, 2016 at 01:50:41PM +0200, Michal Hocko wrote:
> On Tue 07-06-16 14:50:14, Michal Hocko wrote:
> > On Mon 14-03-16 17:39:43, Michal Hocko wrote:
> > > On Tue 01-03-16 19:20:24, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 01, 2016 at 06:17:58PM +0100, Michal Hocko wrote:
> > > [...]
> > > > > 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.
> > > > 
> > > > Hmm that would be another atomic on data path ...
> > > > I'd have to explore that.
> > > 
> > > Did you have any chance to look into this?
> > 
> > So this is my take to get rid of mm_users pinning for an unbounded
> > amount of time. This is even not compile tested. I am not sure how to
> > handle when the mm goes away while there are still work items pending.
> > It seems this is not handled current anyway and only shouts with a
> > warning so this shouldn't cause a new regression AFAICS. I am not
> > familiar with the vnet code at all so I might be missing many things,
> > though. Does the below sound even remotely reasonable to you Michael?
> 
> I have checked the vnet code and it doesn't seem to rely on
> copy_from_user/get_user AFAICS. Other users of use_mm() need to copy to
> the userspace only as well.

E.g. vhost_get_vq_desc calls __get_user quite a lot.
I think it should be safe for it to fail, but
not to trigger warnings/errors.

> So we should be perfectly safe to OOM reap
> address space even when it is shared by the kthread [1] so this is
> not really needed for the OOM correctness purpose. It would be much
> nicer if the kthread didn't pin the mm for two long outside of the OOM
> handling as well of course but that lowers the priority of the change.
> 
> [1] http://lkml.kernel.org/r/20160613112348.GC6518@dhcp22.suse.cz
> -- 
> Michal Hocko
> SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web