All of lore.kernel.org
 help / color / mirror / Atom feed
From: Catalin Marinas <catalin.marinas@arm.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andreas Gruenbacher <agruenba@redhat.com>,
	Matthew Wilcox <willy@infradead.org>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Will Deacon <will@kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	linux-btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH 3/3] btrfs: Avoid live-lock in search_ioctl() on hardware with sub-page faults
Date: Mon, 29 Nov 2021 23:12:41 +0000	[thread overview]
Message-ID: <YaVeafjbutiVXavv@arm.com> (raw)
In-Reply-To: <CAHk-=wjZ6zME2SzohM1P_-B0BNi2JJgvz22ypF-EuAQiVKipRg@mail.gmail.com>

On Mon, Nov 29, 2021 at 01:53:01PM -0800, Linus Torvalds wrote:
> On Mon, Nov 29, 2021 at 12:56 PM Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > I think that would be useful, though it doesn't solve the potential
> > livelock with sub-page faults.
> 
> I was assuming we'd just do the sub-page faults.
> 
> In fact, I was assuming we'd basically just replace all the PAGE_ALIGN
> and PAGE_SIZE with SUBPAGE_{ALIGN,SIZE}, together with something like
> 
>         if (size > PAGE_SIZE)
>                 size = PAGE_SIZE;
> 
> to limit that size thing (or possibly make that "min size" be a
> parameter, so that people who have things like that "I need at least
> this initial structure to be copied" issue can document their minimum
> size needs).

Ah, so fault_in_writeable() would never fault in the whole range (if too
large). When copy_to_user() goes beyond the faulted in range, it may
fail and we go back to fault in a bit more of the range. A copy loop
would be equivalent to:

	fault_addr = ubuf;
	end = ubuf + size;
	while (1) {
		if (fault_in_writeable(fault_addr,
				       min(PAGE_SIZE, end - fault_addr)))
			break;
		left = copy_to_user(ubuf, kbuf, size);
		if (!left)
			break;
		fault_addr = end - left;
	}

That should work. I'll think about it tomorrow, getting late over here.

(I may still keep the sub-page probing in the arch code, see my earlier
exchanges with Andreas)

-- 
Catalin

WARNING: multiple messages have this Message-ID (diff)
From: Catalin Marinas <catalin.marinas@arm.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andreas Gruenbacher <agruenba@redhat.com>,
	Matthew Wilcox <willy@infradead.org>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Will Deacon <will@kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	linux-btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH 3/3] btrfs: Avoid live-lock in search_ioctl() on hardware with sub-page faults
Date: Mon, 29 Nov 2021 23:12:41 +0000	[thread overview]
Message-ID: <YaVeafjbutiVXavv@arm.com> (raw)
In-Reply-To: <CAHk-=wjZ6zME2SzohM1P_-B0BNi2JJgvz22ypF-EuAQiVKipRg@mail.gmail.com>

On Mon, Nov 29, 2021 at 01:53:01PM -0800, Linus Torvalds wrote:
> On Mon, Nov 29, 2021 at 12:56 PM Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > I think that would be useful, though it doesn't solve the potential
> > livelock with sub-page faults.
> 
> I was assuming we'd just do the sub-page faults.
> 
> In fact, I was assuming we'd basically just replace all the PAGE_ALIGN
> and PAGE_SIZE with SUBPAGE_{ALIGN,SIZE}, together with something like
> 
>         if (size > PAGE_SIZE)
>                 size = PAGE_SIZE;
> 
> to limit that size thing (or possibly make that "min size" be a
> parameter, so that people who have things like that "I need at least
> this initial structure to be copied" issue can document their minimum
> size needs).

Ah, so fault_in_writeable() would never fault in the whole range (if too
large). When copy_to_user() goes beyond the faulted in range, it may
fail and we go back to fault in a bit more of the range. A copy loop
would be equivalent to:

	fault_addr = ubuf;
	end = ubuf + size;
	while (1) {
		if (fault_in_writeable(fault_addr,
				       min(PAGE_SIZE, end - fault_addr)))
			break;
		left = copy_to_user(ubuf, kbuf, size);
		if (!left)
			break;
		fault_addr = end - left;
	}

That should work. I'll think about it tomorrow, getting late over here.

(I may still keep the sub-page probing in the arch code, see my earlier
exchanges with Andreas)

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-11-29 23:16 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-24 19:20 [PATCH 0/3] Avoid live-lock in fault-in+uaccess loops with sub-page faults Catalin Marinas
2021-11-24 19:20 ` Catalin Marinas
2021-11-24 19:20 ` [PATCH 1/3] mm: Introduce fault_in_exact_writeable() to probe for " Catalin Marinas
2021-11-24 19:20   ` Catalin Marinas
2021-11-24 19:20 ` [PATCH 2/3] arm64: Add support for sub-page faults user probing Catalin Marinas
2021-11-24 19:20   ` Catalin Marinas
2021-11-24 19:20 ` [PATCH 3/3] btrfs: Avoid live-lock in search_ioctl() on hardware with sub-page faults Catalin Marinas
2021-11-24 19:20   ` Catalin Marinas
2021-11-24 20:03   ` Matthew Wilcox
2021-11-24 20:03     ` Matthew Wilcox
2021-11-24 20:37     ` Catalin Marinas
2021-11-24 20:37       ` Catalin Marinas
2021-11-25 22:25       ` Andreas Gruenbacher
2021-11-25 22:25         ` Andreas Gruenbacher
2021-11-25 22:42         ` Catalin Marinas
2021-11-25 22:42           ` Catalin Marinas
2021-11-26 22:29           ` Andreas Gruenbacher
2021-11-26 22:29             ` Andreas Gruenbacher
2021-11-26 22:57             ` Catalin Marinas
2021-11-26 22:57               ` Catalin Marinas
2021-11-27  3:52               ` Andreas Gruenbacher
2021-11-27  3:52                 ` Andreas Gruenbacher
2021-11-27 12:39                 ` Andreas Gruenbacher
2021-11-27 12:39                   ` Andreas Gruenbacher
2021-11-27 15:21                   ` Catalin Marinas
2021-11-27 15:21                     ` Catalin Marinas
2021-11-27 18:05                     ` Andreas Gruenbacher
2021-11-27 18:05                       ` Andreas Gruenbacher
2021-11-29 12:16                       ` Catalin Marinas
2021-11-29 12:16                         ` Catalin Marinas
2021-11-29 13:33                         ` Andreas Gruenbacher
2021-11-29 13:33                           ` Andreas Gruenbacher
2021-11-29 15:36                           ` Catalin Marinas
2021-11-29 15:36                             ` Catalin Marinas
2021-11-29 18:40                             ` Linus Torvalds
2021-11-29 18:40                               ` Linus Torvalds
2021-11-29 19:31                               ` Andreas Gruenbacher
2021-11-29 19:31                                 ` Andreas Gruenbacher
2021-11-29 20:56                               ` Catalin Marinas
2021-11-29 20:56                                 ` Catalin Marinas
2021-11-29 21:53                                 ` Linus Torvalds
2021-11-29 21:53                                   ` Linus Torvalds
2021-11-29 23:12                                   ` Catalin Marinas [this message]
2021-11-29 23:12                                     ` Catalin Marinas
2021-11-29 13:52                       ` Catalin Marinas
2021-11-29 13:52                         ` Catalin Marinas
2021-11-27 14:33                 ` Catalin Marinas
2021-11-27 14:33                   ` Catalin Marinas
2021-11-24 23:00     ` Linus Torvalds
2021-11-24 23:00       ` Linus Torvalds
2021-11-25 11:10       ` Catalin Marinas
2021-11-25 11:10         ` Catalin Marinas
2021-11-25 18:13         ` Linus Torvalds
2021-11-25 18:13           ` Linus Torvalds
2021-11-25 20:43           ` Catalin Marinas
2021-11-25 20:43             ` Catalin Marinas
2021-11-25 21:02             ` Matthew Wilcox
2021-11-25 21:02               ` Matthew Wilcox
2021-11-25 21:29               ` Catalin Marinas
2021-11-25 21:29                 ` Catalin Marinas
2021-11-25 21:40               ` Andreas Gruenbacher
2021-11-25 21:40                 ` Andreas Gruenbacher
2021-11-26 16:42   ` David Sterba
2021-11-26 16:42     ` David Sterba
2021-11-24 21:36 ` [PATCH 0/3] Avoid live-lock in fault-in+uaccess loops " Andrew Morton
2021-11-24 21:36   ` Andrew Morton
2021-11-24 22:31   ` Catalin Marinas
2021-11-24 22:31     ` Catalin Marinas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YaVeafjbutiVXavv@arm.com \
    --to=catalin.marinas@arm.com \
    --cc=agruenba@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=dsterba@suse.com \
    --cc=josef@toxicpanda.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=will@kernel.org \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.