Linux userland API discussions
 help / color / mirror / Atom feed
* Re: [PATCH v2] prctl: add PR_[GS]ET_KILLABLE
From: Jürg Billeter @ 2018-07-31 16:12 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Thomas Gleixner, Eric Biederman, linux-api,
	linux-kernel
In-Reply-To: <20180731143949.GA1890@redhat.com>

On Tue, 2018-07-31 at 16:39 +0200, Oleg Nesterov wrote:
> On 07/31, Jürg Billeter wrote:
> > SIGINT, SIGQUIT and SIGTSTP are used in job control for ^C, ^\, ^Z.
> > While a task with the SIGNAL_UNKILLABLE flag could install handlers for
> > these signals, this is not sufficient to implement a shell that uses
> > CLONE_NEWPID for child processes:
> 
> Ah. My question wasn't clear, sorry.
> 
> Could you explain your use-case? Why a shell wants to use
> CLONE_NEWPID?

To guarantee that there won't be any runaway processes, i.e., ensure
that no descendants (background helper daemons or misbehaving
processes) survive when the child process is terminated. And to prevent
children from killing their ancestors. This is not something that can
be always-on in all shells, but it could be an option for users that
want this control/isolation.

> And what do we actually want in, say, ^Z case? Just stop the child reaper
> or may be it would be better to stop the whole pid namespace?

Stopping the whole PID namespace would be interesting, however, I think
this should be discussed separately if and when there is a proposal to
support this. For now the process group is stopped, same as without PID
namespaces.

> >  * As SIGSTOP is ignored when raised from the SIGNAL_UNKILLABLE process
> >    itself, it's not possible to implement the stop action in a custom
> >    SIGTSTP handler.
> 
> Yes. So may be we actually want to change __isig() paths to use
> SEND_SIG_FORCED (this is not that simple), or perhaps we can change
> __send_signal() to not drop SIGSTOP sent to itself, or may be we can even
> introduce SIG_DFL_EVEN_IF_INIT, I dunno.

In my opinion, my patch is much simpler and also more general as it
covers all scenarios where regular signal handling is required or
desired for "init" processes, with minimal code changes (after
PR_SET_KILLABLE, binaries that expect SIG_DFL to work can be executed
without changes).

> >  * Many applications do not install handlers for these signals and
> >    thus, job control won't work properly with unmodified applications.
> 
> I can't understand this. An application should be changed anyway to do
> PR_SET_KILLABLE?

