* Re: [PATCH] mm, vmalloc: fix vmalloc users tracking properly
From: Geert Uytterhoeven @ 2017-05-10 9:37 UTC (permalink / raw)
To: Michal Hocko
Cc: Linus Torvalds, Andrew Morton, Tobias Klauser, Linux MM, LKML
In-Reply-To: <20170509153702.GR6481@dhcp22.suse.cz>
On Tue, May 9, 2017 at 5:37 PM, Michal Hocko <mhocko@kernel.org> wrote:
> Sigh. I've apparently managed to screw up again. This should address the
> nommu breakage reported by 0-day.
> ---
> From 95d49bf93ae4467f3f918520ec03b3596e5b36cc Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 9 May 2017 16:27:39 +0200
> Subject: [PATCH] mm, vmalloc: fix vmalloc users tracking properly
>
> 1f5307b1e094 ("mm, vmalloc: properly track vmalloc users") has pulled
> asm/pgtable.h include dependency to linux/vmalloc.h and that turned out
> to be a bad idea for some architectures. E.g. m68k fails with
> In file included from arch/m68k/include/asm/pgtable_mm.h:145:0,
> from arch/m68k/include/asm/pgtable.h:4,
> from include/linux/vmalloc.h:9,
> from arch/m68k/kernel/module.c:9:
> arch/m68k/include/asm/mcf_pgtable.h: In function 'nocache_page':
>>> arch/m68k/include/asm/mcf_pgtable.h:339:43: error: 'init_mm' undeclared (first use in this function)
> #define pgd_offset_k(address) pgd_offset(&init_mm, address)
>
> as spotted by kernel build bot. nios2 fails for other reason
> In file included from ./include/asm-generic/io.h:767:0,
> from ./arch/nios2/include/asm/io.h:61,
> from ./include/linux/io.h:25,
> from ./arch/nios2/include/asm/pgtable.h:18,
> from ./include/linux/mm.h:70,
> from ./include/linux/pid_namespace.h:6,
> from ./include/linux/ptrace.h:9,
> from ./arch/nios2/include/uapi/asm/elf.h:23,
> from ./arch/nios2/include/asm/elf.h:22,
> from ./include/linux/elf.h:4,
> from ./include/linux/module.h:15,
> from init/main.c:16:
> ./include/linux/vmalloc.h: In function '__vmalloc_node_flags':
> ./include/linux/vmalloc.h:99:40: error: 'PAGE_KERNEL' undeclared (first use in this function); did you mean 'GFP_KERNEL'?
>
> which is due to the newly added #include <asm/pgtable.h>, which on nios2
> includes <linux/io.h> and thus <asm/io.h> and <asm-generic/io.h> which
> again includes <linux/vmalloc.h>.
>
> Tweaking that around just turns out a bigger headache than
> necessary. This patch reverts 1f5307b1e094 and reimplements the original
> fix in a different way. __vmalloc_node_flags can stay static inline
> which will cover vmalloc* functions. We only have one external user
> (kvmalloc_node) and we can export __vmalloc_node_flags_caller and
> provide the caller directly. This is much simpler and it doesn't really
> need any games with header files.
>
> Fixes: 1f5307b1e094 ("mm, vmalloc: properly track vmalloc users")
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm, vmalloc: fix vmalloc users tracking properly
From: Tobias Klauser @ 2017-05-10 9:51 UTC (permalink / raw)
To: Michal Hocko; +Cc: Linus Torvalds, Andrew Morton, linux-mm, LKML
In-Reply-To: <20170509153702.GR6481@dhcp22.suse.cz>
On 2017-05-09 at 17:37:02 +0200, Michal Hocko <mhocko@kernel.org> wrote:
> Sigh. I've apparently managed to screw up again. This should address the
> nommu breakage reported by 0-day.
> ---
> From 95d49bf93ae4467f3f918520ec03b3596e5b36cc Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 9 May 2017 16:27:39 +0200
> Subject: [PATCH] mm, vmalloc: fix vmalloc users tracking properly
>
> 1f5307b1e094 ("mm, vmalloc: properly track vmalloc users") has pulled
> asm/pgtable.h include dependency to linux/vmalloc.h and that turned out
> to be a bad idea for some architectures. E.g. m68k fails with
> In file included from arch/m68k/include/asm/pgtable_mm.h:145:0,
> from arch/m68k/include/asm/pgtable.h:4,
> from include/linux/vmalloc.h:9,
> from arch/m68k/kernel/module.c:9:
> arch/m68k/include/asm/mcf_pgtable.h: In function 'nocache_page':
> >> arch/m68k/include/asm/mcf_pgtable.h:339:43: error: 'init_mm' undeclared (first use in this function)
> #define pgd_offset_k(address) pgd_offset(&init_mm, address)
>
> as spotted by kernel build bot. nios2 fails for other reason
> In file included from ./include/asm-generic/io.h:767:0,
> from ./arch/nios2/include/asm/io.h:61,
> from ./include/linux/io.h:25,
> from ./arch/nios2/include/asm/pgtable.h:18,
> from ./include/linux/mm.h:70,
> from ./include/linux/pid_namespace.h:6,
> from ./include/linux/ptrace.h:9,
> from ./arch/nios2/include/uapi/asm/elf.h:23,
> from ./arch/nios2/include/asm/elf.h:22,
> from ./include/linux/elf.h:4,
> from ./include/linux/module.h:15,
> from init/main.c:16:
> ./include/linux/vmalloc.h: In function '__vmalloc_node_flags':
> ./include/linux/vmalloc.h:99:40: error: 'PAGE_KERNEL' undeclared (first use in this function); did you mean 'GFP_KERNEL'?
>
> which is due to the newly added #include <asm/pgtable.h>, which on nios2
> includes <linux/io.h> and thus <asm/io.h> and <asm-generic/io.h> which
> again includes <linux/vmalloc.h>.
>
> Tweaking that around just turns out a bigger headache than
> necessary. This patch reverts 1f5307b1e094 and reimplements the original
> fix in a different way. __vmalloc_node_flags can stay static inline
> which will cover vmalloc* functions. We only have one external user
> (kvmalloc_node) and we can export __vmalloc_node_flags_caller and
> provide the caller directly. This is much simpler and it doesn't really
> need any games with header files.
>
> Fixes: 1f5307b1e094 ("mm, vmalloc: properly track vmalloc users")
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Tested-by: Tobias Klauser <tklauser@distanz.ch>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 01/27] fs: remove unneeded forward definition of mm_struct from fs.h
From: Jan Kara @ 2017-05-10 11:04 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170509154930.29524-2-jlayton@redhat.com>
On Tue 09-05-17 11:49:04, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> include/linux/fs.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7251f7bb45e8..38adefd8e2a0 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1252,8 +1252,6 @@ extern void f_delown(struct file *filp);
> extern pid_t f_getown(struct file *filp);
> extern int send_sigurg(struct fown_struct *fown);
>
> -struct mm_struct;
> -
> /*
> * Umount options
> */
> --
> 2.9.3
>
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 05/27] btrfs: btrfs_wait_tree_block_writeback can be void return
From: Jan Kara @ 2017-05-10 11:09 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170509154930.29524-6-jlayton@redhat.com>
On Tue 09-05-17 11:49:08, Jeff Layton wrote:
> Nothing checks its return value.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/btrfs/disk-io.c | 6 +++---
> fs/btrfs/disk-io.h | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index eb1ee7b6f532..8c479bd5534a 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1222,10 +1222,10 @@ int btrfs_write_tree_block(struct extent_buffer *buf)
> buf->start + buf->len - 1);
> }
>
> -int btrfs_wait_tree_block_writeback(struct extent_buffer *buf)
> +void btrfs_wait_tree_block_writeback(struct extent_buffer *buf)
> {
> - return filemap_fdatawait_range(buf->pages[0]->mapping,
> - buf->start, buf->start + buf->len - 1);
> + filemap_fdatawait_range(buf->pages[0]->mapping,
> + buf->start, buf->start + buf->len - 1);
> }
>
> struct extent_buffer *read_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr,
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 2e0ec29bfd69..9cc87835abb5 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -127,7 +127,7 @@ int btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct inode *inode,
> extent_submit_bio_hook_t *submit_bio_done);
> unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info);
> int btrfs_write_tree_block(struct extent_buffer *buf);
> -int btrfs_wait_tree_block_writeback(struct extent_buffer *buf);
> +void btrfs_wait_tree_block_writeback(struct extent_buffer *buf);
> int btrfs_init_log_root_tree(struct btrfs_trans_handle *trans,
> struct btrfs_fs_info *fs_info);
> int btrfs_add_log_tree(struct btrfs_trans_handle *trans,
> --
> 2.9.3
>
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 11/27] fuse: set mapping error in writepage_locked when it fails
From: Jan Kara @ 2017-05-10 11:13 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170509154930.29524-12-jlayton@redhat.com>
On Tue 09-05-17 11:49:14, Jeff Layton wrote:
> This ensures that we see errors on fsync when writeback fails.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/fuse/file.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index ec238fb5a584..07d0efcb050c 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
> err_free:
> fuse_request_free(req);
> err:
> + mapping_set_error(page->mapping, error);
> end_page_writeback(page);
> return error;
> }
> --
> 2.9.3
>
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 12/27] cifs: set mapping error when page writeback fails in writepage or launder_pages
From: Jan Kara @ 2017-05-10 11:14 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170509154930.29524-13-jlayton@redhat.com>
On Tue 09-05-17 11:49:15, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/cifs/file.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 21d404535739..0bee7f8d91ad 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2234,14 +2234,16 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc)
> set_page_writeback(page);
> retry_write:
> rc = cifs_partialpagewrite(page, 0, PAGE_SIZE);
> - if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL)
> - goto retry_write;
> - else if (rc == -EAGAIN)
> + if (rc == -EAGAIN) {
> + if (wbc->sync_mode == WB_SYNC_ALL)
> + goto retry_write;
> redirty_page_for_writepage(wbc, page);
> - else if (rc != 0)
> + } else if (rc != 0) {
> SetPageError(page);
> - else
> + mapping_set_error(page->mapping, rc);
> + } else {
> SetPageUptodate(page);
> + }
> end_page_writeback(page);
> put_page(page);
> free_xid(xid);
> --
> 2.9.3
>
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 13/27] lib: add errseq_t type and infrastructure for handling it
From: Jeff Layton @ 2017-05-10 11:29 UTC (permalink / raw)
To: NeilBrown, linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4,
linux-cifs, linux-nfs, linux-mm, jfs-discussion, linux-xfs,
cluster-devel, linux-f2fs-devel, v9fs-developer, linux-nilfs,
linux-block
Cc: dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <87inl9n0wu.fsf@notabene.neil.brown.name>
On Wed, 2017-05-10 at 08:03 +1000, NeilBrown wrote:
> On Tue, May 09 2017, Jeff Layton wrote:
>
> > An errseq_t is a way of recording errors in one place, and allowing any
> > number of "subscribers" to tell whether an error has been set again
> > since a previous time.
> >
> > It's implemented as an unsigned 32-bit value that is managed with atomic
> > operations. The low order bits are designated to hold an error code
> > (max size of MAX_ERRNO). The upper bits are used as a counter.
> >
> > The API works with consumers sampling an errseq_t value at a particular
> > point in time. Later, that value can be used to tell whether new errors
> > have been set since that time.
> >
> > Note that there is a 1 in 512k risk of collisions here if new errors
> > are being recorded frequently, since we have so few bits to use as a
> > counter. To mitigate this, one bit is used as a flag to tell whether the
> > value has been sampled since a new value was recorded. That allows
> > us to avoid bumping the counter if no one has sampled it since it
> > was last bumped.
> >
> > Later patches will build on this infrastructure to change how writeback
> > errors are tracked in the kernel.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>
> I like that this is a separate lib/*.c - nicely structured too.
>
> Reviewed-by: NeilBrown <neilb@suse.com>
>
>
Thanks, yeah...it occurred to me that this scheme is not really specific
to writeback errors. While I can't think of another use-case for
errseq_t's right offhand, I think this makes for cleaner layering and
should make it easy to use in other ways should they arise.
--
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 13/27] lib: add errseq_t type and infrastructure for handling it
From: Jan Kara @ 2017-05-10 11:34 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170509154930.29524-14-jlayton@redhat.com>
On Tue 09-05-17 11:49:16, Jeff Layton wrote:
> An errseq_t is a way of recording errors in one place, and allowing any
> number of "subscribers" to tell whether an error has been set again
> since a previous time.
>
> It's implemented as an unsigned 32-bit value that is managed with atomic
> operations. The low order bits are designated to hold an error code
> (max size of MAX_ERRNO). The upper bits are used as a counter.
>
> The API works with consumers sampling an errseq_t value at a particular
> point in time. Later, that value can be used to tell whether new errors
> have been set since that time.
>
> Note that there is a 1 in 512k risk of collisions here if new errors
> are being recorded frequently, since we have so few bits to use as a
> counter. To mitigate this, one bit is used as a flag to tell whether the
> value has been sampled since a new value was recorded. That allows
> us to avoid bumping the counter if no one has sampled it since it
> was last bumped.
>
> Later patches will build on this infrastructure to change how writeback
> errors are tracked in the kernel.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
The patch looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Just two nits below:
...
> +int errseq_check_and_advance(errseq_t *eseq, errseq_t *since)
> +{
> + int err = 0;
> + errseq_t old, new;
> +
> + /*
> + * Most callers will want to use the inline wrapper to check this,
> + * so that the common case of no error is handled without needing
> + * to lock.
> + */
I'm not sure which locking you are speaking about here. Is the comment
stale?
> + old = READ_ONCE(*eseq);
> + if (old != *since) {
> + /*
> + * Set the flag and try to swap it into place if it has
> + * changed.
> + *
> + * We don't care about the outcome of the swap here. If the
> + * swap doesn't occur, then it has either been updated by a
> + * writer who is bumping the seq count anyway, or another
"bumping the seq count anyway" part is not quite true. Writer may see
ERRSEQ_SEEN not set and so just update the error code and leave seq count
as is. But since you compare full errseq_t for equality, this works out as
designed...
> + * reader who is just setting the "seen" flag. Either outcome
> + * is OK, and we can advance "since" and return an error based
> + * on what we have.
> + */
> + new = old | ERRSEQ_SEEN;
> + if (new != old)
> + cmpxchg(eseq, old, new);
> + *since = new;
> + err = -(new & MAX_ERRNO);
> + }
> + return err;
> +}
> +EXPORT_SYMBOL(errseq_check_and_advance);
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: RFC v2: post-init-read-only protection for data allocated dynamically
From: Michal Hocko @ 2017-05-10 11:43 UTC (permalink / raw)
To: Igor Stoppa
Cc: Dave Hansen, Laura Abbott, linux-mm, linux-kernel,
kernel-hardening@lists.openwall.com
In-Reply-To: <885311a2-5b9f-4402-0a71-5a3be7870aa0@huawei.com>
On Wed 10-05-17 11:57:42, Igor Stoppa wrote:
> On 10/05/17 11:05, Michal Hocko wrote:
[...]
> > To me it seems that this being an initialization mostly thingy a simple
> > allocator which manages a pool of pages (one set of sealed and one for
> > allocations)
>
> Shouldn't also the set of pages used for keeping track of the others be
> sealed? Once one is ro, also the other should not change.
Heh, that really depends how much consistency and robustness you want to
achieve. It is really hard to defend against targeted attacks against
the allocator metadata when a code is running in the kernel.
> > and which only appends new objects as they fit to unsealed
> > pages would be sufficient for starter.
>
> Any "free" that might happen during the initialization transient, would
> actually result in an untracked gap, right?
yes. And once the whole page is free it would get unsealed and returned
to the (page) allocator. This approach would inevitably lead to internal
fragmentation but reducing that would require a pool which is shared for
objects with the common life cycle which is quite hard with requirements
you have (you would have to convey the allocation context to all users
somehow).
> What about the size of the pool of pages?
I wouldn't see that as a big deal. New pages would be allocated as
needed.
> No predefined size, instead request a new page, when the memory
> remaining from the page currently in use is not enough to fit the latest
> allocation request?
exactly
> There are also two aspect we discussed earlier:
>
> - livepatch: how to deal with it? Identify the page it wants to modify
> and temporarily un-protect it?
Livepatch doesn't support data structures patching currently and even if
it would have to understand those data structures and do something like
copy&replace...
> - modules: unloading and reloading modules will eventually lead to
> permanently lost pages, in increasing number.
Each module should free all objects that were allocated on its behalf
and that should result in pages being freed as well
> Loading/unloading repeatedly the same module is probably not so common,
> with a major exception being USB, where almost anything can show up.
> And disappear.
> This seems like a major showstopper for the linear allocator you propose.
I am not sure I understand. If such a module kept allocations behind it
would be a memory leak no matter what.
> My reasoning in pursuing the kmalloc approach was that it is already
> equipped with mechanisms for dealing with these sort of cases, where
> memory can be fragmented.
Yeah, but kmalloc is optimized for a completely different usecase. You
can reuse same pages again and again while you clearly cannot do the
same once you seal a page and make it read only. Well unless you want to
open time windows when the page stops being RO or use a different
mapping for the allocator.
But try to consider how many features of the slab allocator you are
actually going to need wrt. to tweaks it would have to implement to
support this new use case. Maybe duplicating general purpose caches and
creating specialized explicitly is a viable path. I haven't tried
it.
> I also wouldn't risk introducing bugs with my homebrew allocator ...
>
> The initial thought was that there could be a master toggle to
> seal/unseal all the memory affected.
>
> But you were not too excited, iirc :-D
yes, If there are different users a pool (kmem_cache like) would be more
natural.
> Alternatively, kmalloc could be enhanced to unseal only the pages it
> wants to modify.
You would have to stop the world to prevent from an accidental overwrite
during that time. Which makes the whole thing quite dubious IMHO.
> I don't think much can be done for data that is placed together, in the
> same page with something that needs to be altered.
> But what is outside of that page could still enjoy the protection from
> the seal.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 3/3] arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
From: Will Deacon @ 2017-05-10 11:55 UTC (permalink / raw)
To: Catalin Marinas
Cc: Florian Fainelli, Michal Hocko, open list, Kirill A. Shutemov,
Ard Biesheuvel, Russell King, Chris Wilson,
open list:MEMORY MANAGEMENT, zijun_hu, angus, Andrey Ryabinin,
Andrew Morton, linux-arm-kernel
In-Reply-To: <20170510083803.ur44myyb35lqcuw7@localhost>
On Wed, May 10, 2017 at 09:38:03AM +0100, Catalin Marinas wrote:
> On Mon, May 08, 2017 at 11:07:24AM +0100, Will Deacon wrote:
> > On Fri, May 05, 2017 at 02:07:28PM -0700, Florian Fainelli wrote:
> > > On 05/03/2017 04:18 AM, Will Deacon wrote:
> > > > On Thu, Apr 27, 2017 at 11:19:02AM -0700, Florian Fainelli wrote:
> > > >> When CONFIG_ARM64_MODULE_PLTS is enabled, the first allocation using the
> > > >> module space fails, because the module is too big, and then the module
> > > >> allocation is attempted from vmalloc space. Silence the first allocation
> > > >> failure in that case by setting __GFP_NOWARN.
> > > >>
> > > >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > >> ---
> > > >> arch/arm64/kernel/module.c | 7 ++++++-
> > > >> 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > I'm not sure what the merge plan is for these, but the arm64 bit here
> > > > looks fine to me:
> > > >
> > > > Acked-by: Will Deacon <will.deacon@arm.com>
> > >
> > > Thanks, not sure either, would you or Catalin want to pick this series?
> >
> > We'd need an Ack from Russell on the arch/arm/ part before we could take
> > this series.
>
> The first patch touches mm/vmalloc.c, so we could also merge the series
> via akpm's tree. Andrew, do you have any preference?
Michal Hocko acked that one, so I think we can take the whole series via
arm64.
Will
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 14/27] fs: new infrastructure for writeback error handling and reporting
From: Jan Kara @ 2017-05-10 11:48 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170509154930.29524-15-jlayton@redhat.com>
On Tue 09-05-17 11:49:17, Jeff Layton wrote:
> Most filesystems currently use mapping_set_error and
> filemap_check_errors for setting and reporting/clearing writeback errors
> at the mapping level. filemap_check_errors is indirectly called from
> most of the filemap_fdatawait_* functions and from
> filemap_write_and_wait*. These functions are called from all sorts of
> contexts to wait on writeback to finish -- e.g. mostly in fsync, but
> also in truncate calls, getattr, etc.
>
> The non-fsync callers are problematic. We should be reporting writeback
> errors during fsync, but many places spread over the tree clear out
> errors before they can be properly reported, or report errors at
> nonsensical times.
>
> If I get -EIO on a stat() call, there is no reason for me to assume that
> it is because some previous writeback failed. The fact that it also
> clears out the error such that a subsequent fsync returns 0 is a bug,
> and a nasty one since that's potentially silent data corruption.
>
> This patch adds a small bit of new infrastructure for setting and
> reporting errors during address_space writeback. While the above was my
> original impetus for adding this, I think it's also the case that
> current fsync semantics are just problematic for userland. Most
> applications that call fsync do so to ensure that the data they wrote
> has hit the backing store.
>
> In the case where there are multiple writers to the file at the same
> time, this is really hard to determine. The first one to call fsync will
> see any stored error, and the rest get back 0. The processes with open
> fds may not be associated with one another in any way. They could even
> be in different containers, so ensuring coordination between all fsync
> callers is not really an option.
>
> One way to remedy this would be to track what file descriptor was used
> to dirty the file, but that's rather cumbersome and would likely be
> slow. However, there is a simpler way to improve the semantics here
> without incurring too much overhead.
>
> This set adds an errseq_t to struct address_space, and a corresponding
> one is added to struct file. Writeback errors are recorded in the
> mapping's errseq_t, and the one in struct file is used as the "since"
> value.
>
> This changes the semantics of the Linux fsync implementation such that
> applications can now use it to determine whether there were any
> writeback errors since fsync(fd) was last called (or since the file was
> opened in the case of fsync having never been called).
>
> Note that those writeback errors may have occurred when writing data
> that was dirtied via an entirely different fd, but that's the case now
> with the current mapping_set_error/filemap_check_error infrastructure.
> This will at least prevent you from getting a false report of success.
>
> The new behavior is still consistent with the POSIX spec, and is more
> reliable for application developers. This patch just adds some basic
> infrastructure for doing this. Later patches will change the existing
> code to use this new infrastructure.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
Just one nit below. Otherwise the patch looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 954d510b765a..d6138b6411ff 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -168,6 +168,7 @@ struct file *alloc_file(const struct path *path, fmode_t mode,
> file->f_path = *path;
> file->f_inode = path->dentry->d_inode;
> file->f_mapping = path->dentry->d_inode->i_mapping;
> + file->f_wb_err = filemap_sample_wb_error(file->f_mapping);
Why do you sample here when you also sample in do_dentry_open()? I didn't
find any alloc_file() callers that would possibly care about writeback
errors...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 14/27] fs: new infrastructure for writeback error handling and reporting
From: Jeff Layton @ 2017-05-10 12:19 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170510114840.GF25137@quack2.suse.cz>
On Wed, 2017-05-10 at 13:48 +0200, Jan Kara wrote:
> On Tue 09-05-17 11:49:17, Jeff Layton wrote:
> > Most filesystems currently use mapping_set_error and
> > filemap_check_errors for setting and reporting/clearing writeback errors
> > at the mapping level. filemap_check_errors is indirectly called from
> > most of the filemap_fdatawait_* functions and from
> > filemap_write_and_wait*. These functions are called from all sorts of
> > contexts to wait on writeback to finish -- e.g. mostly in fsync, but
> > also in truncate calls, getattr, etc.
> >
> > The non-fsync callers are problematic. We should be reporting writeback
> > errors during fsync, but many places spread over the tree clear out
> > errors before they can be properly reported, or report errors at
> > nonsensical times.
> >
> > If I get -EIO on a stat() call, there is no reason for me to assume that
> > it is because some previous writeback failed. The fact that it also
> > clears out the error such that a subsequent fsync returns 0 is a bug,
> > and a nasty one since that's potentially silent data corruption.
> >
> > This patch adds a small bit of new infrastructure for setting and
> > reporting errors during address_space writeback. While the above was my
> > original impetus for adding this, I think it's also the case that
> > current fsync semantics are just problematic for userland. Most
> > applications that call fsync do so to ensure that the data they wrote
> > has hit the backing store.
> >
> > In the case where there are multiple writers to the file at the same
> > time, this is really hard to determine. The first one to call fsync will
> > see any stored error, and the rest get back 0. The processes with open
> > fds may not be associated with one another in any way. They could even
> > be in different containers, so ensuring coordination between all fsync
> > callers is not really an option.
> >
> > One way to remedy this would be to track what file descriptor was used
> > to dirty the file, but that's rather cumbersome and would likely be
> > slow. However, there is a simpler way to improve the semantics here
> > without incurring too much overhead.
> >
> > This set adds an errseq_t to struct address_space, and a corresponding
> > one is added to struct file. Writeback errors are recorded in the
> > mapping's errseq_t, and the one in struct file is used as the "since"
> > value.
> >
> > This changes the semantics of the Linux fsync implementation such that
> > applications can now use it to determine whether there were any
> > writeback errors since fsync(fd) was last called (or since the file was
> > opened in the case of fsync having never been called).
> >
> > Note that those writeback errors may have occurred when writing data
> > that was dirtied via an entirely different fd, but that's the case now
> > with the current mapping_set_error/filemap_check_error infrastructure.
> > This will at least prevent you from getting a false report of success.
> >
> > The new behavior is still consistent with the POSIX spec, and is more
> > reliable for application developers. This patch just adds some basic
> > infrastructure for doing this. Later patches will change the existing
> > code to use this new infrastructure.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>
> Just one nit below. Otherwise the patch looks good to me. You can add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index 954d510b765a..d6138b6411ff 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -168,6 +168,7 @@ struct file *alloc_file(const struct path *path, fmode_t mode,
> > file->f_path = *path;
> > file->f_inode = path->dentry->d_inode;
> > file->f_mapping = path->dentry->d_inode->i_mapping;
> > + file->f_wb_err = filemap_sample_wb_error(file->f_mapping);
>
> Why do you sample here when you also sample in do_dentry_open()? I didn't
> find any alloc_file() callers that would possibly care about writeback
> errors...
>
> Honza
I basically used the setting of f_mapping as a guideline as to where to
sample it for initialization. My thinking was that if f_mapping ever
ended up different then you'd probably also want f_wb_err to be
resampled anyway.
I can drop this hunk if you think we don't need it.
--
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 06/27] fs: check for writeback errors after syncing out buffers in generic_file_fsync
From: Matthew Wilcox @ 2017-05-10 12:48 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170509154930.29524-7-jlayton@redhat.com>
On Tue, May 09, 2017 at 11:49:09AM -0400, Jeff Layton wrote:
> ext2 currently does a test+clear of the AS_EIO flag, which is
> is problematic for some coming changes.
>
> What we really need to do instead is call filemap_check_errors
> in __generic_file_fsync after syncing out the buffers. That
> will be sufficient for this case, and help other callers detect
> these errors properly as well.
>
> With that, we don't need to twiddle it in ext2.
>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm/khugepaged: Add missed tracepoint for collapse_huge_page_swapin
From: Vlastimil Babka @ 2017-05-10 13:09 UTC (permalink / raw)
To: SeongJae Park, akpm, kirill.shutemov; +Cc: hannes, linux-mm, linux-kernel
In-Reply-To: <20170507101813.30187-1-sj38.park@gmail.com>
On 05/07/2017 12:18 PM, SeongJae Park wrote:
> One return case of `__collapse_huge_page_swapin()` does not invoke
> tracepoint while every other return case does. This commit adds a
> tracepoint invocation for the case.
>
> Signed-off-by: SeongJae Park <sj38.park@gmail.com>
Right. But extra points by turning all of the "trace+return false"
instances into some kind of "goto out".
> ---
> mm/khugepaged.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ba40b7f673f4..9aad377c67a8 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -909,8 +909,10 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,
> return false;
> }
> /* check if the pmd is still valid */
> - if (mm_find_pmd(mm, address) != pmd)
> + if (mm_find_pmd(mm, address) != pmd) {
> + trace_mm_collapse_huge_page_swapin(mm, swapped_in, referenced, 0);
> return false;
> + }
> }
> if (ret & VM_FAULT_ERROR) {
> trace_mm_collapse_huge_page_swapin(mm, swapped_in, referenced, 0);
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [patch 1/3] MM: remove unused quiet_vmstat function
From: Rik van Riel @ 2017-05-10 13:36 UTC (permalink / raw)
To: Marcelo Tosatti, linux-kernel, linux-mm; +Cc: Luiz Capitulino, Linux RT Users
In-Reply-To: <20170503184039.737799631@redhat.com>
On Wed, 2017-05-03 at 15:40 -0300, Marcelo Tosatti wrote:
> Remove unused quiet_vmstat function.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Reviewed-by: Rik van Riel <riel@redhat.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [v3 0/9] parallelized "struct page" zeroing
From: Pasha Tatashin @ 2017-05-10 13:42 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-kernel, sparclinux, linux-mm, linuxppc-dev, linux-s390,
borntraeger, heiko.carstens, davem
In-Reply-To: <20170510072419.GC31466@dhcp22.suse.cz>
>
> Well, I didn't object to this particular part. I was mostly concerned
> about
> http://lkml.kernel.org/r/1494003796-748672-4-git-send-email-pasha.tatashin@oracle.com
> and the "zero" argument for other functions. I guess we can do without
> that. I _think_ that we should simply _always_ initialize the page at the
> __init_single_page time rather than during the allocation. That would
> require dropping __GFP_ZERO for non-memblock allocations. Or do you
> think we could regress for single threaded initialization?
>
Hi Michal,
Thats exactly right, I am worried that we will regress when there is no
parallelized initialization of "struct pages" if we force
unconditionally do memset() in __init_single_page(). The overhead of
calling memset() on a smaller chunks (64-bytes) may cause the
regression, this is why I opted only for parallelized case to zero this
metadata. This way, we are guaranteed to see great improvements from
this change without having regressions on platforms and builds that do
not support parallelized initialization of "struct pages".
However, on some chips such as latest SPARCs it is beneficial to have
memset() right inside __init_single_page() even for single threaded
case, because it can act as a prefetch on chips with optimized block
initialized store instructions.
Pasha
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 14/27] fs: new infrastructure for writeback error handling and reporting
From: Jan Kara @ 2017-05-10 13:46 UTC (permalink / raw)
To: Jeff Layton
Cc: Jan Kara, linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4,
linux-cifs, linux-nfs, linux-mm, jfs-discussion, linux-xfs,
cluster-devel, linux-f2fs-devel, v9fs-developer, linux-nilfs,
linux-block, dhowells, akpm, hch, ross.zwisler, mawilcox, jack,
viro, corbet, neilb, clm, tytso, axboe, josef, hubcap, rpeterso,
bo.li.liu
In-Reply-To: <1494418790.2688.7.camel@redhat.com>
On Wed 10-05-17 08:19:50, Jeff Layton wrote:
> On Wed, 2017-05-10 at 13:48 +0200, Jan Kara wrote:
> > On Tue 09-05-17 11:49:17, Jeff Layton wrote:
> > > diff --git a/fs/file_table.c b/fs/file_table.c
> > > index 954d510b765a..d6138b6411ff 100644
> > > --- a/fs/file_table.c
> > > +++ b/fs/file_table.c
> > > @@ -168,6 +168,7 @@ struct file *alloc_file(const struct path *path, fmode_t mode,
> > > file->f_path = *path;
> > > file->f_inode = path->dentry->d_inode;
> > > file->f_mapping = path->dentry->d_inode->i_mapping;
> > > + file->f_wb_err = filemap_sample_wb_error(file->f_mapping);
> >
> > Why do you sample here when you also sample in do_dentry_open()? I didn't
> > find any alloc_file() callers that would possibly care about writeback
> > errors...
> >
> > Honza
>
> I basically used the setting of f_mapping as a guideline as to where to
> sample it for initialization. My thinking was that if f_mapping ever
> ended up different then you'd probably also want f_wb_err to be
> resampled anyway.
OK, makes sense.
> I can drop this hunk if you think we don't need it.
I don't really care. I was just wondering whether I'm missing something...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH -mm -v10 1/3] mm, THP, swap: Delay splitting THP during swap out
From: Johannes Weiner @ 2017-05-10 13:56 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel,
Andrea Arcangeli, Ebru Akagunduz, Michal Hocko, Tejun Heo,
Hugh Dickins, Shaohua Li, Rik van Riel, cgroups
In-Reply-To: <20170501235332.GA4411@bbox>
Hi Michan,
On Tue, May 02, 2017 at 08:53:32AM +0900, Minchan Kim wrote:
> @@ -1144,7 +1144,7 @@ void swap_free(swp_entry_t entry)
> /*
> * Called after dropping swapcache to decrease refcnt to swap entries.
> */
> -void swapcache_free(swp_entry_t entry)
> +void __swapcache_free(swp_entry_t entry)
> {
> struct swap_info_struct *p;
>
> @@ -1156,7 +1156,7 @@ void swapcache_free(swp_entry_t entry)
> }
>
> #ifdef CONFIG_THP_SWAP
> -void swapcache_free_cluster(swp_entry_t entry)
> +void __swapcache_free_cluster(swp_entry_t entry)
> {
> unsigned long offset = swp_offset(entry);
> unsigned long idx = offset / SWAPFILE_CLUSTER;
> @@ -1182,6 +1182,14 @@ void swapcache_free_cluster(swp_entry_t entry)
> }
> #endif /* CONFIG_THP_SWAP */
>
> +void swapcache_free(struct page *page, swp_entry_t entry)
> +{
> + if (!PageTransHuge(page))
> + __swapcache_free(entry);
> + else
> + __swapcache_free_cluster(entry);
> +}
I don't think this is cleaner :/
On your second patch:
> @@ -1125,8 +1125,28 @@ static unsigned long shrink_page_list(struct list_head *page_list,
> !PageSwapCache(page)) {
> if (!(sc->gfp_mask & __GFP_IO))
> goto keep_locked;
> - if (!add_to_swap(page, page_list))
> +swap_retry:
> + /*
> + * Retry after split if we fail to allocate
> + * swap space of a THP.
> + */
> + if (!add_to_swap(page)) {
> + if (!PageTransHuge(page) ||
> + split_huge_page_to_list(page, page_list))
> + goto activate_locked;
> + goto swap_retry;
> + }
This is definitely better.
However, I think it'd be cleaner without the label here:
if (!add_to_swap(page)) {
if (!PageTransHuge(page))
goto activate_locked;
/* Split THP and swap individual base pages */
if (split_huge_page_to_list(page, page_list))
goto activate_locked;
if (!add_to_swap(page))
goto activate_locked;
}
> + /*
> + * Got swap space successfully. But unfortunately,
> + * we don't support a THP page writeout so split it.
> + */
> + if (PageTransHuge(page) &&
> + split_huge_page_to_list(page, page_list)) {
> + delete_from_swap_cache(page);
> goto activate_locked;
> + }
Pulling this out of add_to_swap() is an improvement for sure. Add an
XXX: before that "we don't support THP writes" comment for good
measure :)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 13/27] lib: add errseq_t type and infrastructure for handling it
From: Matthew Wilcox @ 2017-05-10 14:18 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170509154930.29524-14-jlayton@redhat.com>
On Tue, May 09, 2017 at 11:49:16AM -0400, Jeff Layton wrote:
> +++ b/lib/errseq.c
> @@ -0,0 +1,199 @@
> +#include <linux/err.h>
> +#include <linux/bug.h>
> +#include <linux/atomic.h>
> +#include <linux/errseq.h>
> +
> +/*
> + * An errseq_t is a way of recording errors in one place, and allowing any
> + * number of "subscribers" to tell whether it has changed since an arbitrary
> + * time of their choosing.
You use the word "time" in several places in the documentation, but I think
it's clearer to say "sampling point" or "sample", since you're not using jiffies
or nanoseconds. For example, I'd phrase this paragraph this way:
* An errseq_t is a way of recording errors in one place, and allowing any
* number of "subscribers" to tell whether it has changed since they last
* sampled it.
> + * The general idea is for consumers to sample an errseq_t value at a
> + * particular point in time. Later, that value can be used to tell whether any
> + * new errors have occurred since that time.
* The general idea is for consumers to sample an errseq_t value. That
* value can be used to tell whether any new errors have occurred since
* the last time it was sampled.
> +/* The "ones" bit for the counter */
Maybe "The lowest bit of the counter"?
> +/**
> + * errseq_check - has an error occurred since a particular point in time?
"has an error occurred since the last time it was sampled"
> +/**
> + * errseq_check_and_advance - check an errseq_t and advance it to the current value
> + * @eseq: pointer to value being checked reported
"value being checked reported"?
> +int errseq_check_and_advance(errseq_t *eseq, errseq_t *since)
> +{
> + int err = 0;
> + errseq_t old, new;
> +
> + /*
> + * Most callers will want to use the inline wrapper to check this,
> + * so that the common case of no error is handled without needing
> + * to lock.
> + */
> + old = READ_ONCE(*eseq);
> + if (old != *since) {
> + /*
> + * Set the flag and try to swap it into place if it has
> + * changed.
> + *
> + * We don't care about the outcome of the swap here. If the
> + * swap doesn't occur, then it has either been updated by a
> + * writer who is bumping the seq count anyway, or another
> + * reader who is just setting the "seen" flag. Either outcome
> + * is OK, and we can advance "since" and return an error based
> + * on what we have.
> + */
> + new = old | ERRSEQ_SEEN;
> + if (new != old)
> + cmpxchg(eseq, old, new);
> + *since = new;
> + err = -(new & MAX_ERRNO);
> + }
I probably need to read through the patchset some more to understand this.
Naively, surely "since" should be updated to the current value of 'eseq'
if we failed the cmpxchg()?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v4 13/27] lib: add errseq_t type and infrastructure for handling it
From: Jeff Layton @ 2017-05-10 14:56 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-fsdevel, linux-kernel, linux-btrfs, linux-ext4, linux-cifs,
linux-nfs, linux-mm, jfs-discussion, linux-xfs, cluster-devel,
linux-f2fs-devel, v9fs-developer, linux-nilfs, linux-block,
dhowells, akpm, hch, ross.zwisler, mawilcox, jack, viro, corbet,
neilb, clm, tytso, axboe, josef, hubcap, rpeterso, bo.li.liu
In-Reply-To: <20170510141821.GB1590@bombadil.infradead.org>
On Wed, 2017-05-10 at 07:18 -0700, Matthew Wilcox wrote:
> On Tue, May 09, 2017 at 11:49:16AM -0400, Jeff Layton wrote:
> > +++ b/lib/errseq.c
> > @@ -0,0 +1,199 @@
> > +#include <linux/err.h>
> > +#include <linux/bug.h>
> > +#include <linux/atomic.h>
> > +#include <linux/errseq.h>
> > +
> > +/*
> > + * An errseq_t is a way of recording errors in one place, and allowing any
> > + * number of "subscribers" to tell whether it has changed since an arbitrary
> > + * time of their choosing.
>
> You use the word "time" in several places in the documentation, but I think
> it's clearer to say "sampling point" or "sample", since you're not using jiffies
> or nanoseconds. For example, I'd phrase this paragraph this way:
>
> * An errseq_t is a way of recording errors in one place, and allowing any
> * number of "subscribers" to tell whether it has changed since they last
> * sampled it.
>
> > + * The general idea is for consumers to sample an errseq_t value at a
> > + * particular point in time. Later, that value can be used to tell whether any
> > + * new errors have occurred since that time.
>
> * The general idea is for consumers to sample an errseq_t value. That
> * value can be used to tell whether any new errors have occurred since
> * the last time it was sampled.
>
> > +/* The "ones" bit for the counter */
>
> Maybe "The lowest bit of the counter"?
>
> > +/**
> > + * errseq_check - has an error occurred since a particular point in time?
>
> "has an error occurred since the last time it was sampled"
>
> > +/**
> > + * errseq_check_and_advance - check an errseq_t and advance it to the current value
> > + * @eseq: pointer to value being checked reported
>
> "value being checked reported"?
>
Thanks. I'm cleaning up the comments like you suggest.
> > +int errseq_check_and_advance(errseq_t *eseq, errseq_t *since)
> > +{
> > + int err = 0;
> > + errseq_t old, new;
> > +
> > + /*
> > + * Most callers will want to use the inline wrapper to check this,
> > + * so that the common case of no error is handled without needing
> > + * to lock.
> > + */
> > + old = READ_ONCE(*eseq);
> > + if (old != *since) {
> > + /*
> > + * Set the flag and try to swap it into place if it has
> > + * changed.
> > + *
> > + * We don't care about the outcome of the swap here. If the
> > + * swap doesn't occur, then it has either been updated by a
> > + * writer who is bumping the seq count anyway, or another
> > + * reader who is just setting the "seen" flag. Either outcome
> > + * is OK, and we can advance "since" and return an error based
> > + * on what we have.
> > + */
> > + new = old | ERRSEQ_SEEN;
> > + if (new != old)
> > + cmpxchg(eseq, old, new);
> > + *since = new;
> > + err = -(new & MAX_ERRNO);
> > + }
>
> I probably need to read through the patchset some more to understand this.
> Naively, surely "since" should be updated to the current value of 'eseq'
> if we failed the cmpxchg()?
I don't think so. If we want to do that, then we'll need to redrive the
cmpxchg to set the SEEN flag if it's now clear. Storing the value in
"since" is effectively sampling it, so you do have to mark it seen.
The good news is that I think that "new" is just as valid a value to
store here as *eseq would be. It ends up representing an errseq_t value
that never actually got stored in eseq, but that's OK with the way this
works.
--
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [v3 0/9] parallelized "struct page" zeroing
From: Michal Hocko @ 2017-05-10 14:57 UTC (permalink / raw)
To: Pasha Tatashin
Cc: linux-kernel, sparclinux, linux-mm, linuxppc-dev, linux-s390,
borntraeger, heiko.carstens, davem
In-Reply-To: <3f5f1416-aa91-a2ff-cc89-b97fcaa3e4db@oracle.com>
On Wed 10-05-17 09:42:22, Pasha Tatashin wrote:
> >
> >Well, I didn't object to this particular part. I was mostly concerned
> >about
> >http://lkml.kernel.org/r/1494003796-748672-4-git-send-email-pasha.tatashin@oracle.com
> >and the "zero" argument for other functions. I guess we can do without
> >that. I _think_ that we should simply _always_ initialize the page at the
> >__init_single_page time rather than during the allocation. That would
> >require dropping __GFP_ZERO for non-memblock allocations. Or do you
> >think we could regress for single threaded initialization?
> >
>
> Hi Michal,
>
> Thats exactly right, I am worried that we will regress when there is no
> parallelized initialization of "struct pages" if we force unconditionally do
> memset() in __init_single_page(). The overhead of calling memset() on a
> smaller chunks (64-bytes) may cause the regression, this is why I opted only
> for parallelized case to zero this metadata. This way, we are guaranteed to
> see great improvements from this change without having regressions on
> platforms and builds that do not support parallelized initialization of
> "struct pages".
Have you measured that? I do not think it would be super hard to
measure. I would be quite surprised if this added much if anything at
all as the whole struct page should be in the cache line already. We do
set reference count and other struct members. Almost nobody should be
looking at our page at this time and stealing the cache line. On the
other hand a large memcpy will basically wipe everything away from the
cpu cache. Or am I missing something?
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [v3 0/9] parallelized "struct page" zeroing
From: Pasha Tatashin @ 2017-05-10 15:01 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-kernel, sparclinux, linux-mm, linuxppc-dev, linux-s390,
borntraeger, heiko.carstens, davem
In-Reply-To: <20170510145726.GM31466@dhcp22.suse.cz>
On 05/10/2017 10:57 AM, Michal Hocko wrote:
> On Wed 10-05-17 09:42:22, Pasha Tatashin wrote:
>>>
>>> Well, I didn't object to this particular part. I was mostly concerned
>>> about
>>> http://lkml.kernel.org/r/1494003796-748672-4-git-send-email-pasha.tatashin@oracle.com
>>> and the "zero" argument for other functions. I guess we can do without
>>> that. I _think_ that we should simply _always_ initialize the page at the
>>> __init_single_page time rather than during the allocation. That would
>>> require dropping __GFP_ZERO for non-memblock allocations. Or do you
>>> think we could regress for single threaded initialization?
>>>
>>
>> Hi Michal,
>>
>> Thats exactly right, I am worried that we will regress when there is no
>> parallelized initialization of "struct pages" if we force unconditionally do
>> memset() in __init_single_page(). The overhead of calling memset() on a
>> smaller chunks (64-bytes) may cause the regression, this is why I opted only
>> for parallelized case to zero this metadata. This way, we are guaranteed to
>> see great improvements from this change without having regressions on
>> platforms and builds that do not support parallelized initialization of
>> "struct pages".
>
> Have you measured that? I do not think it would be super hard to
> measure. I would be quite surprised if this added much if anything at
> all as the whole struct page should be in the cache line already. We do
> set reference count and other struct members. Almost nobody should be
> looking at our page at this time and stealing the cache line. On the
> other hand a large memcpy will basically wipe everything away from the
> cpu cache. Or am I missing something?
>
Perhaps you are right, and I will measure on x86. But, I suspect hit can
become unacceptable on some platfoms: there is an overhead of calling a
function, even if it is leaf-optimized, and there is an overhead in
memset() to check for alignments of size and address, types of setting
(zeroing vs. non-zeroing), etc., that adds up quickly.
Pasha
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [v3 0/9] parallelized "struct page" zeroing
From: David Miller @ 2017-05-10 15:19 UTC (permalink / raw)
To: mhocko
Cc: pasha.tatashin, linux-kernel, sparclinux, linux-mm, linuxppc-dev,
linux-s390, borntraeger, heiko.carstens
In-Reply-To: <20170510145726.GM31466@dhcp22.suse.cz>
From: Michal Hocko <mhocko@kernel.org>
Date: Wed, 10 May 2017 16:57:26 +0200
> Have you measured that? I do not think it would be super hard to
> measure. I would be quite surprised if this added much if anything at
> all as the whole struct page should be in the cache line already. We do
> set reference count and other struct members. Almost nobody should be
> looking at our page at this time and stealing the cache line. On the
> other hand a large memcpy will basically wipe everything away from the
> cpu cache. Or am I missing something?
I guess it might be clearer if you understand what the block
initializing stores do on sparc64. There are no memory accesses at
all.
The cpu just zeros out the cache line, that's it.
No L3 cache line is allocated. So this "wipe everything" behavior
will not happen in the L3.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [v3 0/9] parallelized "struct page" zeroing
From: David Miller @ 2017-05-10 15:20 UTC (permalink / raw)
To: pasha.tatashin
Cc: mhocko, linux-kernel, sparclinux, linux-mm, linuxppc-dev,
linux-s390, borntraeger, heiko.carstens
In-Reply-To: <ab667486-54a0-a36e-6797-b5f7b83c10f7@oracle.com>
From: Pasha Tatashin <pasha.tatashin@oracle.com>
Date: Wed, 10 May 2017 11:01:40 -0400
> Perhaps you are right, and I will measure on x86. But, I suspect hit
> can become unacceptable on some platfoms: there is an overhead of
> calling a function, even if it is leaf-optimized, and there is an
> overhead in memset() to check for alignments of size and address,
> types of setting (zeroing vs. non-zeroing), etc., that adds up
> quickly.
Another source of overhead on the sparc64 side is that we much
do memory barriers around the block initializiing stores. So
batching calls to memset() amortize that as well.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: RFC v2: post-init-read-only protection for data allocated dynamically
From: Igor Stoppa @ 2017-05-10 15:19 UTC (permalink / raw)
To: Michal Hocko, Dave Hansen, Laura Abbott
Cc: linux-mm, linux-kernel, kernel-hardening@lists.openwall.com
In-Reply-To: <20170510114319.GK31466@dhcp22.suse.cz>
On 10/05/17 14:43, Michal Hocko wrote:
> On Wed 10-05-17 11:57:42, Igor Stoppa wrote:
>> On 10/05/17 11:05, Michal Hocko wrote:
> [...]
>>> To me it seems that this being an initialization mostly thingy a simple
>>> allocator which manages a pool of pages (one set of sealed and one for
>>> allocations)
>>
>> Shouldn't also the set of pages used for keeping track of the others be
>> sealed? Once one is ro, also the other should not change.
>
> Heh, that really depends how much consistency and robustness you want to
> achieve. It is really hard to defend against targeted attacks against
> the allocator metadata when a code is running in the kernel.
Taking the trouble to implement the sealing, then anything that doesn't
have a justification for staying R/W is fair game for sealing, IMHO.
>>> and which only appends new objects as they fit to unsealed
>>> pages would be sufficient for starter.
>>
>> Any "free" that might happen during the initialization transient, would
>> actually result in an untracked gap, right?
>
> yes. And once the whole page is free it would get unsealed and returned
> to the (page) allocator.
Which means that there must be some way to track the freeing.
I intentionally omitted it, because I wasn't sure it would still be
compatible with the idea of a simple linear allocator.
> This approach would inevitably lead to internal
> fragmentation but reducing that would require a pool which is shared for
> objects with the common life cycle which is quite hard with requirements
> you have (you would have to convey the allocation context to all users
> somehow).
What if the users were unaware of most of the context and would only use
some flag, say GFP_SEAL?
Shouldn't the allocator be the only one aware of the context?
Context being the actual set of pages used.
Other idea: for each logical group of objects having same lifecycle,
define a pool, then do linear allocation within the pool for the
respective logical group.
Still some way would be needed to track the utilization of each page,
but it would ensure that when a logical group is discarded, all its
related pages are freed.
>> What about the size of the pool of pages?
>
> I wouldn't see that as a big deal. New pages would be allocated as
> needed.
ok
[...]
>> - modules: unloading and reloading modules will eventually lead to
>> permanently lost pages, in increasing number.
>
> Each module should free all objects that were allocated on its behalf
> and that should result in pages being freed as well
Only if the objects are enforced to be contiguous and the start is at
the beginning of a page, which seems to go in the direction of having a
memory pool for each module.
>> Loading/unloading repeatedly the same module is probably not so common,
>> with a major exception being USB, where almost anything can show up.
>> And disappear.
>> This seems like a major showstopper for the linear allocator you propose.
>
> I am not sure I understand. If such a module kept allocations behind it
> would be a memory leak no matter what.
What I had in mind is that, with a global linear allocator _without_
support for returning "freed" pages, there would be a memory consumption
progressively increasing.
But even if the module frees correctly its allocations and they are
tracked correctly, it's still possible that some page doesn't get
returned, unless the module had started using data from the beginning of
a brand new page and nothing else but that module used it.
So it really looks like we are discussing a per-module (linear) allocator.
Probably that's what you meant all the time and I just realized it now ...
>> My reasoning in pursuing the kmalloc approach was that it is already
>> equipped with mechanisms for dealing with these sort of cases, where
>> memory can be fragmented.
>
> Yeah, but kmalloc is optimized for a completely different usecase. You
> can reuse same pages again and again while you clearly cannot do the
> same once you seal a page and make it read only.
No, but during the allocation transient, I could.
Cons: less protection for what is already in the page.
Pros: tighter packing.
> Well unless you want to
> open time windows when the page stops being RO or use a different
> mapping for the allocator.
Yes, I was proposing to temporarily make the specific page RW.
> But try to consider how many features of the slab allocator you are
> actually going to need wrt. to tweaks it would have to implement to
> support this new use case. Maybe duplicating general purpose caches and
> creating specialized explicitly is a viable path. I haven't tried
> it.
>
>> I also wouldn't risk introducing bugs with my homebrew allocator ...
>>
>> The initial thought was that there could be a master toggle to
>> seal/unseal all the memory affected.
>>
>> But you were not too excited, iirc :-D
>
> yes, If there are different users a pool (kmem_cache like) would be more
> natural.
>
>> Alternatively, kmalloc could be enhanced to unseal only the pages it
>> wants to modify.
>
> You would have to stop the world to prevent from an accidental overwrite
> during that time. Which makes the whole thing quite dubious IMHO.
>
>> I don't think much can be done for data that is placed together, in the
>> same page with something that needs to be altered.
>> But what is outside of that page could still enjoy the protection from
>> the seal.
Recap
-----
The latest proposal would be (I can create a new version of the RFC if
preferred):
Have a per-module linear memory allocator, using on on-demand new pages,
with some way to track:
* pages used in each pool (ex: a next ptr in each page)
* free space at the end of the page
the allocation would be aligned accordingly to arch requirements
implemented, for example, with a counter
Pages are sealed as soon as they fill up and a next one is allocated.
Or they are explicitly sealed by their respective module.
In the typical case the freeing would happen on the entire pool, for
example when the module is unloaded.
It might be even nicer to have a master teardown call, for the whole pool.
There is still the problem of how to deal with large physical pages and
kmalloc.
Having a per-module pool of pages is likely to generate even more waste,
if the pages are particularly large.
So I'd like to play a little what-if scenario:
what if I was to support exclusively virtual memory and convert to it
everything that might need sealing?
I cannot find any reason why this could not be done, even if the
original code uses kmalloc.
Extension
---------
What I discussed so far is about things that are not expected to change.
At most they would be freed, as units.
However, if some other data exhibits quasi-or-characteristics, it could
be protected as well.
With the understanding that there would be holes in the memory
allocation and a linear allocator probably would not be enough anymore.
This could be achieved by keeping a bitmaps of machine aligned words.
Ex: a 4k page with 8bytes words would need 64 bytes.
--
igor
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox