On Mon, Jun 24, 2019 at 09:31:42PM +0800, Linus Torvalds wrote: > On Mon, Jun 24, 2019 at 3:43 PM Peter Xu <peterx@xxxxxxxxxx> wrote: > > > > Should we still be able to react on signal_pending() as part of fault > > handling (because that's what this patch wants to do, at least for an > > user-mode page fault)? Please kindly correct me if I misunderstood... > > I think that with this patch (modulo possible fix-ups) then yes, as > long as we're returning to user mode we can do signal_pending() and > return RETRY. > > But I think we really want to add a new FAULT_FLAG_INTERRUPTIBLE bit > for that (the same way we already have FAULT_FLAG_KILLABLE for things > that can react to fatal signals), and only do it when that is set. > Then the page fault handler can set that flag when it's doing a > user-mode page fault. > > Does that sound reasonable? Yes that sounds reasonable to me, and that matches perfectly with TASK_INTERRUPTIBLE and TASK_KILLABLE. The only thing that I am a bit uncertain is whether we should define FAULT_FLAG_INTERRUPTIBLE as a new bit or make it simply a combination of: FAULT_FLAG_KILLABLE | FAULT_FLAG_USER The problem is that when we do set_current_state() with either TASK_INTERRUPTIBLE or TASK_KILLABLE we'll only choose one of them, but never both. Here since the fault flag is a bitmask then if we introduce a new FAULT_FLAG_INTERRUPTIBLE bit and use it in the fault flags then we should probably be sure that FAULT_FLAG_KILLABLE is also set when with that (since IMHO it won't make much sense to make a page fault "interruptable" but "un-killable"...). Considering that TASK_INTERRUPTIBLE should also always in user-mode page faults so this dependency seems to exist with FAULT_FLAG_USER. Then I'm thinking maybe using the combination to express the meaning that "we would like this page fault to be interruptable, even for general userspace signals" would be nicer? AFAIK currently only handle_userfault() have such code to handle normal signals besides SIGKILL, and it was trying to detect this using this rule already: return_to_userland = (vmf->flags & (FAULT_FLAG_USER|FAULT_FLAG_KILLABLE)) == (FAULT_FLAG_USER|FAULT_FLAG_KILLABLE); Then if we define that globally and officially then we can probably replace this simply with: return_to_userland = vmf->flags & FAULT_FLAG_INTERRUPTIBLE; What do you think? Thanks, -- Peter Xu