PR_SET_KILLABLE can be called (e.g., by the shell) between clone() and
execve(). (Some applications may have issues running as subreaper but
that's a separate matter, signal handling will work as expected).

> > +	case PR_SET_KILLABLE:
> > +		if (arg2 != 1 || arg3 || arg4 || arg5)
> > +			return -EINVAL;
> > +		spin_lock_irq(&me->sighand->siglock);
> > +		me->signal->flags &= ~SIGNAL_UNKILLABLE;
> > +		spin_unlock_irq(&me->sighand->siglock);
> 
> OK, but then you need to change the CLONE_PARENT/SIGNAL_UNKILLABLE check
> in copy_process().

Good point, need a different check for the PID namespace root process
in copy_process().

Thanks,
Jürg

^ permalink raw reply

* Re: [PATCH v2] prctl: add PR_[GS]ET_KILLABLE
From: Oleg Nesterov @ 2018-07-31 14:39 UTC (permalink / raw)
  To: Jürg Billeter
  Cc: Andrew Morton, Thomas Gleixner, Eric Biederman, linux-api,
	linux-kernel
In-Reply-To: <20180731070337.61004-1-j@bitron.ch>

On 07/31, Jürg Billeter wrote:
>
> PR_SET_KILLABLE clears the SIGNAL_UNKILLABLE flag. This allows
> CLONE_NEWPID tasks to restore normal signal behavior, opting out of the
> special signal protection for init processes. This prctl does not allow
> setting the SIGNAL_UNKILLABLE flag, only clearing.
>
> The SIGNAL_UNKILLABLE flag, which is implicitly set for tasks cloned
> with CLONE_NEWPID, has the effect of ignoring all signals (from
> userspace) if the corresponding handler is set to SIG_DFL. The only
> exceptions are SIGKILL and SIGSTOP and they are only accepted if raised
> from an ancestor namespace.
>
> SIGINT, SIGQUIT and SIGTSTP are used in job control for ^C, ^\, ^Z.
> While a task with the SIGNAL_UNKILLABLE flag could install handlers for
> these signals, this is not sufficient to implement a shell that uses
> CLONE_NEWPID for child processes:

Ah. My question wasn't clear, sorry.

Could you explain your use-case? Why a shell wants to use CLONE_NEWPID?
And what do we actually want in, say, ^Z case? Just stop the child reaper
or may be it would be better to stop the whole pid namespace?

>  * As SIGSTOP is ignored when raised from the SIGNAL_UNKILLABLE process
>    itself, it's not possible to implement the stop action in a custom
>    SIGTSTP handler.

Yes. So may be we actually want to change __isig() paths to use
SEND_SIG_FORCED (this is not that simple), or perhaps we can change
__send_signal() to not drop SIGSTOP sent to itself, or may be we can even
introduce SIG_DFL_EVEN_IF_INIT, I dunno.

>  * Many applications do not install handlers for these signals and
>    thus, job control won't work properly with unmodified applications.

I can't understand this. An application should be changed anyway to do
PR_SET_KILLABLE?


Let me clarify. I am not arguing with this patch, probably it makes sense in
any case. I am just trying to understand your real motivation for this change.



> +	case PR_SET_KILLABLE:
> +		if (arg2 != 1 || arg3 || arg4 || arg5)
> +			return -EINVAL;
> +		spin_lock_irq(&me->sighand->siglock);
> +		me->signal->flags &= ~SIGNAL_UNKILLABLE;
> +		spin_unlock_irq(&me->sighand->siglock);

OK, but then you need to change the CLONE_PARENT/SIGNAL_UNKILLABLE check
in copy_process().

Oleg.

^ permalink raw reply

* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: David Howells @ 2018-07-31 13:20 UTC (permalink / raw)
  To: Jann Horn
  Cc: dhowells, Al Viro, Linux API, Linus Torvalds, linux-fsdevel,
	kernel list
In-Reply-To: <CAG48ez0SdSPkimsk-MW7oSQO-Y64X-LD4JXnuPsZXKuz=y1_Kg@mail.gmail.com>

Jann Horn <jannh@google.com> wrote:

> > +       strcpy(p->f_fs_name, dentry->d_sb->s_type->name);
> 
> Can you use strlcpy() instead? From a quick look, I don't see anything
> that actually limits the size of filesystem names, even though
> everything in-kernel probably fits into the 16 bytes you've allocated
> for the name.

Sure.  Should I increase the field size to 32, I wonder?

David

^ permalink raw reply

* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: David Howells @ 2018-07-31 12:39 UTC (permalink / raw)
  To: Al Viro; +Cc: dhowells, linux-api, torvalds, linux-fsdevel, linux-kernel
In-Reply-To: <20180731041642.GH30522@ZenIV.linux.org.uk>

Al Viro <viro@ZenIV.linux.org.uk> wrote:

> Umm...  What's so special about cell/volume/domain/realm?

Nothing particularly.  But they're something various network filesystems might
find useful.  cell for AFS, domain for CIFS, realm for things that use
kerberos.

volume_id/uuid/name would be usable by ext4 too, for example.

> And what do we do when a random filesystem gets added - should its
> parameters go into catch-all pile (attr_parameter),

FSINFO_ATTR_PARAMETER is a way to enumerate the configuration parameters
passed to mount, as an alternative to parsing /proc/mounts.  So, for example,
afs has:

	enum afs_param {
		Opt_autocell,
		Opt_dyn,
		Opt_source,
		nr__afs_params
	};

	static const struct fs_parameter_spec afs_param_specs[nr__afs_params] = {
		[Opt_autocell]	= { fs_param_takes_no_value },
		[Opt_dyn]	= { fs_param_takes_no_value },
		[Opt_source]	= { fs_param_is_string },
	};

	static const struct constant_table afs_param_keys[] = {
		{ "autocell",	Opt_autocell },
		{ "dyn",	Opt_dyn },
		{ "source",	Opt_source },
	};

My thought is that calling fsinfo(..., "/some/afs/file", &params, ...) with:

	struct fsinfo_params params = {
		.request = FSINFO_ATTR_PARAMETER,
		.Nth	 = <parameter-number>,
	};

would get you back, for example:

	Nth	Result
	=======	==========================================
	0	"autocell" (or "" if not set)
	1	"dyn" (or "" if not set)
	2	"source=%#grand.central.org:root.cell."
	3+	-ENODATA (ie. there are no more)

where Nth corresponds to the parameter specified by
FSINFO_ATTR_PARAM_DESCRIPTION and Nth.

Now for some filesystems, cgroups-v1 for example, there are parameters beyond
the list (the subsystem name) and these can be listed after the predefined
parameters, eg.:

	Nth	Result
	=======	==========================================
	0	"all" or ""
	1	"clone_children" or ""
	2	"cpuset_v2_mode" or ""
	3	"name" or ""
	4	"none" or ""
	5	"noprefix" or ""
	6	"release_agent" or ""
	7	"xattr" or ""
	8	"<subsys0>" or ""
	9	"<subsys1>" or ""
	10	"<subsys2>" or ""
	...	-ENODATA

> or should they get classes of their own?

Yes.

> For Cthulhu sake, who's going to maintain that enum in face of
> random out-of-tree filesystems, each wanting a class or two its own?

They don't get their own numbers unless they're in-tree.  Full stop.  We have
the same issue with system calls and not-yet-upstream new syscalls.

Note that, as I have the code now, the "type" of return value for each
attribute must also be declared to the fsinfo() core, and the fsinfo core does
the copy to/from userspace.

> We'd tried that with device numbers; ask hpa how well has that
> worked and how much did he love the whole experience...

What would you do instead?  I would prefer to avoid using text strings as keys
because then I need a big lookup table, and possibly this gets devolved to
each filesystem to handle - which ends up even more of a mess because then
there's nothing to hold consistency.

David

^ permalink raw reply

* [PATCH v4 6/6] mm, slab: shorten kmalloc cache names for large sizes
From: Vlastimil Babka @ 2018-07-31  9:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, linux-api, Roman Gushchin, Michal Hocko,
	Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox, Vlastimil Babka
In-Reply-To: <20180731090649.16028-1-vbabka@suse.cz>

Kmalloc cache names can get quite long for large object sizes, when the sizes
are expressed in bytes. Use 'k' and 'M' prefixes to make the names as short
as possible e.g. in /proc/slabinfo. This works, as we mostly use power-of-two
sizes, with exceptions only below 1k.

Example: 'kmalloc-4194304' becomes 'kmalloc-4M'

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Christoph Lameter <cl@linux.com>
Acked-by: Roman Gushchin <guro@fb.com>
---
 mm/slab_common.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 03f40b273ea3..a07fcb2551f6 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1050,15 +1050,15 @@ const struct kmalloc_info_struct kmalloc_info[] __initconst = {
 	{"kmalloc-16",             16},		{"kmalloc-32",             32},
 	{"kmalloc-64",             64},		{"kmalloc-128",           128},
 	{"kmalloc-256",           256},		{"kmalloc-512",           512},
-	{"kmalloc-1024",         1024},		{"kmalloc-2048",         2048},
-	{"kmalloc-4096",         4096},		{"kmalloc-8192",         8192},
-	{"kmalloc-16384",       16384},		{"kmalloc-32768",       32768},
-	{"kmalloc-65536",       65536},		{"kmalloc-131072",     131072},
-	{"kmalloc-262144",     262144},		{"kmalloc-524288",     524288},
-	{"kmalloc-1048576",   1048576},		{"kmalloc-2097152",   2097152},
-	{"kmalloc-4194304",   4194304},		{"kmalloc-8388608",   8388608},
-	{"kmalloc-16777216", 16777216},		{"kmalloc-33554432", 33554432},
-	{"kmalloc-67108864", 67108864}
+	{"kmalloc-1k",           1024},		{"kmalloc-2k",           2048},
+	{"kmalloc-4k",           4096},		{"kmalloc-8k",           8192},
+	{"kmalloc-16k",         16384},		{"kmalloc-32k",         32768},
+	{"kmalloc-64k",         65536},		{"kmalloc-128k",       131072},
+	{"kmalloc-256k",       262144},		{"kmalloc-512k",       524288},
+	{"kmalloc-1M",        1048576},		{"kmalloc-2M",        2097152},
+	{"kmalloc-4M",        4194304},		{"kmalloc-8M",        8388608},
+	{"kmalloc-16M",      16777216},		{"kmalloc-32M",      33554432},
+	{"kmalloc-64M",      67108864}
 };
 
 /*
@@ -1108,6 +1108,21 @@ void __init setup_kmalloc_cache_index_table(void)
 	}
 }
 
+static const char *
+kmalloc_cache_name(const char *prefix, unsigned int size)
+{
+
+	static const char units[3] = "\0kM";
+	int idx = 0;
+
+	while (size >= 1024 && (size % 1024 == 0)) {
+		size /= 1024;
+		idx++;
+	}
+
+	return kasprintf(GFP_NOWAIT, "%s-%u%c", prefix, size, units[idx]);
+}
+
 static void __init
 new_kmalloc_cache(int idx, int type, slab_flags_t flags)
 {
@@ -1115,7 +1130,7 @@ new_kmalloc_cache(int idx, int type, slab_flags_t flags)
 
 	if (type == KMALLOC_RECLAIM) {
 		flags |= SLAB_RECLAIM_ACCOUNT;
-		name = kasprintf(GFP_NOWAIT, "kmalloc-rcl-%u",
+		name = kmalloc_cache_name("kmalloc-rcl",
 						kmalloc_info[idx].size);
 		BUG_ON(!name);
 	} else {
@@ -1164,8 +1179,7 @@ void __init create_kmalloc_caches(slab_flags_t flags)
 
 		if (s) {
 			unsigned int size = kmalloc_size(i);
-			char *n = kasprintf(GFP_NOWAIT,
-				 "dma-kmalloc-%u", size);
+			const char *n = kmalloc_cache_name("dma-kmalloc", size);
 
 			BUG_ON(!n);
 			kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache(
-- 
2.18.0

^ permalink raw reply related

* [PATCH v4 5/6] mm, proc: add KReclaimable to /proc/meminfo
From: Vlastimil Babka @ 2018-07-31  9:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, linux-api, Roman Gushchin, Michal Hocko,
	Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox, Vlastimil Babka
In-Reply-To: <20180731090649.16028-1-vbabka@suse.cz>

The vmstat NR_KERNEL_MISC_RECLAIMABLE counter is for kernel non-slab
allocations that can be reclaimed via shrinker. In /proc/meminfo, we can show
the sum of all reclaimable kernel allocations (including slab) as
"KReclaimable". Add the same counter also to per-node meminfo under /sys

With this counter, users will have more complete information about
kernel memory usage. Non-slab reclaimable pages (currently just the ION
allocator) will not be missing from /proc/meminfo, making users wonder
where part of their memory went. More precisely, they already appear in
MemAvailable, but without the new counter, it's not obvious why the
value in MemAvailable doesn't fully correspond with the sum of other
counters participating in it.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Roman Gushchin <guro@fb.com>
---
 Documentation/filesystems/proc.txt |  4 ++++
 drivers/base/node.c                | 19 ++++++++++++-------
 fs/proc/meminfo.c                  | 16 ++++++++--------
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 520f6a84cf50..6a255f960ab5 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -858,6 +858,7 @@ Writeback:           0 kB
 AnonPages:      861800 kB
 Mapped:         280372 kB
 Shmem:             644 kB
+KReclaimable:   168048 kB
 Slab:           284364 kB
 SReclaimable:   159856 kB
 SUnreclaim:     124508 kB
@@ -921,6 +922,9 @@ AnonHugePages: Non-file backed huge pages mapped into userspace page tables
 ShmemHugePages: Memory used by shared memory (shmem) and tmpfs allocated
               with huge pages
 ShmemPmdMapped: Shared memory mapped into userspace with huge pages
+KReclaimable: Kernel allocations that the kernel will attempt to reclaim
+              under memory pressure. Includes SReclaimable (below), and other
+              direct allocations with a shrinker.
         Slab: in-kernel data structures cache
 SReclaimable: Part of Slab, that might be reclaimed, such as caches
   SUnreclaim: Part of Slab, that cannot be reclaimed on memory pressure
diff --git a/drivers/base/node.c b/drivers/base/node.c
index a5e821d09656..81cef8031eae 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -67,8 +67,11 @@ static ssize_t node_read_meminfo(struct device *dev,
 	int nid = dev->id;
 	struct pglist_data *pgdat = NODE_DATA(nid);
 	struct sysinfo i;
+	unsigned long sreclaimable, sunreclaimable;
 
 	si_meminfo_node(&i, nid);
+	sreclaimable = node_page_state(pgdat, NR_SLAB_RECLAIMABLE);
+	sunreclaimable = node_page_state(pgdat, NR_SLAB_UNRECLAIMABLE);
 	n = sprintf(buf,
 		       "Node %d MemTotal:       %8lu kB\n"
 		       "Node %d MemFree:        %8lu kB\n"
@@ -118,6 +121,7 @@ static ssize_t node_read_meminfo(struct device *dev,
 		       "Node %d NFS_Unstable:   %8lu kB\n"
 		       "Node %d Bounce:         %8lu kB\n"
 		       "Node %d WritebackTmp:   %8lu kB\n"
+		       "Node %d KReclaimable:   %8lu kB\n"
 		       "Node %d Slab:           %8lu kB\n"
 		       "Node %d SReclaimable:   %8lu kB\n"
 		       "Node %d SUnreclaim:     %8lu kB\n"
@@ -138,20 +142,21 @@ static ssize_t node_read_meminfo(struct device *dev,
 		       nid, K(node_page_state(pgdat, NR_UNSTABLE_NFS)),
 		       nid, K(sum_zone_node_page_state(nid, NR_BOUNCE)),
 		       nid, K(node_page_state(pgdat, NR_WRITEBACK_TEMP)),
-		       nid, K(node_page_state(pgdat, NR_SLAB_RECLAIMABLE) +
-			      node_page_state(pgdat, NR_SLAB_UNRECLAIMABLE)),
-		       nid, K(node_page_state(pgdat, NR_SLAB_RECLAIMABLE)),
+		       nid, K(sreclaimable +
+			      node_page_state(pgdat, NR_KERNEL_MISC_RECLAIMABLE)),
+		       nid, K(sreclaimable + sunreclaimable),
+		       nid, K(sreclaimable),
+		       nid, K(sunreclaimable)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-		       nid, K(node_page_state(pgdat, NR_SLAB_UNRECLAIMABLE)),
+		       ,
 		       nid, K(node_page_state(pgdat, NR_ANON_THPS) *
 				       HPAGE_PMD_NR),
 		       nid, K(node_page_state(pgdat, NR_SHMEM_THPS) *
 				       HPAGE_PMD_NR),
 		       nid, K(node_page_state(pgdat, NR_SHMEM_PMDMAPPED) *
-				       HPAGE_PMD_NR));
-#else
-		       nid, K(node_page_state(pgdat, NR_SLAB_UNRECLAIMABLE)));
+				       HPAGE_PMD_NR)
 #endif
+		       );
 	n += hugetlb_report_node_meminfo(nid, buf + n);
 	return n;
 }
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index 2fb04846ed11..61a18477bc07 100644
--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -37,6 +37,7 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 	long cached;
 	long available;
 	unsigned long pages[NR_LRU_LISTS];
+	unsigned long sreclaimable, sunreclaim;
 	int lru;
 
 	si_meminfo(&i);
@@ -52,6 +53,8 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 		pages[lru] = global_node_page_state(NR_LRU_BASE + lru);
 
 	available = si_mem_available();
+	sreclaimable = global_node_page_state(NR_SLAB_RECLAIMABLE);
+	sunreclaim = global_node_page_state(NR_SLAB_UNRECLAIMABLE);
 
 	show_val_kb(m, "MemTotal:       ", i.totalram);
 	show_val_kb(m, "MemFree:        ", i.freeram);
@@ -93,14 +96,11 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 	show_val_kb(m, "Mapped:         ",
 		    global_node_page_state(NR_FILE_MAPPED));
 	show_val_kb(m, "Shmem:          ", i.sharedram);
-	show_val_kb(m, "Slab:           ",
-		    global_node_page_state(NR_SLAB_RECLAIMABLE) +
-		    global_node_page_state(NR_SLAB_UNRECLAIMABLE));
-
-	show_val_kb(m, "SReclaimable:   ",
-		    global_node_page_state(NR_SLAB_RECLAIMABLE));
-	show_val_kb(m, "SUnreclaim:     ",
-		    global_node_page_state(NR_SLAB_UNRECLAIMABLE));
+	show_val_kb(m, "KReclaimable:   ", sreclaimable +
+		    global_node_page_state(NR_KERNEL_MISC_RECLAIMABLE));
+	show_val_kb(m, "Slab:           ", sreclaimable + sunreclaim);
+	show_val_kb(m, "SReclaimable:   ", sreclaimable);
+	show_val_kb(m, "SUnreclaim:     ", sunreclaim);
 	seq_printf(m, "KernelStack:    %8lu kB\n",
 		   global_zone_page_state(NR_KERNEL_STACK_KB));
 	show_val_kb(m, "PageTables:     ",
-- 
2.18.0

^ permalink raw reply related

* [PATCH v4 4/6] mm: rename and change semantics of nr_indirectly_reclaimable_bytes
From: Vlastimil Babka @ 2018-07-31  9:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, linux-api, Roman Gushchin, Michal Hocko,
	Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox, Vlastimil Babka, Vijayanand Jitta,
	Laura Abbott, Sumit Semwal
In-Reply-To: <20180731090649.16028-1-vbabka@suse.cz>

The vmstat counter NR_INDIRECTLY_RECLAIMABLE_BYTES was introduced by commit
eb59254608bc ("mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES") with the goal of
accounting objects that can be reclaimed, but cannot be allocated via a
SLAB_RECLAIM_ACCOUNT cache. This is now possible via kmalloc() with
__GFP_RECLAIMABLE flag, and the dcache external names user is converted.

The counter is however still useful for accounting direct page allocations
(i.e. not slab) with a shrinker, such as the ION page pool. So keep it, and:

- change granularity to pages to be more like other counters; sub-page
  allocations should be able to use kmalloc
- rename the counter to NR_KERNEL_MISC_RECLAIMABLE
- expose the counter again in vmstat as "nr_kernel_misc_reclaimable"; we can
  again remove the check for not printing "hidden" counters

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Vijayanand Jitta <vjitta@codeaurora.org>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Acked-by: Christoph Lameter <cl@linux.com>
Acked-by: Roman Gushchin <guro@fb.com>
---
 drivers/staging/android/ion/ion_page_pool.c |  8 ++++----
 include/linux/mmzone.h                      |  2 +-
 mm/page_alloc.c                             | 19 +++++++------------
 mm/util.c                                   |  3 +--
 mm/vmstat.c                                 |  6 +-----
 5 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index 9bc56eb48d2a..0d2a95957ee8 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -33,8 +33,8 @@ static void ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
 		pool->low_count++;
 	}
 
-	mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
-			    (1 << (PAGE_SHIFT + pool->order)));
+	mod_node_page_state(page_pgdat(page), NR_KERNEL_MISC_RECLAIMABLE,
+							1 << pool->order);
 	mutex_unlock(&pool->mutex);
 }
 
@@ -53,8 +53,8 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high)
 	}
 
 	list_del(&page->lru);
-	mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
-			    -(1 << (PAGE_SHIFT + pool->order)));
+	mod_node_page_state(page_pgdat(page), NR_KERNEL_MISC_RECLAIMABLE,
+							-(1 << pool->order));
 	return page;
 }
 
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 32699b2dc52a..c2f6bc4c9e8a 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -180,7 +180,7 @@ enum node_stat_item {
 	NR_VMSCAN_IMMEDIATE,	/* Prioritise for reclaim when writeback ends */
 	NR_DIRTIED,		/* page dirtyings since bootup */
 	NR_WRITTEN,		/* page writings since bootup */
-	NR_INDIRECTLY_RECLAIMABLE_BYTES, /* measured in bytes */
+	NR_KERNEL_MISC_RECLAIMABLE,	/* reclaimable non-slab kernel pages */
 	NR_VM_NODE_STAT_ITEMS
 };
 
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5d800d61ddb7..91f75bf4404d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4704,6 +4704,7 @@ long si_mem_available(void)
 	unsigned long pagecache;
 	unsigned long wmark_low = 0;
 	unsigned long pages[NR_LRU_LISTS];
+	unsigned long reclaimable;
 	struct zone *zone;
 	int lru;
 
@@ -4729,19 +4730,13 @@ long si_mem_available(void)
 	available += pagecache;
 
 	/*
-	 * Part of the reclaimable slab consists of items that are in use,
-	 * and cannot be freed. Cap this estimate at the low watermark.
+	 * Part of the reclaimable slab and other kernel memory consists of
+	 * items that are in use, and cannot be freed. Cap this estimate at the
+	 * low watermark.
 	 */
-	available += global_node_page_state(NR_SLAB_RECLAIMABLE) -
-		     min(global_node_page_state(NR_SLAB_RECLAIMABLE) / 2,
-			 wmark_low);
-
-	/*
-	 * Part of the kernel memory, which can be released under memory
-	 * pressure.
-	 */
-	available += global_node_page_state(NR_INDIRECTLY_RECLAIMABLE_BYTES) >>
-		PAGE_SHIFT;
+	reclaimable = global_node_page_state(NR_SLAB_RECLAIMABLE) +
+			global_node_page_state(NR_KERNEL_MISC_RECLAIMABLE);
+	available += reclaimable - min(reclaimable / 2, wmark_low);
 
 	if (available < 0)
 		available = 0;
diff --git a/mm/util.c b/mm/util.c
index 3351659200e6..891f0654e7b5 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -675,8 +675,7 @@ int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
 		 * Part of the kernel memory, which can be released
 		 * under memory pressure.
 		 */
-		free += global_node_page_state(
-			NR_INDIRECTLY_RECLAIMABLE_BYTES) >> PAGE_SHIFT;
+		free += global_node_page_state(NR_KERNEL_MISC_RECLAIMABLE);
 
 		/*
 		 * Leave reserved pages. The pages are not for anonymous pages.
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 8ba0870ecddd..c5e52f94ba5f 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1161,7 +1161,7 @@ const char * const vmstat_text[] = {
 	"nr_vmscan_immediate_reclaim",
 	"nr_dirtied",
 	"nr_written",
-	"", /* nr_indirectly_reclaimable */
+	"nr_kernel_misc_reclaimable",
 
 	/* enum writeback_stat_item counters */
 	"nr_dirty_threshold",
@@ -1704,10 +1704,6 @@ static int vmstat_show(struct seq_file *m, void *arg)
 	unsigned long *l = arg;
 	unsigned long off = l - (unsigned long *)m->private;
 
-	/* Skip hidden vmstat items. */
-	if (*vmstat_text[off] == '\0')
-		return 0;
-
 	seq_puts(m, vmstat_text[off]);
 	seq_put_decimal_ull(m, " ", *l);
 	seq_putc(m, '\n');
-- 
2.18.0

^ permalink raw reply related

* [PATCH v4 3/6] dcache: allocate external names from reclaimable kmalloc caches
From: Vlastimil Babka @ 2018-07-31  9:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, linux-api, Roman Gushchin, Michal Hocko,
	Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox, Vlastimil Babka
In-Reply-To: <20180731090649.16028-1-vbabka@suse.cz>

We can use the newly introduced kmalloc-reclaimable-X caches, to allocate
external names in dcache, which will take care of the proper accounting
automatically, and also improve anti-fragmentation page grouping.

This effectively reverts commit f1782c9bc547 ("dcache: account external names
as indirectly reclaimable memory") and instead passes __GFP_RECLAIMABLE to
kmalloc(). The accounting thus moves from NR_INDIRECTLY_RECLAIMABLE_BYTES to
NR_SLAB_RECLAIMABLE, which is also considered in MemAvailable calculation and
overcommit decisions.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Roman Gushchin <guro@fb.com>
---
 fs/dcache.c | 38 +++++++++-----------------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 0e8e5de3c48a..518c9ed8db8c 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -257,24 +257,10 @@ static void __d_free(struct rcu_head *head)
 	kmem_cache_free(dentry_cache, dentry); 
 }
 
-static void __d_free_external_name(struct rcu_head *head)
-{
-	struct external_name *name = container_of(head, struct external_name,
-						  u.head);
-
-	mod_node_page_state(page_pgdat(virt_to_page(name)),
-			    NR_INDIRECTLY_RECLAIMABLE_BYTES,
-			    -ksize(name));
-
-	kfree(name);
-}
-
 static void __d_free_external(struct rcu_head *head)
 {
 	struct dentry *dentry = container_of(head, struct dentry, d_u.d_rcu);
-
-	__d_free_external_name(&external_name(dentry)->u.head);
-
+	kfree(external_name(dentry));
 	kmem_cache_free(dentry_cache, dentry);
 }
 
@@ -305,7 +291,7 @@ void release_dentry_name_snapshot(struct name_snapshot *name)
 		struct external_name *p;
 		p = container_of(name->name, struct external_name, name[0]);
 		if (unlikely(atomic_dec_and_test(&p->u.count)))
-			call_rcu(&p->u.head, __d_free_external_name);
+			kfree_rcu(p, u.head);
 	}
 }
 EXPORT_SYMBOL(release_dentry_name_snapshot);
@@ -1608,7 +1594,6 @@ EXPORT_SYMBOL(d_invalidate);
  
 struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 {
-	struct external_name *ext = NULL;
 	struct dentry *dentry;
 	char *dname;
 	int err;
@@ -1629,14 +1614,15 @@ struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 		dname = dentry->d_iname;
 	} else if (name->len > DNAME_INLINE_LEN-1) {
 		size_t size = offsetof(struct external_name, name[1]);
-
-		ext = kmalloc(size + name->len, GFP_KERNEL_ACCOUNT);
-		if (!ext) {
+		struct external_name *p = kmalloc(size + name->len,
+						  GFP_KERNEL_ACCOUNT |
+						  __GFP_RECLAIMABLE);
+		if (!p) {
 			kmem_cache_free(dentry_cache, dentry); 
 			return NULL;
 		}
-		atomic_set(&ext->u.count, 1);
-		dname = ext->name;
+		atomic_set(&p->u.count, 1);
+		dname = p->name;
 	} else  {
 		dname = dentry->d_iname;
 	}	
@@ -1675,12 +1661,6 @@ struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 		}
 	}
 
-	if (unlikely(ext)) {
-		pg_data_t *pgdat = page_pgdat(virt_to_page(ext));
-		mod_node_page_state(pgdat, NR_INDIRECTLY_RECLAIMABLE_BYTES,
-				    ksize(ext));
-	}
-
 	this_cpu_inc(nr_dentry);
 
 	return dentry;
@@ -2761,7 +2741,7 @@ static void copy_name(struct dentry *dentry, struct dentry *target)
 		dentry->d_name.hash_len = target->d_name.hash_len;
 	}
 	if (old_name && likely(atomic_dec_and_test(&old_name->u.count)))
-		call_rcu(&old_name->u.head, __d_free_external_name);
+		kfree_rcu(old_name, u.head);
 }
 
 /*
-- 
2.18.0

^ permalink raw reply related

* [PATCH v4 2/6] mm, slab/slub: introduce kmalloc-reclaimable caches
From: Vlastimil Babka @ 2018-07-31  9:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, linux-api, Roman Gushchin, Michal Hocko,
	Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox, Vlastimil Babka
In-Reply-To: <20180731090649.16028-1-vbabka@suse.cz>

Kmem caches can be created with a SLAB_RECLAIM_ACCOUNT flag, which indicates
they contain objects which can be reclaimed under memory pressure (typically
through a shrinker). This makes the slab pages accounted as NR_SLAB_RECLAIMABLE
in vmstat, which is reflected also the MemAvailable meminfo counter and in
overcommit decisions. The slab pages are also allocated with __GFP_RECLAIMABLE,
which is good for anti-fragmentation through grouping pages by mobility.

The generic kmalloc-X caches are created without this flag, but sometimes are
used also for objects that can be reclaimed, which due to varying size cannot
have a dedicated kmem cache with SLAB_RECLAIM_ACCOUNT flag. A prominent example
are dcache external names, which prompted the creation of a new, manually
managed vmstat counter NR_INDIRECTLY_RECLAIMABLE_BYTES in commit f1782c9bc547
("dcache: account external names as indirectly reclaimable memory").

To better handle this and any other similar cases, this patch introduces
SLAB_RECLAIM_ACCOUNT variants of kmalloc caches, named kmalloc-rcl-X.
They are used whenever the kmalloc() call passes __GFP_RECLAIMABLE among gfp
flags. They are added to the kmalloc_caches array as a new type. Allocations
with both __GFP_DMA and __GFP_RECLAIMABLE will use a dma type cache.

This change only applies to SLAB and SLUB, not SLOB. This is fine, since SLOB's
target are tiny system and this patch does add some overhead of kmem management
objects.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Christoph Lameter <cl@linux.com>
Acked-by: Roman Gushchin <guro@fb.com>
---
 include/linux/slab.h | 16 ++++++++++++++-
 mm/slab_common.c     | 48 ++++++++++++++++++++++++++++----------------
 2 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index f35f2c1f37b9..b79a7cd59875 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -295,8 +295,13 @@ static inline void __check_heap_object(const void *ptr, unsigned long n,
 #define SLAB_OBJ_MIN_SIZE      (KMALLOC_MIN_SIZE < 16 ? \
                                (KMALLOC_MIN_SIZE) : 16)
 
+/*
+ * Whenever changing this, take care of that kmalloc_type() and
+ * create_kmalloc_caches() still work as intended.
+ */
 enum kmalloc_cache_type {
 	KMALLOC_NORMAL = 0,
+	KMALLOC_RECLAIM,
 #ifdef CONFIG_ZONE_DMA
 	KMALLOC_DMA,
 #endif
@@ -310,12 +315,21 @@ kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
 static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
 {
 	int is_dma = 0;
+	int type_dma = 0;
+	int is_reclaimable;
 
 #ifdef CONFIG_ZONE_DMA
 	is_dma = !!(flags & __GFP_DMA);
+	type_dma = is_dma * KMALLOC_DMA;
 #endif
 
-	return is_dma;
+	is_reclaimable = !!(flags & __GFP_RECLAIMABLE);
+
+	/*
+	 * If an allocation is both __GFP_DMA and __GFP_RECLAIMABLE, return
+	 * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE
+	 */
+	return type_dma + (is_reclaimable & !is_dma) * KMALLOC_RECLAIM;
 }
 
 /*
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 4572941440f3..03f40b273ea3 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1108,10 +1108,21 @@ void __init setup_kmalloc_cache_index_table(void)
 	}
 }
 
-static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
+static void __init
+new_kmalloc_cache(int idx, int type, slab_flags_t flags)
 {
-	kmalloc_caches[KMALLOC_NORMAL][idx] = create_kmalloc_cache(
-					kmalloc_info[idx].name,
+	const char *name;
+
+	if (type == KMALLOC_RECLAIM) {
+		flags |= SLAB_RECLAIM_ACCOUNT;
+		name = kasprintf(GFP_NOWAIT, "kmalloc-rcl-%u",
+						kmalloc_info[idx].size);
+		BUG_ON(!name);
+	} else {
+		name = kmalloc_info[idx].name;
+	}
+
+	kmalloc_caches[type][idx] = create_kmalloc_cache(name,
 					kmalloc_info[idx].size, flags, 0,
 					kmalloc_info[idx].size);
 }
@@ -1123,22 +1134,25 @@ static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
  */
 void __init create_kmalloc_caches(slab_flags_t flags)
 {
-	int i;
-	int type = KMALLOC_NORMAL;
+	int i, type;
 
-	for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
-		if (!kmalloc_caches[type][i])
-			new_kmalloc_cache(i, flags);
+	for (type = KMALLOC_NORMAL; type <= KMALLOC_RECLAIM; type++) {
+		for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
+			if (!kmalloc_caches[type][i])
+				new_kmalloc_cache(i, type, flags);
 
-		/*
-		 * Caches that are not of the two-to-the-power-of size.
-		 * These have to be created immediately after the
-		 * earlier power of two caches
-		 */
-		if (KMALLOC_MIN_SIZE <= 32 && !kmalloc_caches[type][1] && i == 6)
-			new_kmalloc_cache(1, flags);
-		if (KMALLOC_MIN_SIZE <= 64 && !kmalloc_caches[type][2] && i == 7)
-			new_kmalloc_cache(2, flags);
+			/*
+			 * Caches that are not of the two-to-the-power-of size.
+			 * These have to be created immediately after the
+			 * earlier power of two caches
+			 */
+			if (KMALLOC_MIN_SIZE <= 32 && i == 6 &&
+					!kmalloc_caches[type][1])
+				new_kmalloc_cache(1, type, flags);
+			if (KMALLOC_MIN_SIZE <= 64 && i == 7 &&
+					!kmalloc_caches[type][2])
+				new_kmalloc_cache(2, type, flags);
+		}
 	}
 
 	/* Kmalloc array is now usable */
-- 
2.18.0

^ permalink raw reply related

* [PATCH v4 1/6] mm, slab: combine kmalloc_caches and kmalloc_dma_caches
From: Vlastimil Babka @ 2018-07-31  9:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, linux-api, Roman Gushchin, Michal Hocko,
	Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox, Vlastimil Babka
In-Reply-To: <20180731090649.16028-1-vbabka@suse.cz>

The kmalloc caches currently mainain separate (optional) array
kmalloc_dma_caches for __GFP_DMA allocations. There are tests for __GFP_DMA in
the allocation hotpaths. We can avoid the branches by combining kmalloc_caches
and kmalloc_dma_caches into a single two-dimensional array where the outer
dimension is cache "type". This will also allow to add kmalloc-reclaimable
caches as a third type.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Christoph Lameter <cl@linux.com>
Acked-by: Roman Gushchin <guro@fb.com>
---
 include/linux/slab.h | 42 +++++++++++++++++++++++++++++++-----------
 mm/slab.c            |  4 ++--
 mm/slab_common.c     | 31 ++++++++++++-------------------
 mm/slub.c            | 13 +++++++------
 4 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 14e3fe4bd6a1..f35f2c1f37b9 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -295,12 +295,29 @@ static inline void __check_heap_object(const void *ptr, unsigned long n,
 #define SLAB_OBJ_MIN_SIZE      (KMALLOC_MIN_SIZE < 16 ? \
                                (KMALLOC_MIN_SIZE) : 16)
 
+enum kmalloc_cache_type {
+	KMALLOC_NORMAL = 0,
+#ifdef CONFIG_ZONE_DMA
+	KMALLOC_DMA,
+#endif
+	NR_KMALLOC_TYPES
+};
+
 #ifndef CONFIG_SLOB
-extern struct kmem_cache *kmalloc_caches[KMALLOC_SHIFT_HIGH + 1];
+extern struct kmem_cache *
+kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
+
+static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
+{
+	int is_dma = 0;
+
 #ifdef CONFIG_ZONE_DMA
-extern struct kmem_cache *kmalloc_dma_caches[KMALLOC_SHIFT_HIGH + 1];
+	is_dma = !!(flags & __GFP_DMA);
 #endif
 
+	return is_dma;
+}
+
 /*
  * Figure out which kmalloc slab an allocation of a certain size
  * belongs to.
@@ -501,18 +518,20 @@ static __always_inline void *kmalloc_large(size_t size, gfp_t flags)
 static __always_inline void *kmalloc(size_t size, gfp_t flags)
 {
 	if (__builtin_constant_p(size)) {
+#ifndef CONFIG_SLOB
+		unsigned int index;
+#endif
 		if (size > KMALLOC_MAX_CACHE_SIZE)
 			return kmalloc_large(size, flags);
 #ifndef CONFIG_SLOB
-		if (!(flags & GFP_DMA)) {
-			unsigned int index = kmalloc_index(size);
+		index = kmalloc_index(size);
 
-			if (!index)
-				return ZERO_SIZE_PTR;
+		if (!index)
+			return ZERO_SIZE_PTR;
 
-			return kmem_cache_alloc_trace(kmalloc_caches[index],
-					flags, size);
-		}
+		return kmem_cache_alloc_trace(
+				kmalloc_caches[kmalloc_type(flags)][index],
+				flags, size);
 #endif
 	}
 	return __kmalloc(size, flags);
@@ -542,13 +561,14 @@ static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
 {
 #ifndef CONFIG_SLOB
 	if (__builtin_constant_p(size) &&
-		size <= KMALLOC_MAX_CACHE_SIZE && !(flags & GFP_DMA)) {
+		size <= KMALLOC_MAX_CACHE_SIZE) {
 		unsigned int i = kmalloc_index(size);
 
 		if (!i)
 			return ZERO_SIZE_PTR;
 
-		return kmem_cache_alloc_node_trace(kmalloc_caches[i],
+		return kmem_cache_alloc_node_trace(
+				kmalloc_caches[kmalloc_type(flags)][i],
 						flags, node, size);
 	}
 #endif
diff --git a/mm/slab.c b/mm/slab.c
index aa76a70e087e..9515798f37b2 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1288,7 +1288,7 @@ void __init kmem_cache_init(void)
 	 * Initialize the caches that provide memory for the  kmem_cache_node
 	 * structures first.  Without this, further allocations will bug.
 	 */
-	kmalloc_caches[INDEX_NODE] = create_kmalloc_cache(
+	kmalloc_caches[KMALLOC_NORMAL][INDEX_NODE] = create_kmalloc_cache(
 				kmalloc_info[INDEX_NODE].name,
 				kmalloc_size(INDEX_NODE), ARCH_KMALLOC_FLAGS,
 				0, kmalloc_size(INDEX_NODE));
@@ -1304,7 +1304,7 @@ void __init kmem_cache_init(void)
 		for_each_online_node(nid) {
 			init_list(kmem_cache, &init_kmem_cache_node[CACHE_CACHE + nid], nid);
 
-			init_list(kmalloc_caches[INDEX_NODE],
+			init_list(kmalloc_caches[KMALLOC_NORMAL][INDEX_NODE],
 					  &init_kmem_cache_node[SIZE_NODE + nid], nid);
 		}
 	}
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 2296caf87bfb..4572941440f3 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -973,14 +973,10 @@ struct kmem_cache *__init create_kmalloc_cache(const char *name,
 	return s;
 }
 
-struct kmem_cache *kmalloc_caches[KMALLOC_SHIFT_HIGH + 1] __ro_after_init;
+struct kmem_cache *
+kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1] __ro_after_init;
 EXPORT_SYMBOL(kmalloc_caches);
 
-#ifdef CONFIG_ZONE_DMA
-struct kmem_cache *kmalloc_dma_caches[KMALLOC_SHIFT_HIGH + 1] __ro_after_init;
-EXPORT_SYMBOL(kmalloc_dma_caches);
-#endif
-
 /*
  * Conversion table for small slabs sizes / 8 to the index in the
  * kmalloc array. This is necessary for slabs < 192 since we have non power
@@ -1040,12 +1036,7 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 	} else
 		index = fls(size - 1);
 
-#ifdef CONFIG_ZONE_DMA
-	if (unlikely((flags & GFP_DMA)))
-		return kmalloc_dma_caches[index];
-
-#endif
-	return kmalloc_caches[index];
+	return kmalloc_caches[kmalloc_type(flags)][index];
 }
 
 /*
@@ -1119,7 +1110,8 @@ void __init setup_kmalloc_cache_index_table(void)
 
 static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
 {
-	kmalloc_caches[idx] = create_kmalloc_cache(kmalloc_info[idx].name,
+	kmalloc_caches[KMALLOC_NORMAL][idx] = create_kmalloc_cache(
+					kmalloc_info[idx].name,
 					kmalloc_info[idx].size, flags, 0,
 					kmalloc_info[idx].size);
 }
@@ -1132,9 +1124,10 @@ static void __init new_kmalloc_cache(int idx, slab_flags_t flags)
 void __init create_kmalloc_caches(slab_flags_t flags)
 {
 	int i;
+	int type = KMALLOC_NORMAL;
 
 	for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
-		if (!kmalloc_caches[i])
+		if (!kmalloc_caches[type][i])
 			new_kmalloc_cache(i, flags);
 
 		/*
@@ -1142,9 +1135,9 @@ void __init create_kmalloc_caches(slab_flags_t flags)
 		 * These have to be created immediately after the
 		 * earlier power of two caches
 		 */
-		if (KMALLOC_MIN_SIZE <= 32 && !kmalloc_caches[1] && i == 6)
+		if (KMALLOC_MIN_SIZE <= 32 && !kmalloc_caches[type][1] && i == 6)
 			new_kmalloc_cache(1, flags);
-		if (KMALLOC_MIN_SIZE <= 64 && !kmalloc_caches[2] && i == 7)
+		if (KMALLOC_MIN_SIZE <= 64 && !kmalloc_caches[type][2] && i == 7)
 			new_kmalloc_cache(2, flags);
 	}
 
@@ -1153,7 +1146,7 @@ void __init create_kmalloc_caches(slab_flags_t flags)
 
 #ifdef CONFIG_ZONE_DMA
 	for (i = 0; i <= KMALLOC_SHIFT_HIGH; i++) {
-		struct kmem_cache *s = kmalloc_caches[i];
+		struct kmem_cache *s = kmalloc_caches[KMALLOC_NORMAL][i];
 
 		if (s) {
 			unsigned int size = kmalloc_size(i);
@@ -1161,8 +1154,8 @@ void __init create_kmalloc_caches(slab_flags_t flags)
 				 "dma-kmalloc-%u", size);
 
 			BUG_ON(!n);
-			kmalloc_dma_caches[i] = create_kmalloc_cache(n,
-				size, SLAB_CACHE_DMA | flags, 0, 0);
+			kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache(
+				n, size, SLAB_CACHE_DMA | flags, 0, 0);
 		}
 	}
 #endif
diff --git a/mm/slub.c b/mm/slub.c
index 51258eff4178..a7b4657ea8e0 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4659,6 +4659,7 @@ static int list_locations(struct kmem_cache *s, char *buf,
 static void __init resiliency_test(void)
 {
 	u8 *p;
+	int type = KMALLOC_NORMAL;
 
 	BUILD_BUG_ON(KMALLOC_MIN_SIZE > 16 || KMALLOC_SHIFT_HIGH < 10);
 
@@ -4671,7 +4672,7 @@ static void __init resiliency_test(void)
 	pr_err("\n1. kmalloc-16: Clobber Redzone/next pointer 0x12->0x%p\n\n",
 	       p + 16);
 
-	validate_slab_cache(kmalloc_caches[4]);
+	validate_slab_cache(kmalloc_caches[type][4]);
 
 	/* Hmmm... The next two are dangerous */
 	p = kzalloc(32, GFP_KERNEL);
@@ -4680,33 +4681,33 @@ static void __init resiliency_test(void)
 	       p);
 	pr_err("If allocated object is overwritten then not detectable\n\n");
 
-	validate_slab_cache(kmalloc_caches[5]);
+	validate_slab_cache(kmalloc_caches[type][5]);
 	p = kzalloc(64, GFP_KERNEL);
 	p += 64 + (get_cycles() & 0xff) * sizeof(void *);
 	*p = 0x56;
 	pr_err("\n3. kmalloc-64: corrupting random byte 0x56->0x%p\n",
 	       p);
 	pr_err("If allocated object is overwritten then not detectable\n\n");
-	validate_slab_cache(kmalloc_caches[6]);
+	validate_slab_cache(kmalloc_caches[type][6]);
 
 	pr_err("\nB. Corruption after free\n");
 	p = kzalloc(128, GFP_KERNEL);
 	kfree(p);
 	*p = 0x78;
 	pr_err("1. kmalloc-128: Clobber first word 0x78->0x%p\n\n", p);
-	validate_slab_cache(kmalloc_caches[7]);
+	validate_slab_cache(kmalloc_caches[type][7]);
 
 	p = kzalloc(256, GFP_KERNEL);
 	kfree(p);
 	p[50] = 0x9a;
 	pr_err("\n2. kmalloc-256: Clobber 50th byte 0x9a->0x%p\n\n", p);
-	validate_slab_cache(kmalloc_caches[8]);
+	validate_slab_cache(kmalloc_caches[type][8]);
 
 	p = kzalloc(512, GFP_KERNEL);
 	kfree(p);
 	p[512] = 0xab;
 	pr_err("\n3. kmalloc-512: Clobber redzone 0xab->0x%p\n\n", p);
-	validate_slab_cache(kmalloc_caches[9]);
+	validate_slab_cache(kmalloc_caches[type][9]);
 }
 #else
 #ifdef CONFIG_SYSFS
-- 
2.18.0

^ permalink raw reply related

* [PATCH v4 0/6] kmalloc-reclaimable caches
From: Vlastimil Babka @ 2018-07-31  9:06 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, linux-api, Roman Gushchin, Michal Hocko,
	Johannes Weiner, Christoph Lameter, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox, Vlastimil Babka, Laura Abbott,
	Sumit Semwal, Vijayanand Jitta

v4 changes:
- drop mm, slab: allocate off-slab freelists as reclaimable when appropriate
  - per Mel, reclaiming objects from a slab doesn't guarantee that the whole
    slab page will be freed and freelist reclaimed, so it can do more harm
    than good
- change KMALLOC_* constants from #define to enum, per Christoph
- use the symbolic constants in kmalloc_type(), per Roman
- add acks from Mel, Roman and Christoph, thanks!
v3 changes:
- fix missing hunk in patch 5/7
- more verbose cover letter and patch 6/7 commit log
v2 changes:
- shorten cache names to kmalloc-rcl-<SIZE>
- last patch shortens <SIZE> for all kmalloc caches to e.g. "1k", "4M"
- include dma caches to the 2D kmalloc_caches[] array to avoid a branch
- vmstat counter nr_indirectly_reclaimable_bytes renamed to
  nr_kernel_misc_reclaimable, doesn't include kmalloc-rcl-*
- /proc/meminfo counter renamed to KReclaimable, includes kmalloc-rcl*
  and nr_kernel_misc_reclaimable

Hi,

as discussed at LSF/MM [1] here's a patchset that introduces
kmalloc-reclaimable caches (more details in the second patch) and uses them for
dcache external names. That allows us to repurpose the
NR_INDIRECTLY_RECLAIMABLE_BYTES counter later in the series.

With patch 3/6, dcache external names are allocated from kmalloc-rcl-*
caches, eliminating the need for manual accounting. More importantly, it
also ensures the reclaimable kmalloc allocations are grouped in pages
separate from the regular kmalloc allocations. The need for proper
accounting of dcache external names has shown it's easy for misbehaving
process to allocate lots of them, causing premature OOMs. Without the
added grouping, it's likely that a similar workload can interleave the
dcache external names allocations with regular kmalloc allocations
(note: I haven't searched myself for an example of such regular kmalloc
allocation, but I would be very surprised if there wasn't some). A
pathological case would be e.g. one 64byte regular allocations with 63
external dcache names in a page (64x64=4096), which means the page is
not freed even after reclaiming after all dcache names, and the process
can thus "steal" the whole page with single 64byte allocation.

If there other kmalloc users similar to dcache external names become
identified, they can also benefit from the new functionality simply by
adding __GFP_RECLAIMABLE to the kmalloc calls.

Side benefits of the patchset (that could be also merged separately)
include removed branch for detecting __GFP_DMA kmalloc(), and shortening
kmalloc cache names in /proc/slabinfo output. The latter is potentially
an ABI break in case there are tools parsing the names and expecting the
values to be in bytes.

This is how /proc/slabinfo looks like after booting in virtme:

...
kmalloc-rcl-4M         0      0 4194304    1 1024 : tunables    1    1    0 : slabdata      0      0      0
...
kmalloc-rcl-96         7     32    128   32    1 : tunables  120   60    8 : slabdata      1      1      0
kmalloc-rcl-64        25    128     64   64    1 : tunables  120   60    8 : slabdata      2      2      0
kmalloc-rcl-32         0      0     32  124    1 : tunables  120   60    8 : slabdata      0      0      0
kmalloc-4M             0      0 4194304    1 1024 : tunables    1    1    0 : slabdata      0      0      0
kmalloc-2M             0      0 2097152    1  512 : tunables    1    1    0 : slabdata      0      0      0
kmalloc-1M             0      0 1048576    1  256 : tunables    1    1    0 : slabdata      0      0      0
...

/proc/vmstat with renamed nr_indirectly_reclaimable_bytes counter:

...
nr_slab_reclaimable 2817
nr_slab_unreclaimable 1781
...
nr_kernel_misc_reclaimable 0
...

/proc/meminfo with new KReclaimable counter:

...
Shmem:               564 kB
KReclaimable:      11260 kB
Slab:              18368 kB
SReclaimable:      11260 kB
SUnreclaim:         7108 kB
KernelStack:        1248 kB
...

Thanks,
Vlastimil

Vlastimil Babka (6):
  mm, slab: combine kmalloc_caches and kmalloc_dma_caches
  mm, slab/slub: introduce kmalloc-reclaimable caches
  dcache: allocate external names from reclaimable kmalloc caches
  mm: rename and change semantics of nr_indirectly_reclaimable_bytes
  mm, proc: add KReclaimable to /proc/meminfo
  mm, slab: shorten kmalloc cache names for large sizes

 Documentation/filesystems/proc.txt          |   4 +
 drivers/base/node.c                         |  19 ++--
 drivers/staging/android/ion/ion_page_pool.c |   8 +-
 fs/dcache.c                                 |  38 ++------
 fs/proc/meminfo.c                           |  16 +--
 include/linux/mmzone.h                      |   2 +-
 include/linux/slab.h                        |  56 ++++++++---
 mm/page_alloc.c                             |  19 ++--
 mm/slab.c                                   |   4 +-
 mm/slab_common.c                            | 103 ++++++++++++--------
 mm/slub.c                                   |  13 +--
 mm/util.c                                   |   3 +-
 mm/vmstat.c                                 |   6 +-
 13 files changed, 163 insertions(+), 128 deletions(-)

-- 
2.18.0

^ permalink raw reply

* Re: [PATCH v3 7/7] mm, slab: shorten kmalloc cache names for large sizes
From: Vlastimil Babka @ 2018-07-31  8:55 UTC (permalink / raw)
  To: Christopher Lameter
  Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
	Michal Hocko, Johannes Weiner, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox
In-Reply-To: <01000164ebe0d06f-8f639717-8d32-4eb9-9cc1-708332b12ca6-000000@email.amazonses.com>

On 07/30/2018 05:48 PM, Christopher Lameter wrote:
> On Wed, 18 Jul 2018, Vlastimil Babka wrote:
> 
>> +static const char *
>> +kmalloc_cache_name(const char *prefix, unsigned int size)
>> +{
>> +
>> +	static const char units[3] = "\0kM";
>> +	int idx = 0;
>> +
>> +	while (size >= 1024 && (size % 1024 == 0)) {
>> +		size /= 1024;
>> +		idx++;
>> +	}
>> +
>> +	return kasprintf(GFP_NOWAIT, "%s-%u%c", prefix, size, units[idx]);
>> +}
> 
> This is likely to occur elsewhere in the kernel. Maybe generalize it a
> bit?

I'll try later on top, as that's generic printf code then.

> Acked-by: Christoph Lameter <cl@linux.com>

Thanks for all acks.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* [PATCH v2] prctl: add PR_[GS]ET_KILLABLE
From: Jürg Billeter @ 2018-07-31  7:03 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Oleg Nesterov, Thomas Gleixner, Eric Biederman, linux-api,
	linux-kernel, Jürg Billeter
In-Reply-To: <20180730075241.24002-1-j@bitron.ch>

PR_SET_KILLABLE clears the SIGNAL_UNKILLABLE flag. This allows
CLONE_NEWPID tasks to restore normal signal behavior, opting out of the
special signal protection for init processes. This prctl does not allow
setting the SIGNAL_UNKILLABLE flag, only clearing.

The SIGNAL_UNKILLABLE flag, which is implicitly set for tasks cloned
with CLONE_NEWPID, has the effect of ignoring all signals (from
userspace) if the corresponding handler is set to SIG_DFL. The only
exceptions are SIGKILL and SIGSTOP and they are only accepted if raised
from an ancestor namespace.

SIGINT, SIGQUIT and SIGTSTP are used in job control for ^C, ^\, ^Z.
While a task with the SIGNAL_UNKILLABLE flag could install handlers for
these signals, this is not sufficient to implement a shell that uses
CLONE_NEWPID for child processes:

 * As SIGSTOP is ignored when raised from the SIGNAL_UNKILLABLE process
   itself, it's not possible to implement the stop action in a custom
   SIGTSTP handler.
 * Many applications do not install handlers for these signals and
   thus, job control won't work properly with unmodified applications.

There are other scenarios besides job control in a shell where
applications rely on the default actions as described in signal(7) and
PID isolation may be useful. This new prctl makes the signal protection
for "init" processes optional, without breaking backward compatibility.

Signed-off-by: Jürg Billeter <j@bitron.ch>
---
 v2: Hold siglock for PR_SET_KILLABLE, expand commit message.

 include/uapi/linux/prctl.h |  4 ++++
 kernel/sys.c               | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index c0d7ea0bf5b6..92afb63da727 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -219,4 +219,8 @@ struct prctl_mm_map {
 # define PR_SPEC_DISABLE		(1UL << 2)
 # define PR_SPEC_FORCE_DISABLE		(1UL << 3)
 
+/* Control SIGNAL_UNKILLABLE */
+#define PR_GET_KILLABLE			54
+#define PR_SET_KILLABLE			55
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 38509dc1f77b..92c9322cfb98 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2484,6 +2484,19 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			return -EINVAL;
 		error = arch_prctl_spec_ctrl_set(me, arg2, arg3);
 		break;
+	case PR_GET_KILLABLE:
+		if (arg3 || arg4 || arg5)
+			return -EINVAL;
+		error = put_user(!(me->signal->flags & SIGNAL_UNKILLABLE),
+				 (int __user *)arg2);
+		break;
+	case PR_SET_KILLABLE:
+		if (arg2 != 1 || arg3 || arg4 || arg5)
+			return -EINVAL;
+		spin_lock_irq(&me->sighand->siglock);
+		me->signal->flags &= ~SIGNAL_UNKILLABLE;
+		spin_unlock_irq(&me->sighand->siglock);
+		break;
 	default:
 		error = -EINVAL;
 		break;
-- 
2.18.0

^ permalink raw reply related

* Re: [PATCH 34/38] vfs: syscall: Add fsinfo() to query filesystem information [ver #10]
From: Al Viro @ 2018-07-31  4:16 UTC (permalink / raw)
  To: David Howells; +Cc: linux-api, torvalds, linux-fsdevel, linux-kernel
In-Reply-To: <153271291017.9458.7827028432894772673.stgit@warthog.procyon.org.uk>

On Fri, Jul 27, 2018 at 06:35:10PM +0100, David Howells wrote:
> params->request indicates the attribute/attributes to be queried.  This can
> be one of:
> 
> 	fsinfo_attr_statfs		- statfs-style info
> 	fsinfo_attr_fsinfo		- Information about fsinfo()
> 	fsinfo_attr_ids			- Filesystem IDs
> 	fsinfo_attr_limits		- Filesystem limits
> 	fsinfo_attr_supports		- What's supported in statx(), IOC flags
> 	fsinfo_attr_capabilities	- Filesystem capabilities
> 	fsinfo_attr_timestamp_info	- Inode timestamp info
> 	fsinfo_attr_volume_id		- Volume ID (string)
> 	fsinfo_attr_volume_uuid		- Volume UUID
> 	fsinfo_attr_volume_name		- Volume name (string)
> 	fsinfo_attr_cell_name		- Cell name (string)
> 	fsinfo_attr_domain_name		- Domain name (string)
> 	fsinfo_attr_realm_name		- Realm name (string)
> 	fsinfo_attr_server_name		- Name of the Nth server (string)
> 	fsinfo_attr_server_address	- Mth address of the Nth server
> 	fsinfo_attr_parameter		- Nth mount parameter (string)
> 	fsinfo_attr_source		- Nth mount source name (string)
> 	fsinfo_attr_name_encoding	- Filename encoding (string)
> 	fsinfo_attr_name_codepage	- Filename codepage (string)
> 	fsinfo_attr_io_size		- I/O size hints

Umm...  What's so special about cell/volume/domain/realm?  And
what do we do when a random filesystem gets added - should its
parameters go into catch-all pile (attr_parameter), or should they
get classes of their own?

For Cthulhu sake, who's going to maintain that enum in face of
random out-of-tree filesystems, each wanting a class or two its own?
We'd tried that with device numbers; ask hpa how well has that
worked and how much did he love the whole experience...

^ permalink raw reply

* Re: [PATCH] prctl: add PR_[GS]ET_KILLABLE
From: Thomas Gleixner @ 2018-07-30 19:39 UTC (permalink / raw)
  To: Jürg Billeter
  Cc: Oleg Nesterov, Andrew Morton, Eric Biederman, linux-api,
	linux-kernel
In-Reply-To: <922baeb3d6d6387c480026fc3a6ca01d915f60a5.camel@bitron.ch>

[-- Attachment #1: Type: text/plain, Size: 1665 bytes --]

On Mon, 30 Jul 2018, Jürg Billeter wrote:

> On Mon, 2018-07-30 at 12:17 +0200, Oleg Nesterov wrote:
> > On 07/30, Jürg Billeter wrote:
> > > 
> > > This is required for job control in a shell that uses CLONE_NEWPID for
> > > child processes.
> > 
> > Could you explain in more details?
> 
> The SIGNAL_UNKILLABLE flag, which is implicitly set for tasks cloned
> with CLONE_NEWPID, has the effect of ignoring all signals (from
> userspace) if the corresponding handler is set to SIG_DFL. The only
> exceptions are SIGKILL and SIGSTOP and they are only accepted if raised
> from an ancestor namespace.
> 
> SIGINT, SIGQUIT and SIGTSTP are used in job control for ^C, ^\, ^Z.
> While a task with the SIGNAL_UNKILLABLE flag could install handlers for
> these signals, this is not sufficient to implement a shell that uses
> CLONE_NEWPID for child processes:
> 
>  * As SIGSTOP is ignored when raised from the SIGNAL_UNKILLABLE process
>    itself, I don't think it's possible to implement the stop action in
>    a custom SIGTSTP handler.
>  * Many applications do not install handlers for these signals and
>    thus, job control won't work properly with unmodified applications.
> 
> Job control in a shell is just an example. There are other scenarios,
> of course, where applications rely on the default actions as described
> in signal(7), and PID isolation may be useful. In my opinion, the
> kernel support for preventing accidental killing of the "init" process
> should really be optional and this new prctl provides this without
> breaking backward compatibility.

This makes sense and exactly that information needs to be in the changelog.

Thanks,

	tglx

^ permalink raw reply

* Re: [RFC PATCH for 4.18 00/16] Restartable Sequences
From: Mathieu Desnoyers @ 2018-07-30 19:34 UTC (permalink / raw)
  To: Pavel Machek
  Cc: carlos, Florian Weimer, Peter Zijlstra, Paul E. McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, linux-kernel, linux-api,
	Paul Turner, Andrew Morton, Russell King, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, Andrew Hunter, Andi Kleen,
	Chris Lameter, Ben Maurer, rostedt, Josh Triplett, Linu
In-Reply-To: <20180730190715.GA29926@amd>

----- On Jul 30, 2018, at 3:07 PM, Pavel Machek pavel@ucw.cz wrote:

> Hi!
> 
>> > Thanks for pointer.
>> > 
>> > +Restartable sequences are atomic with respect to preemption (making
>> > it
>> > +atomic with respect to other threads running on the same CPU), as
>> > well
>> > +as signal delivery (user-space execution contexts nested over the
>> > same
>> > +thread).
>> > 
>> > So the threads are protected against sigkill when running the
>> > restartable sequence?
>> 
>> In that scenario, SIGKILL _will_ be delivered, hence execution of the
>> rseq critical section will never reach the commit instruction. This
>> follows the guarantee provided that the rseq c.s. either executes
>> completely "atomically" wrt preemption/signal delivery, *or* gets
>> aborted. In this case, sigkill will reap the entire process, so
> 
> The text above does not mention abort -- so I was just making
> sure. Maybe mentioning it would be good idea?

How about this ?

Restartable sequences are atomic with respect to preemption (making it
atomic with respect to other threads running on the same CPU), as well
as signal delivery (user-space execution contexts nested over the same
thread). They either complete atomically with respect to preemption on
the current CPU and signal delivery, or they are aborted.

[...]

> 
>> > +Optimistic cache of the CPU number on which the current thread is
>> > +running. Its value is guaranteed to always be a possible CPU number,
>> > +even when rseq is not initialized. The value it contains should
>> > always
>> > +be confirmed by reading the cpu_id field.
>> > 
>> > I'm not sure what "optimistic cache" is...
>> 
>> Perhaps we can find a better wording.
>> 
>> It's "optimistic" in the sense that it's always guaranteed to hold a
>> valid CPU number within the range [ 0 .. nr_possible_cpus - 1 ]. It can
>> therefore be loaded by user-space and then used as an offset, without
>> having to check whether it is within valid bounds compared to the number
>> of possible CPUs in the system.
>> 
>> This works even if the kernel on which the application runs on does not
>> support rseq at all: the __rseq_abi->cpu_id_start field stays initialized at
>> 0, which is indeed a valid CPU number. It's therefore valid to use it as an
>> offset in per-cpu data structures, and only validate whether it's actually the
>> current CPU number by comparing it with the __rseq_abi->cpu_id field
>> within the rseq critical section. If rseq is not available in the kernel,
>> that cpu_id field stays initialized at -1, so the comparison always fails,
>> as intended.
>> 
>> It's then up to user-space to use a fall-back mechanism, considering that
>> rseq is not available.
>> 
>> Advice on improved wording would be welcome.
> 
> Ok, that makes sense, but I'd not understand it from the man
> page. Perhaps your text should be put there?

How about this ?

.TP
.in +4n
.I cpu_id_start
Optimistic cache of the CPU number on which the current thread is
running. Its value is guaranteed to always be a possible CPU number,
even when rseq is not initialized. The value it contains should always
be confirmed by reading the cpu_id field.

This field is an optimistic cache in the sense that it is always
guaranteed to hold a valid CPU number in the range [ 0 ..
nr_possible_cpus - 1 ]. It can therefore be loaded by user-space and
used as an offset in per-cpu data structures without having to
check whether its value is within the valid bounds compared to the
number of possible CPUs in the system.

For user-space applications executed on a kernel without rseq support,
the cpu_id_start field stays initialized at 0, which is indeed a valid
CPU number. It is therefore valid to use it as an offset in per-cpu data
structures, and only validate whether it's actually the current CPU
number by comparing it with the cpu_id field within the rseq critical
section. If the kernel does not provide rseq support, that cpu_id field
stays initialized at -1, so the comparison always fails, as intended.

It is then up to user-space to use a fall-back mechanism, considering
that rseq is not available.

[...]

> 
>> > (Will not
>> > this need to be bigger on machines with bigger cache sizes?)
>> > 
>> > above it says:
>> > 
>> > +.B Structure size
>> > +This structure is extensible. Its size is passed as parameter to the
>> > +rseq system call.
>> > 
>> > I'm reading source, so maybe it refers to different structure.
>> 
>> It can be aligned on a larger multiple. This requirement of 32 bytes
>> is a minimum. Therefore, if we ever extend struct rseq, or if an
>> architecture shows benefit from aligning struct rseq on larger boundaries,
>> it is free to do so. It will still respect the requirement of alignment on
>> 32 bytes boundaries.
> 
> Well, elsewhere it said "This structure has a fixed size of 32 bytes."
> Now it says structure size is passed with every syscalls. Now I'm
> confused (but maybe that's caused by reading source, not formatted
> document).

This is the layout for struct rseq_cs version 0.

The variable-sized structure is struct rseq.

struct rseq is typically in a TLS, and contains a "rseq_cs" field
which is a pointer to the struct rseq_cs descriptor describing the
currently active rseq critical section.

Hoping this clears up the confusion.

Thanks for the review!

Mathieu


> 
> Best regards,
>									Pavel
> 
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply

* Re: [PATCH] prctl: add PR_[GS]ET_KILLABLE
From: Jürg Billeter @ 2018-07-30 19:32 UTC (permalink / raw)
  To: Oleg Nesterov; +Cc: Andrew Morton, Eric Biederman, linux-api, linux-kernel
In-Reply-To: <20180730101659.GA24781@redhat.com>

On Mon, 2018-07-30 at 12:17 +0200, Oleg Nesterov wrote:
> On 07/30, Jürg Billeter wrote:
> > 
> > This is required for job control in a shell that uses CLONE_NEWPID for
> > child processes.
> 
> Could you explain in more details?

The SIGNAL_UNKILLABLE flag, which is implicitly set for tasks cloned
with CLONE_NEWPID, has the effect of ignoring all signals (from
userspace) if the corresponding handler is set to SIG_DFL. The only
exceptions are SIGKILL and SIGSTOP and they are only accepted if raised
from an ancestor namespace.

SIGINT, SIGQUIT and SIGTSTP are used in job control for ^C, ^\, ^Z.
While a task with the SIGNAL_UNKILLABLE flag could install handlers for
these signals, this is not sufficient to implement a shell that uses
CLONE_NEWPID for child processes:

 * As SIGSTOP is ignored when raised from the SIGNAL_UNKILLABLE process
   itself, I don't think it's possible to implement the stop action in
   a custom SIGTSTP handler.
 * Many applications do not install handlers for these signals and
   thus, job control won't work properly with unmodified applications.

Job control in a shell is just an example. There are other scenarios,
of course, where applications rely on the default actions as described
in signal(7), and PID isolation may be useful. In my opinion, the
kernel support for preventing accidental killing of the "init" process
should really be optional and this new prctl provides this without
breaking backward compatibility.

> > +	case PR_SET_KILLABLE:
> > +		if (arg2 != 1 || arg3 || arg4 || arg5)
> > +			return -EINVAL;
> > +		me->signal->flags &= ~SIGNAL_UNKILLABLE;
> 
> this needs spin_lock_irq(me->sighand->siglock).

Thanks for the review, will fix this for v2.

Jürg

^ permalink raw reply

* Re: [RFC PATCH for 4.18 00/16] Restartable Sequences
From: Pavel Machek @ 2018-07-30 19:07 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: carlos, Florian Weimer, Peter Zijlstra, Paul E. McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, linux-kernel, linux-api,
	Paul Turner, Andrew Morton, Russell King, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, Andrew Hunter, Andi Kleen,
	Chris Lameter, Ben Maurer, rostedt, Josh Triplett, Linu
In-Reply-To: <1005916991.7038.1532976120931.JavaMail.zimbra@efficios.com>

[-- Attachment #1: Type: text/plain, Size: 4928 bytes --]

Hi!

> > Thanks for pointer.
> > 
> > +Restartable sequences are atomic with respect to preemption (making
> > it
> > +atomic with respect to other threads running on the same CPU), as
> > well
> > +as signal delivery (user-space execution contexts nested over the
> > same
> > +thread).
> > 
> > So the threads are protected against sigkill when running the
> > restartable sequence?
> 
> In that scenario, SIGKILL _will_ be delivered, hence execution of the
> rseq critical section will never reach the commit instruction. This
> follows the guarantee provided that the rseq c.s. either executes
> completely "atomically" wrt preemption/signal delivery, *or* gets
> aborted. In this case, sigkill will reap the entire process, so

The text above does not mention abort -- so I was just making
sure. Maybe mentioning it would be good idea?

> > +Restartable sequences must not perform system calls. Doing so may
> > result
> > +in termination of the process by a segmentation fault.
> > +
> > 
> > "may result"? It would be nice to always catch that.
> 
> I would also like this, but unfortunately this check adds overhead to every
> system call, therefore this is only enforced with CONFIG_DEBUG_RSEQ=y builds.

Makes sense. (But this is probably bad place to talk about
CONFIG_DEBUG_RSEQ=y. Too bad.)

> > +Optimistic cache of the CPU number on which the current thread is
> > +running. Its value is guaranteed to always be a possible CPU number,
> > +even when rseq is not initialized. The value it contains should
> > always
> > +be confirmed by reading the cpu_id field.
> > 
> > I'm not sure what "optimistic cache" is...
> 
> Perhaps we can find a better wording.
> 
> It's "optimistic" in the sense that it's always guaranteed to hold a
> valid CPU number within the range [ 0 .. nr_possible_cpus - 1 ]. It can
> therefore be loaded by user-space and then used as an offset, without
> having to check whether it is within valid bounds compared to the number
> of possible CPUs in the system.
> 
> This works even if the kernel on which the application runs on does not
> support rseq at all: the __rseq_abi->cpu_id_start field stays initialized at
> 0, which is indeed a valid CPU number. It's therefore valid to use it as an
> offset in per-cpu data structures, and only validate whether it's actually the
> current CPU number by comparing it with the __rseq_abi->cpu_id field
> within the rseq critical section. If rseq is not available in the kernel,
> that cpu_id field stays initialized at -1, so the comparison always fails,
> as intended.
> 
> It's then up to user-space to use a fall-back mechanism, considering that
> rseq is not available.
> 
> Advice on improved wording would be welcome.

Ok, that makes sense, but I'd not understand it from the man
page. Perhaps your text should be put there?

> > +Flags indicating the restart behavior for the current thread. This is
> > +mainly used for debugging purposes. Can be either:
> > +.IP \[bu]
> > +RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
> > +.IP \[bu]
> > +RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
> > +.IP \[bu]
> > 
> > Flags tell me there may be more then one, but "can be either" tells me
> > just one flag is allowed.
> 
> Combining them is allowed. Will fix by saying: "Can be a combination of:".

Thanks.

> > +.B Structure alignment
> > +This structure is aligned on multiples of 32 bytes.
> > +.TP
> > +.B Structure size
> > +This structure has a fixed size of 32 bytes.
> > +.B Structure alignment
> > +This structure is aligned on multiples of 32 bytes.
> > +.TP
> > +.B Structure size
> > +This structure has a fixed size of 32 bytes.
> > 
> > I believe we normally say "is aligned on 32-bytes boundary".
> 
> OK will fix. I think it should then become:
> 
> "is aligned on 32-byte boundary." (no plural for byte)

Thanks.

> > (Will not
> > this need to be bigger on machines with bigger cache sizes?)
> > 
> > above it says:
> > 
> > +.B Structure size
> > +This structure is extensible. Its size is passed as parameter to the
> > +rseq system call.
> > 
> > I'm reading source, so maybe it refers to different structure.
> 
> It can be aligned on a larger multiple. This requirement of 32 bytes
> is a minimum. Therefore, if we ever extend struct rseq, or if an
> architecture shows benefit from aligning struct rseq on larger boundaries,
> it is free to do so. It will still respect the requirement of alignment on
> 32 bytes boundaries.

Well, elsewhere it said "This structure has a fixed size of 32 bytes."
Now it says structure size is passed with every syscalls. Now I'm
confused (but maybe that's caused by reading source, not formatted
document).

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* Re: [RFC PATCH ghak90 (was ghak32) V3 01/10] audit: add container id
From: Richard Guy Briggs @ 2018-07-30 18:47 UTC (permalink / raw)
  To: Paul Moore
  Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
	linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
	viro, simo, Eric Paris, serge
In-Reply-To: <CAHC9VhQ3KwG-YmpJsOa9k=YqQmaQTaJXeB=eM9Fm80hujbngBg@mail.gmail.com>

On 2018-07-24 17:54, Paul Moore wrote:
> On Tue, Jul 24, 2018 at 3:09 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-07-20 18:13, Paul Moore wrote:
> > > On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Implement the proc fs write to set the audit container identifier of a
> > > > process, emitting an AUDIT_CONTAINER_ID record to document the event.
> > > >
> > > > This is a write from the container orchestrator task to a proc entry of
> > > > the form /proc/PID/audit_containerid where PID is the process ID of the
> > > > newly created task that is to become the first task in a container, or
> > > > an additional task added to a container.
> > > >
> > > > The write expects up to a u64 value (unset: 18446744073709551615).
> > > >
> > > > The writer must have capability CAP_AUDIT_CONTROL.
> > > >
> > > > This will produce a record such as this:
> > > >   type=CONTAINER_ID msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
> > > >
> > > > The "op" field indicates an initial set.  The "pid" to "ses" fields are
> > > > the orchestrator while the "opid" field is the object's PID, the process
> > > > being "contained".  Old and new audit container identifier values are
> > > > given in the "contid" fields, while res indicates its success.
> > > >
> > > > It is not permitted to unset or re-set the audit container identifier.
> > > > A child inherits its parent's audit container identifier, but then can
> > > > be set only once after.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > > See: https://github.com/linux-audit/audit-userspace/issues/51
> > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  fs/proc/base.c             | 37 ++++++++++++++++++++++++
> > > >  include/linux/audit.h      | 25 ++++++++++++++++
> > > >  include/uapi/linux/audit.h |  2 ++
> > > >  kernel/auditsc.c           | 71 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 135 insertions(+)
> 
> ...
> 
> > > > @@ -2112,6 +2116,73 @@ int audit_set_loginuid(kuid_t loginuid)
> > > >  }
> > > >
> > > >  /**
> > > > + * audit_set_contid - set current task's audit_context contid
> > > > + * @contid: contid value
> > > > + *
> > > > + * Returns 0 on success, -EPERM on permission failure.
> > > > + *
> > > > + * Called (set) from fs/proc/base.c::proc_contid_write().
> > > > + */
> > > > +int audit_set_contid(struct task_struct *task, u64 contid)
> > > > +{
> > > > +       u64 oldcontid;
> > > > +       int rc = 0;
> > > > +       struct audit_buffer *ab;
> > > > +       uid_t uid;
> > > > +       struct tty_struct *tty;
> > > > +       char comm[sizeof(current->comm)];
> > > > +
> > > > +       /* Can't set if audit disabled */
> > > > +       if (!task->audit)
> > > > +               return -ENOPROTOOPT;
> > > > +       oldcontid = audit_get_contid(task);
> > > > +       /* Don't allow the audit containerid to be unset */
> > > > +       if (!cid_valid(contid))
> > > > +               rc = -EINVAL;
> > > > +       /* if we don't have caps, reject */
> > > > +       else if (!capable(CAP_AUDIT_CONTROL))
> > > > +               rc = -EPERM;
> > > > +       /* if task has children or is not single-threaded, deny */
> > > > +       else if (!list_empty(&task->children))
> > > > +               rc = -EBUSY;
> > >
> > > Is this safe without holding tasklist_lock?  I worry we might be
> > > vulnerable to a race with fork().
> > >
> > > > +       else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > > > +               rc = -EALREADY;
> > >
> > > Similar concern here as well, although related to threads.
> >
> > I think you are correct here and tasklist_lock should cover both.  Do we
> > also want rcu_read_lock() immediately preceeding it?
> 
> You'll need to take a closer look and determine the locking scheme. I
> simply took a quick look while reviewing this patch to see what of the
> existing locks, if any, would be most applicable here; tasklist_lock
> seemed like a good starting point.
> 
> It looks like tasklist_lock is defined as a rwlock_t so I'm not sure
> it would make sense to use it with a RCU protected structure
> (typically it's RCU+spinlock), but maybe that is the case with a
> task_struct, you'll need to check.

All I need is a read rather than write tasklist_lock since I'm not
changing any inter-task relationships, which makes it possible to nest
it inside or outside the task_lock().  I don't think I need the RCU
lock.

> > > > +       /* it is already set, and not inherited from the parent, reject */
> > > > +       else if (cid_valid(oldcontid) && !task->audit->inherited)
> > > > +               rc = -EEXIST;
> > >
> > > Maybe I'm missing something, but why do we care about preventing
> > > reassigning the audit container ID in this case?  The task is single
> > > threaded and has no descendants at this point so it should be safe,
> > > yes?  So long as the task changing the audit container ID has
> > > capable(CAP_AUDIT_CONTOL) it shouldn't matter, right?
> >
> > Because we hammered out this idea 6 months ago in the design phase and I
> > thought we all firmly agreed that the audit container identifier could
> > only be set once.  Has any significant discussion happenned since then
> > to change that wisdom?  I just wonder why this is coming up now.
> 
> Implementation, and time, can change how one looks at an earlier
> design.  I believe this is why most well reasoned specifications have
> a reference design.
> 
> Remind me why the design had the restriction of write once for the
> audit container ID?  At this point given the CAP_AUDIT_CONTROL and the
> single-thread, no-children restrictions I'm not sure what harm there
> is in allowing the value to be written multiple times (so long as the
> changes are audited of course).

Looking back through the conversations, I think you may be right that we
no longer need it, but it is easy to re-add if we find it necessary.

> > > Related, I'm questioning if we would ever care if the audit container
> > > ID was inherited or not?
> >
> > We do since that is the only way we can tell if the value has been set
> > once already or inherited unless we check if the parent's audit
> > container identifier is identical (which tells us it was inherited).
> 
> Tied to the above question.  If we don't care about multiple changes,
> given the other constraints, we probably don't need the inherited
> flag.

Agreed.

> paul moore

- RGB

^ permalink raw reply

* Re: [RFC PATCH for 4.18 00/16] Restartable Sequences
From: Mathieu Desnoyers @ 2018-07-30 18:42 UTC (permalink / raw)
  To: Pavel Machek
  Cc: carlos, Florian Weimer, Peter Zijlstra, Paul E. McKenney,
	Boqun Feng, Andy Lutomirski, Dave Watson, linux-kernel, linux-api,
	Paul Turner, Andrew Morton, Russell King, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, Andrew Hunter, Andi Kleen,
	Chris Lameter, Ben Maurer, rostedt, Josh Triplett, Linu
In-Reply-To: <20180728141314.GA25264@amd>

----- On Jul 28, 2018, at 10:13 AM, Pavel Machek pavel@ucw.cz wrote:

> Hi!
> 
>> Documentation-wise, I have posted a rseq man page rfc here:
>> 
>> https://lkml.kernel.org/r/20180616195803.29877-1-mathieu.desnoyers@efficios.com
>> 
>> comments are welcome!
> 
> Thanks for pointer.
> 
> +Restartable sequences are atomic with respect to preemption (making
> it
> +atomic with respect to other threads running on the same CPU), as
> well
> +as signal delivery (user-space execution contexts nested over the
> same
> +thread).
> 
> So the threads are protected against sigkill when running the
> restartable sequence?

In that scenario, SIGKILL _will_ be delivered, hence execution of the
rseq critical section will never reach the commit instruction. This
follows the guarantee provided that the rseq c.s. either executes
completely "atomically" wrt preemption/signal delivery, *or* gets
aborted. In this case, sigkill will reap the entire process, so
the kernel will never actually have to return to userspace and
execute the instruction pointer at the abort_ip, but the rseq c.s.
will never reach the commit instruction.

> 
> +Restartable sequences must not perform system calls. Doing so may
> result
> +in termination of the process by a segmentation fault.
> +
> 
> "may result"? It would be nice to always catch that.

I would also like this, but unfortunately this check adds overhead to every
system call, therefore this is only enforced with CONFIG_DEBUG_RSEQ=y builds.

> 
> +Optimistic cache of the CPU number on which the current thread is
> +running. Its value is guaranteed to always be a possible CPU number,
> +even when rseq is not initialized. The value it contains should
> always
> +be confirmed by reading the cpu_id field.
> 
> I'm not sure what "optimistic cache" is...

Perhaps we can find a better wording.

It's "optimistic" in the sense that it's always guaranteed to hold a
valid CPU number within the range [ 0 .. nr_possible_cpus - 1 ]. It can
therefore be loaded by user-space and then used as an offset, without
having to check whether it is within valid bounds compared to the number
of possible CPUs in the system.

This works even if the kernel on which the application runs on does not
support rseq at all: the __rseq_abi->cpu_id_start field stays initialized at
0, which is indeed a valid CPU number. It's therefore valid to use it as an
offset in per-cpu data structures, and only validate whether it's actually the
current CPU number by comparing it with the __rseq_abi->cpu_id field
within the rseq critical section. If rseq is not available in the kernel,
that cpu_id field stays initialized at -1, so the comparison always fails,
as intended.

It's then up to user-space to use a fall-back mechanism, considering that
rseq is not available.

Advice on improved wording would be welcome.

> 
> +Flags indicating the restart behavior for the current thread. This is
> +mainly used for debugging purposes. Can be either:
> +.IP \[bu]
> +RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
> +.IP \[bu]
> +RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
> +.IP \[bu]
> 
> Flags tell me there may be more then one, but "can be either" tells me
> just one flag is allowed.

Combining them is allowed. Will fix by saying: "Can be a combination of:".

> 
> +.B Structure alignment
> +This structure is aligned on multiples of 32 bytes.
> +.TP
> +.B Structure size
> +This structure has a fixed size of 32 bytes.
> +.B Structure alignment
> +This structure is aligned on multiples of 32 bytes.
> +.TP
> +.B Structure size
> +This structure has a fixed size of 32 bytes.
> 
> I believe we normally say "is aligned on 32-bytes boundary".

OK will fix. I think it should then become:

"is aligned on 32-byte boundary." (no plural for byte)

> (Will not
> this need to be bigger on machines with bigger cache sizes?)
> 
> above it says:
> 
> +.B Structure size
> +This structure is extensible. Its size is passed as parameter to the
> +rseq system call.
> 
> I'm reading source, so maybe it refers to different structure.

It can be aligned on a larger multiple. This requirement of 32 bytes
is a minimum. Therefore, if we ever extend struct rseq, or if an
architecture shows benefit from aligning struct rseq on larger boundaries,
it is free to do so. It will still respect the requirement of alignment on
32 bytes boundaries.

Thoughts ?

Thanks,

Mathieu


> 
> Thanks,
>									Pavel
> 
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply

* Re: [PATCH v3 7/7] mm, slab: shorten kmalloc cache names for large sizes
From: Christopher Lameter @ 2018-07-30 15:48 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
	Michal Hocko, Johannes Weiner, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox
In-Reply-To: <20180718133620.6205-8-vbabka@suse.cz>

On Wed, 18 Jul 2018, Vlastimil Babka wrote:

> +static const char *
> +kmalloc_cache_name(const char *prefix, unsigned int size)
> +{
> +
> +	static const char units[3] = "\0kM";
> +	int idx = 0;
> +
> +	while (size >= 1024 && (size % 1024 == 0)) {
> +		size /= 1024;
> +		idx++;
> +	}
> +
> +	return kasprintf(GFP_NOWAIT, "%s-%u%c", prefix, size, units[idx]);
> +}

This is likely to occur elsewhere in the kernel. Maybe generalize it a
bit?

Acked-by: Christoph Lameter <cl@linux.com>

^ permalink raw reply

* Re: [PATCH v3 5/7] mm: rename and change semantics of nr_indirectly_reclaimable_bytes
From: Christopher Lameter @ 2018-07-30 15:46 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
	Michal Hocko, Johannes Weiner, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox, Vijayanand Jitta, Laura Abbott,
	Sumit Semwal
In-Reply-To: <20180718133620.6205-6-vbabka@suse.cz>


Acked-by: Christoph Lameter <cl@linux.com>

^ permalink raw reply

* Re: [PATCH v3 3/7] mm, slab: allocate off-slab freelists as reclaimable when appropriate
From: Christopher Lameter @ 2018-07-30 15:45 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
	Michal Hocko, Johannes Weiner, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox
In-Reply-To: <20180718133620.6205-4-vbabka@suse.cz>

On Wed, 18 Jul 2018, Vlastimil Babka wrote:

> In SLAB, OFF_SLAB caches allocate management structures (currently just the
> freelist) from kmalloc caches when placement in a slab page together with
> objects would lead to suboptimal memory usage. For SLAB_RECLAIM_ACCOUNT caches,
> we can allocate the freelists from the newly introduced reclaimable kmalloc
> caches, because shrinking the OFF_SLAB cache will in general result to freeing
> of the freelists as well. This should improve accounting and anti-fragmentation
> a bit.

Acked-by: Christoph Lameter <cl@linux.com>

^ permalink raw reply

* Re: [PATCH v3 2/7] mm, slab/slub: introduce kmalloc-reclaimable caches
From: Christopher Lameter @ 2018-07-30 15:41 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
	Michal Hocko, Johannes Weiner, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox
In-Reply-To: <20180718133620.6205-3-vbabka@suse.cz>

On Wed, 18 Jul 2018, Vlastimil Babka wrote:

> index 4299c59353a1..d89e934e0d8b 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -296,11 +296,12 @@ static inline void __check_heap_object(const void *ptr, unsigned long n,
>                                 (KMALLOC_MIN_SIZE) : 16)
>
>  #define KMALLOC_NORMAL	0
> +#define KMALLOC_RECLAIM	1
>  #ifdef CONFIG_ZONE_DMA
> -#define KMALLOC_DMA	1
> -#define KMALLOC_TYPES	2
> +#define KMALLOC_DMA	2
> +#define KMALLOC_TYPES	3
>  #else
> -#define KMALLOC_TYPES	1
> +#define KMALLOC_TYPES	2
>  #endif

I like enums....

Acked-by: Christoph Lameter <cl@linux.com>

^ permalink raw reply

* Re: [PATCH v3 1/7] mm, slab: combine kmalloc_caches and kmalloc_dma_caches
From: Christopher Lameter @ 2018-07-30 15:38 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, linux-api, Roman Gushchin,
	Michal Hocko, Johannes Weiner, David Rientjes, Joonsoo Kim,
	Mel Gorman, Matthew Wilcox
In-Reply-To: <20180718133620.6205-2-vbabka@suse.cz>

On Wed, 18 Jul 2018, Vlastimil Babka wrote:

> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -295,12 +295,28 @@ static inline void __check_heap_object(const void *ptr, unsigned long n,
>  #define SLAB_OBJ_MIN_SIZE      (KMALLOC_MIN_SIZE < 16 ? \
>                                 (KMALLOC_MIN_SIZE) : 16)
>
> +#define KMALLOC_NORMAL	0
> +#ifdef CONFIG_ZONE_DMA
> +#define KMALLOC_DMA	1
> +#define KMALLOC_TYPES	2
> +#else
> +#define KMALLOC_TYPES	1
> +#endif

An emum would be better here I think.

But the patch is ok

Acked-by: Christoph Lameter <cl@linux.com>

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox