* Re: [PATCH 05/18] Add io_uring IO interface
From: Christoph Hellwig @ 2019-01-28 14:57 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-fsdevel, linux-aio, linux-block, hch, jmoyer, avi,
linux-api, linux-man
In-Reply-To: <20190123153536.7081-6-axboe@kernel.dk>
[please make sure linux-api and linux-man are CCed on new syscalls
so that we get API experts to review them]
> io_uring_enter(fd, to_submit, min_complete, flags)
> Initiates IO against the rings mapped to this fd, or waits for
> them to complete, or both. The behavior is controlled by the
> parameters passed in. If 'to_submit' is non-zero, then we'll
> try and submit new IO. If IORING_ENTER_GETEVENTS is set, the
> kernel will wait for 'min_complete' events, if they aren't
> already available. It's valid to set IORING_ENTER_GETEVENTS
> and 'min_complete' == 0 at the same time, this allows the
> kernel to return already completed events without waiting
> for them. This is useful only for polling, as for IRQ
> driven IO, the application can just check the CQ ring
> without entering the kernel.
Especially with poll support now in the series, don't we need a ѕigmask
argument similar to pselect/ppoll/io_pgetevents now to deal with signal
blocking during waiting for events?
> +struct sqe_submit {
> + const struct io_uring_sqe *sqe;
> + unsigned index;
> +};
Can you make sure all the structs use tab indentation for their
field names? Maybe even the same for all structs just to be nice
to my eyes?
> +static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
> + const struct io_uring_sqe *sqe,
> + struct iovec **iovec, struct iov_iter *iter)
> +{
> + void __user *buf = u64_to_user_ptr(sqe->addr);
> +
> +#ifdef CONFIG_COMPAT
> + if (ctx->compat)
> + return compat_import_iovec(rw, buf, sqe->len, UIO_FASTIOV,
> + iovec, iter);
> +#endif
I think we can just check in_compat_syscall() here, which means we
can kill the ->compat member, and the separate compat version of the
setup syscall.
> +/*
> + * IORING_OP_NOP just posts a completion event, nothing else.
> + */
> +static int io_nop(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> +{
> + struct io_ring_ctx *ctx = req->ctx;
> +
> + __io_cqring_add_event(ctx, sqe->user_data, 0, 0);
Can you explain why not taking the completion lock is safe here? And
why we want to have such a somewhat dangerous special case just for the
no-op benchmarking aid?
> +static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
> +{
> + struct io_sq_ring *ring = ctx->sq_ring;
> + unsigned head;
> +
> + head = ctx->cached_sq_head;
> + smp_rmb();
> + if (head == READ_ONCE(ring->r.tail))
> + return false;
Do we really need to optimize the sq_head == tail case so much? Or
am I missing why we are using the cached sq head case here? Maybe
add some more comments for a start.
> +static int __io_uring_enter(struct io_ring_ctx *ctx, unsigned to_submit,
> + unsigned min_complete, unsigned flags)
> +{
> + int ret = 0;
> +
> + if (to_submit) {
> + ret = io_ring_submit(ctx, to_submit);
> + if (ret < 0)
> + return ret;
> + }
> + if (flags & IORING_ENTER_GETEVENTS) {
> + int get_ret;
> +
> + if (!ret && to_submit)
> + min_complete = 0;
Why do we have this special case? Does it need some documentation?
> +
> + get_ret = io_cqring_wait(ctx, min_complete);
> + if (get_ret < 0 && !ret)
> + ret = get_ret;
> + }
> +
> + return ret;
Maybe using different names and slightly different semantics for the
return values would clear some of this up?
if (to_submit) {
submitted = io_ring_submit(ctx, to_submit);
if (submitted < 0)
return submitted;
}
if (flags & IORING_ENTER_GETEVENTS) {
...
ret = io_cqring_wait(ctx, min_complete);
}
return submitted ? submitted : ret;
> +static int io_sq_offload_start(struct io_ring_ctx *ctx)
> +static void io_sq_offload_stop(struct io_ring_ctx *ctx)
Can we just merge these two functions into the callers? Currently
the flow is a little odd with these helpers that don't seem to be
too clear about their responsibilities.
> +static void io_free_scq_urings(struct io_ring_ctx *ctx)
> +{
> + if (ctx->sq_ring) {
> + page_frag_free(ctx->sq_ring);
> + ctx->sq_ring = NULL;
> + }
> + if (ctx->sq_sqes) {
> + page_frag_free(ctx->sq_sqes);
> + ctx->sq_sqes = NULL;
> + }
> + if (ctx->cq_ring) {
> + page_frag_free(ctx->cq_ring);
> + ctx->cq_ring = NULL;
> + }
Why is this using the page_frag helpers? Also the callers just free
these ctx structure, so there isn't much of a point zeroing them out.
Also I'd be tempted to open code the freeing in io_allocate_scq_urings
instead of caling the helper, which would avoid the NULL checks and
make the error handling code a little more obvious.
> + if (mutex_trylock(&ctx->uring_lock)) {
> + ret = __io_uring_enter(ctx, to_submit, min_complete, flags);
do we even need the separate __io_uring_enter helper?
> +static void io_fill_offsets(struct io_uring_params *p)
Do we really need this as a separate helper?
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCHv5 00/10] Heterogeneuos memory node attributes
From: Michal Hocko @ 2019-01-28 14:00 UTC (permalink / raw)
To: Keith Busch
Cc: linux-kernel, linux-acpi, linux-mm, Greg Kroah-Hartman,
Rafael Wysocki, Dave Hansen, Dan Williams, linux-api
In-Reply-To: <20190124230724.10022-1-keith.busch@intel.com>
Is there any reason why is this not CCing linux-api (Cced now).
On Thu 24-01-19 16:07:14, Keith Busch wrote:
> == Changes since v4 ==
>
> All public interfaces have kernel docs.
>
> Renamed "class" to "access", docs and changed logs updated
> accordingly. (Rafael)
>
> The sysfs hierarchy is altered to put initiators and targets in their
> own attribute group directories (Rafael).
>
> The node lists are removed. This feedback is in conflict with v1
> feedback, but consensus wants to remove multi-value sysfs attributes,
> which includes lists. We only have symlinks now, just like v1 provided.
>
> Documentation and code patches are combined such that the code
> introducing new attributes and its documentation are in the same
> patch. (Rafael and Dan).
>
> The performance attributes, bandwidth and latency, are moved into the
> initiators directory. This should make it obvious for which node
> access the attributes apply, which was previously ambiguous.
> (Jonathan Cameron).
>
> The HMAT code selecting "local" initiators is substantially changed.
> Only PXM's that have identical performance to the HMAT's processor PXM
> in Address Range Structure are registered. This is to avoid considering
> nodes identical when only one of several perf attributes are the same.
> (Jonathan Cameron).
>
> Verbose variable naming. Examples include "initiator" and "target"
> instead of "i" and "t", "mem_pxm" and "cpu_pxm" instead of "m" and
> "p". (Rafael)
>
> Compile fixes for when HMEM_REPORTING is not set. This is not a user
> selectable config option, default 'n', and will have to be selected
> by other config options that require it (Greg KH and Rafael).
>
> == Background ==
>
> Platforms may provide multiple types of cpu attached system memory. The
> memory ranges for each type may have different characteristics that
> applications may wish to know about when considering what node they want
> their memory allocated from.
>
> It had previously been difficult to describe these setups as memory
> rangers were generally lumped into the NUMA node of the CPUs. New
> platform attributes have been created and in use today that describe
> the more complex memory hierarchies that can be created.
>
> This series' objective is to provide the attributes from such systems
> that are useful for applications to know about, and readily usable with
> existing tools and libraries.
Can you provide a highlevel description of these new attributes and how
they are supposed to be used.
Mentioning usecases is also due consideirng the amount of code this
adds.
> Keith Busch (10):
> acpi: Create subtable parsing infrastructure
> acpi: Add HMAT to generic parsing tables
> acpi/hmat: Parse and report heterogeneous memory
> node: Link memory nodes to their compute nodes
> acpi/hmat: Register processor domain to its memory
> node: Add heterogenous memory access attributes
> acpi/hmat: Register performance attributes
> node: Add memory caching attributes
> acpi/hmat: Register memory side cache attributes
> doc/mm: New documentation for memory performance
>
> Documentation/ABI/stable/sysfs-devices-node | 87 ++++-
> Documentation/admin-guide/mm/numaperf.rst | 167 ++++++++
> arch/arm64/kernel/acpi_numa.c | 2 +-
> arch/arm64/kernel/smp.c | 4 +-
> arch/ia64/kernel/acpi.c | 12 +-
> arch/x86/kernel/acpi/boot.c | 36 +-
> drivers/acpi/Kconfig | 1 +
> drivers/acpi/Makefile | 1 +
> drivers/acpi/hmat/Kconfig | 9 +
> drivers/acpi/hmat/Makefile | 1 +
> drivers/acpi/hmat/hmat.c | 537 ++++++++++++++++++++++++++
> drivers/acpi/numa.c | 16 +-
> drivers/acpi/scan.c | 4 +-
> drivers/acpi/tables.c | 76 +++-
> drivers/base/Kconfig | 8 +
> drivers/base/node.c | 354 ++++++++++++++++-
> drivers/irqchip/irq-gic-v2m.c | 2 +-
> drivers/irqchip/irq-gic-v3-its-pci-msi.c | 2 +-
> drivers/irqchip/irq-gic-v3-its-platform-msi.c | 2 +-
> drivers/irqchip/irq-gic-v3-its.c | 6 +-
> drivers/irqchip/irq-gic-v3.c | 10 +-
> drivers/irqchip/irq-gic.c | 4 +-
> drivers/mailbox/pcc.c | 2 +-
> include/linux/acpi.h | 6 +-
> include/linux/node.h | 60 ++-
> 25 files changed, 1344 insertions(+), 65 deletions(-)
> create mode 100644 Documentation/admin-guide/mm/numaperf.rst
> create mode 100644 drivers/acpi/hmat/Kconfig
> create mode 100644 drivers/acpi/hmat/Makefile
> create mode 100644 drivers/acpi/hmat/hmat.c
>
> --
> 2.14.4
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Cyril Hrubis @ 2019-01-28 13:49 UTC (permalink / raw)
To: Jiri Kosina
Cc: Matthew Wilcox, Linus Torvalds, Dominique Martinet,
Andy Lutomirski, Josh Snyder, Dave Chinner, Jann Horn,
Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, Linux-MM,
kernel list, Linux API
In-Reply-To: <nycvar.YFH.7.76.1901162238310.6626@cbobk.fhfr.pm>
Hi!
> > Of course, there aren't any tests for RWF_NOWAIT in xfstests. Are there
> > any in LTP?
Just FYI I've send a patch with basic RWF_NOWAIT test for review to LTP
ML and also CCed mailing lists from this thread.
https://lkml.org/lkml/2019/1/28/416
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply
* [PATCH] syscalls/preadv203: Add basic RWF_NOWAIT test
From: Cyril Hrubis @ 2019-01-28 13:46 UTC (permalink / raw)
To: ltp; +Cc: Cyril Hrubis, Jiri Kosina, Linux-MM, kernel list, Linux API
From: Cyril Hrubis <chrubis@suse.cz>
We are attempting to trigger the EAGAIN path for the RWF_NOWAIT flag.
In order to do so the test runs three threads:
* nowait_reader: reads from a random offset from a random file with
RWF_NOWAIT flag and expects to get EAGAIN and short
read sooner or later
* writer_thread: rewrites random file in order to keep the underlying device
bussy so that pages evicted from cache cannot be faulted
immediatelly
* cache_dropper: attempts to evict pages from a cache in order for reader to
hit evicted page sooner or later
Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
CC: Jiri Kosina <jikos@kernel.org>
CC: Linux-MM <linux-mm@kvack.org>
CC: kernel list <linux-kernel@vger.kernel.org>
CC: Linux API <linux-api@vger.kernel.org>
---
I was wondering if we can do a better job at flushing the caches. Is
there an interface for flusing caches just for the device we are using
for the test?
Also the RWF_NOWAIT should probably be benchmarked as well but that is
completely out of scope for LTP.
runtest/syscalls | 2 +
testcases/kernel/syscalls/preadv2/.gitignore | 2 +
testcases/kernel/syscalls/preadv2/Makefile | 4 +
testcases/kernel/syscalls/preadv2/preadv203.c | 266 ++++++++++++++++++
4 files changed, 274 insertions(+)
create mode 100644 testcases/kernel/syscalls/preadv2/preadv203.c
diff --git a/runtest/syscalls b/runtest/syscalls
index 34b47f36b..a69c431f1 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -853,6 +853,8 @@ preadv201 preadv201
preadv201_64 preadv201_64
preadv202 preadv202
preadv202_64 preadv202_64
+preadv203 preadv203
+preadv203_64 preadv203_64
profil01 profil01
diff --git a/testcases/kernel/syscalls/preadv2/.gitignore b/testcases/kernel/syscalls/preadv2/.gitignore
index 759d9ef5b..98b81abea 100644
--- a/testcases/kernel/syscalls/preadv2/.gitignore
+++ b/testcases/kernel/syscalls/preadv2/.gitignore
@@ -2,3 +2,5 @@
/preadv201_64
/preadv202
/preadv202_64
+/preadv203
+/preadv203_64
diff --git a/testcases/kernel/syscalls/preadv2/Makefile b/testcases/kernel/syscalls/preadv2/Makefile
index fc1fbf3c7..fbedd0287 100644
--- a/testcases/kernel/syscalls/preadv2/Makefile
+++ b/testcases/kernel/syscalls/preadv2/Makefile
@@ -11,4 +11,8 @@ include $(abs_srcdir)/../utils/newer_64.mk
%_64: CPPFLAGS += -D_FILE_OFFSET_BITS=64
+preadv203: CFLAGS += -pthread
+preadv203_64: CFLAGS += -pthread
+preadv203_64: LDFLAGS += -pthread
+
include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/preadv2/preadv203.c b/testcases/kernel/syscalls/preadv2/preadv203.c
new file mode 100644
index 000000000..a6d5300f9
--- /dev/null
+++ b/testcases/kernel/syscalls/preadv2/preadv203.c
@@ -0,0 +1,266 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2019 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+/*
+ * This is a basic functional test for RWF_NOWAIT flag, we are attempting to
+ * force preadv2() either to return a short read or EAGAIN with three
+ * concurelntly running threads:
+ *
+ * nowait_reader: reads from a random offset from a random file with
+ * RWF_NOWAIT flag and expects to get EAGAIN and short
+ * read sooner or later
+ *
+ * writer_thread: rewrites random file in order to keep the underlying device
+ * bussy so that pages evicted from cache cannot be faulted
+ * immediatelly
+ *
+ * cache_dropper: attempts to evict pages from a cache in order for reader to
+ * hit evicted page sooner or later
+ */
+
+/*
+ * If test fails with EOPNOTSUPP you have likely hit a glibc bug:
+ *
+ * https://sourceware.org/bugzilla/show_bug.cgi?id=23579
+ *
+ * Which can be worked around by calling preadv2() directly by syscall() such as:
+ *
+ * static ssize_t sys_preadv2(int fd, const struct iovec *iov, int iovcnt,
+ * off_t offset, int flags)
+ * {
+ * return syscall(SYS_preadv2, fd, iov, iovcnt, offset, offset>>32, flags);
+ * }
+ *
+ */
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <sys/uio.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <ctype.h>
+#include <pthread.h>
+
+#include "tst_test.h"
+#include "tst_safe_pthread.h"
+#include "lapi/preadv2.h"
+
+#define CHUNK_SZ 4123
+#define CHUNKS 60
+#define MNTPOINT "mntpoint"
+#define FILES 1000
+
+static int fds[FILES];
+
+static volatile int stop;
+
+static void drop_caches(void)
+{
+ SAFE_FILE_PRINTF("/proc/sys/vm/drop_caches", "3");
+}
+
+/*
+ * All files are divided in chunks each filled with the same bytes starting with
+ * '0' at offset 0 and with increasing value on each next chunk.
+ *
+ * 000....000111....111.......AAA......AAA...
+ * | chunk0 || chunk1 | ... | chunk17 |
+ */
+static int verify_short_read(struct iovec *iov, size_t iov_cnt,
+ off_t off, size_t size)
+{
+ unsigned int i;
+ size_t j, checked = 0;
+
+ for (i = 0; i < iov_cnt; i++) {
+ char *buf = iov[i].iov_base;
+ for (j = 0; j < iov[i].iov_len; j++) {
+ char exp_val = '0' + (off + checked)/CHUNK_SZ;
+
+ if (exp_val != buf[j]) {
+ tst_res(TFAIL,
+ "Wrong value read pos %zu size %zu %c (%i) %c (%i)!",
+ checked, size, exp_val, exp_val,
+ isprint(buf[j]) ? buf[j] : ' ', buf[j]);
+ return 1;
+ }
+
+ if (++checked >= size)
+ return 0;
+ }
+ }
+
+ return 0;
+}
+
+static void *nowait_reader(void *unused LTP_ATTRIBUTE_UNUSED)
+{
+ char buf1[CHUNK_SZ/2];
+ char buf2[CHUNK_SZ];
+ unsigned int full_read_cnt = 0, eagain_cnt = 0;
+ unsigned int short_read_cnt = 0, zero_read_cnt = 0;
+
+ struct iovec rd_iovec[] = {
+ {buf1, sizeof(buf1)},
+ {buf2, sizeof(buf2)},
+ };
+
+ while (!stop) {
+ if (eagain_cnt >= 100 && short_read_cnt >= 10)
+ stop = 1;
+
+ /* Ensure short reads doesn't happen because of tripping on EOF */
+ off_t off = random() % ((CHUNKS - 2) * CHUNK_SZ);
+ int fd = fds[random() % FILES];
+
+ TEST(preadv2(fd, rd_iovec, 2, off, RWF_NOWAIT));
+
+ if (TST_RET < 0) {
+ if (TST_ERR != EAGAIN)
+ tst_brk(TBROK | TTERRNO, "preadv2() failed");
+
+ eagain_cnt++;
+ continue;
+ }
+
+
+ if (TST_RET == 0) {
+ zero_read_cnt++;
+ continue;
+ }
+
+ if (TST_RET != CHUNK_SZ + CHUNK_SZ/2) {
+ verify_short_read(rd_iovec, 2, off, TST_RET);
+ short_read_cnt++;
+ continue;
+ }
+
+ full_read_cnt++;
+ }
+
+ tst_res(TINFO,
+ "Number of full_reads %u, short reads %u, zero len reads %u, EAGAIN(s) %u",
+ full_read_cnt, short_read_cnt, zero_read_cnt, eagain_cnt);
+
+ return (void*)(long)eagain_cnt;
+}
+
+static void *writer_thread(void *unused)
+{
+ char buf[CHUNK_SZ];
+ unsigned int j, write_cnt = 0;
+
+ struct iovec wr_iovec[] = {
+ {buf, sizeof(buf)},
+ };
+
+ while (!stop) {
+ int fd = fds[random() % FILES];
+
+ for (j = 0; j < CHUNKS; j++) {
+ memset(buf, '0' + j, sizeof(buf));
+
+ off_t off = CHUNK_SZ * j;
+
+ if (pwritev(fd, wr_iovec, 1, off) < 0) {
+ if (errno == EBADF) {
+ tst_res(TBROK | TERRNO, "FDs closed?");
+ return unused;
+ }
+
+ tst_brk(TBROK | TERRNO, "pwritev()");
+ }
+
+ write_cnt++;
+ }
+ }
+
+ tst_res(TINFO, "Number of writes %u", write_cnt);
+
+ return unused;
+}
+
+static void *cache_dropper(void *unused)
+{
+ unsigned int drop_cnt = 0;
+
+ while (!stop) {
+ drop_caches();
+ drop_cnt++;
+ }
+
+ tst_res(TINFO, "Cache dropped %u times", drop_cnt);
+
+ return unused;
+}
+
+static void verify_preadv2(void)
+{
+ pthread_t reader, dropper, writer;
+ unsigned int max_runtime = 600;
+ void *eagains;
+
+ stop = 0;
+
+ drop_caches();
+
+ SAFE_PTHREAD_CREATE(&dropper, NULL, cache_dropper, NULL);
+ SAFE_PTHREAD_CREATE(&reader, NULL, nowait_reader, NULL);
+ SAFE_PTHREAD_CREATE(&writer, NULL, writer_thread, NULL);
+
+ while (!stop && max_runtime-- > 0)
+ usleep(100000);
+
+ stop = 1;
+
+ SAFE_PTHREAD_JOIN(reader, &eagains);
+ SAFE_PTHREAD_JOIN(dropper, NULL);
+ SAFE_PTHREAD_JOIN(writer, NULL);
+
+ if (eagains)
+ tst_res(TPASS, "Got some EAGAIN");
+ else
+ tst_res(TFAIL, "Haven't got EAGAIN");
+}
+
+static void setup(void)
+{
+ char path[1024];
+ char buf[CHUNK_SZ];
+ unsigned int i;
+ char j;
+
+ for (i = 0; i < FILES; i++) {
+ snprintf(path, sizeof(path), MNTPOINT"/file_%i", i);
+
+ fds[i] = SAFE_OPEN(path, O_RDWR | O_CREAT, 0644);
+
+ for (j = 0; j < CHUNKS; j++) {
+ memset(buf, '0' + j, sizeof(buf));
+ SAFE_WRITE(1, fds[i], buf, sizeof(buf));
+ }
+ }
+}
+
+static void do_cleanup(void)
+{
+ unsigned int i;
+
+ for (i = 0; i < FILES; i++) {
+ if (fds[i] > 0)
+ SAFE_CLOSE(fds[i]);
+ }
+}
+
+TST_DECLARE_ONCE_FN(cleanup, do_cleanup);
+
+static struct tst_test test = {
+ .setup = setup,
+ .cleanup = cleanup,
+ .test_all = verify_preadv2,
+ .mntpoint = MNTPOINT,
+ .all_filesystems = 1,
+ .needs_tmpdir = 1,
+};
--
2.19.2
^ permalink raw reply related
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dominique Martinet @ 2019-01-28 0:05 UTC (permalink / raw)
To: Jiri Kosina
Cc: Linus Torvalds, Andy Lutomirski, Josh Snyder, Dave Chinner,
Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <nycvar.YFH.7.76.1901272335040.6626@cbobk.fhfr.pm>
Jiri Kosina wrote on Sun, Jan 27, 2019:
> So, any objections to aproaching it this way?
I'm not sure why I'm the main recipient of that mail but answering
because I am -- let's get these patches in through the regular -mm tree
though
--
Dominique
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-27 22:35 UTC (permalink / raw)
To: Dominique Martinet
Cc: Linus Torvalds, Andy Lutomirski, Josh Snyder, Dave Chinner,
Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <nycvar.YFH.7.76.1901241523500.6626@cbobk.fhfr.pm>
On Thu, 24 Jan 2019, Jiri Kosina wrote:
> > Jiri, you've offered resubmitting the last two patches properly, can you
> > incorporate this change or should I just send this directly? (I'd take
> > most of your commit message and add your name somewhere)
>
> I've been running some basic smoke testing with the kernel from
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jikos/jikos.git/log/?h=pagecache-sidechannel-v2
>
> (attaching the respective two patches to apply on top of latest Linus'
> tree to this mail as well), and everything looks good so far.
So, any objections to aproaching it this way?
I've not been able to spot any obvious breakage so far with it.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 07/29] ARM: add kexec_file_load system call number
From: Russell King - ARM Linux admin @ 2019-01-25 16:21 UTC (permalink / raw)
To: Catalin Marinas
Cc: Arnd Bergmann, y2038, linux-api, linux-kernel, linux-arch,
mattst88, will.deacon, tony.luck, fenghua.yu, geert, monstr,
paul.burton, deller, benh, mpe, schwidefsky, heiko.carstens,
dalias, davem, luto, tglx, mingo, hpa, x86, jcmvbkbc, akpm,
deepa.kernel, ebiederm, firoz.khan, linux-alpha, linux-arm-kernel,
linux-ia64, linux-m68k, linux-mips
In-Reply-To: <20190125154359.GG25901@arrakis.emea.arm.com>
On Fri, Jan 25, 2019 at 03:43:59PM +0000, Catalin Marinas wrote:
> On Fri, Jan 18, 2019 at 05:18:13PM +0100, Arnd Bergmann wrote:
> > diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> > index 86de9eb34296..20ed7e026723 100644
> > --- a/arch/arm/tools/syscall.tbl
> > +++ b/arch/arm/tools/syscall.tbl
> > @@ -415,3 +415,4 @@
> > 398 common rseq sys_rseq
> > 399 common io_pgetevents sys_io_pgetevents
> > 400 common migrate_pages sys_migrate_pages
> > +401 common kexec_file_load sys_kexec_file_load
>
> I presume on arm32 this would still return -ENOSYS.
Yes, I already checked. If CONFIG_KEXEC_FILE is not set, then
kernel/kexec_file.c is not built, and we fall back to the stub
in kernel/sys_ni.c.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH v2 29/29] y2038: add 64-bit time_t syscalls to all 32-bit architectures
From: Catalin Marinas @ 2019-01-25 15:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: y2038, linux-api, linux-kernel, linux-arch, mattst88, linux,
will.deacon, tony.luck, fenghua.yu, geert, monstr, paul.burton,
deller, benh, mpe, schwidefsky, heiko.carstens, dalias, davem,
luto, tglx, mingo, hpa, x86, jcmvbkbc, akpm, deepa.kernel,
ebiederm, firoz.khan, linux-alpha, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips
In-Reply-To: <20190118161835.2259170-30-arnd@arndb.de>
On Fri, Jan 18, 2019 at 05:18:35PM +0100, Arnd Bergmann wrote:
> This adds 21 new system calls on each ABI that has 32-bit time_t
> today. All of these have the exact same semantics as their existing
> counterparts, and the new ones all have macro names that end in 'time64'
> for clarification.
>
> This gets us to the point of being able to safely use a C library
> that has 64-bit time_t in user space. There are still a couple of
> loose ends to tie up in various areas of the code, but this is the
> big one, and should be entirely uncontroversial at this point.
>
> In particular, there are four system calls (getitimer, setitimer,
> waitid, and getrusage) that don't have a 64-bit counterpart yet,
> but these can all be safely implemented in the C library by wrapping
> around the existing system calls because the 32-bit time_t they
> pass only counts elapsed time, not time since the epoch. They
> will be dealt with later.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
(as long as compat follows the arm32 syscall numbers)
^ permalink raw reply
* Re: [PATCH v2 25/29] y2038: syscalls: rename y2038 compat syscalls
From: Catalin Marinas @ 2019-01-25 15:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: y2038, linux-api, linux-kernel, linux-arch, mattst88, linux,
will.deacon, tony.luck, fenghua.yu, geert, monstr, paul.burton,
deller, benh, mpe, schwidefsky, heiko.carstens, dalias, davem,
luto, tglx, mingo, hpa, x86, jcmvbkbc, akpm, deepa.kernel,
ebiederm, firoz.khan, linux-alpha, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips
In-Reply-To: <20190118161835.2259170-26-arnd@arndb.de>
On Fri, Jan 18, 2019 at 05:18:31PM +0100, Arnd Bergmann wrote:
> A lot of system calls that pass a time_t somewhere have an implementation
> using a COMPAT_SYSCALL_DEFINEx() on 64-bit architectures, and have
> been reworked so that this implementation can now be used on 32-bit
> architectures as well.
>
> The missing step is to redefine them using the regular SYSCALL_DEFINEx()
> to get them out of the compat namespace and make it possible to build them
> on 32-bit architectures.
>
> Any system call that ends in 'time' gets a '32' suffix on its name for
> that version, while the others get a '_time32' suffix, to distinguish
> them from the normal version, which takes a 64-bit time argument in the
> future.
>
> In this step, only 64-bit architectures are changed, doing this rename
> first lets us avoid touching the 32-bit architectures twice.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
For arm64:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply
* Re: [PATCH v2 07/29] ARM: add kexec_file_load system call number
From: Catalin Marinas @ 2019-01-25 15:43 UTC (permalink / raw)
To: Arnd Bergmann
Cc: y2038, linux-api, linux-kernel, linux-arch, mattst88, linux,
will.deacon, tony.luck, fenghua.yu, geert, monstr, paul.burton,
deller, benh, mpe, schwidefsky, heiko.carstens, dalias, davem,
luto, tglx, mingo, hpa, x86, jcmvbkbc, akpm, deepa.kernel,
ebiederm, firoz.khan, linux-alpha, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips
In-Reply-To: <20190118161835.2259170-8-arnd@arndb.de>
On Fri, Jan 18, 2019 at 05:18:13PM +0100, Arnd Bergmann wrote:
> diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> index 86de9eb34296..20ed7e026723 100644
> --- a/arch/arm/tools/syscall.tbl
> +++ b/arch/arm/tools/syscall.tbl
> @@ -415,3 +415,4 @@
> 398 common rseq sys_rseq
> 399 common io_pgetevents sys_io_pgetevents
> 400 common migrate_pages sys_migrate_pages
> +401 common kexec_file_load sys_kexec_file_load
I presume on arm32 this would still return -ENOSYS.
> diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
> index 261216c3336e..2c30e6f145ff 100644
> --- a/arch/arm64/include/asm/unistd.h
> +++ b/arch/arm64/include/asm/unistd.h
> @@ -44,7 +44,7 @@
> #define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)
> #define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800)
>
> -#define __NR_compat_syscalls 401
> +#define __NR_compat_syscalls 402
> #endif
>
> #define __ARCH_WANT_SYS_CLONE
> diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
> index f15bcbacb8f6..8ca1d4c304f4 100644
> --- a/arch/arm64/include/asm/unistd32.h
> +++ b/arch/arm64/include/asm/unistd32.h
> @@ -823,6 +823,8 @@ __SYSCALL(__NR_rseq, sys_rseq)
> __SYSCALL(__NR_io_pgetevents, compat_sys_io_pgetevents)
> #define __NR_migrate_pages 400
> __SYSCALL(__NR_migrate_pages, compat_sys_migrate_pages)
> +#define __NR_kexec_file_load 401
> +__SYSCALL(__NR_kexec_file_load, sys_kexec_file_load)
For arm64:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply
* Re: [PATCH v2 06/29] ARM: add migrate_pages() system call
From: Catalin Marinas @ 2019-01-25 15:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: y2038, linux-api, linux-kernel, linux-arch, mattst88, linux,
will.deacon, tony.luck, fenghua.yu, geert, monstr, paul.burton,
deller, benh, mpe, schwidefsky, heiko.carstens, dalias, davem,
luto, tglx, mingo, hpa, x86, jcmvbkbc, akpm, deepa.kernel,
ebiederm, firoz.khan, linux-alpha, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips
In-Reply-To: <20190118161835.2259170-7-arnd@arndb.de>
On Fri, Jan 18, 2019 at 05:18:12PM +0100, Arnd Bergmann wrote:
> The migrate_pages system call has an assigned number on all architectures
> except ARM. When it got added initially in commit d80ade7b3231 ("ARM:
> Fix warning: #warning syscall migrate_pages not implemented"), it was
> intentionally left out based on the observation that there are no 32-bit
> ARM NUMA systems.
>
> However, there are now arm64 NUMA machines that can in theory run 32-bit
> kernels (actually enabling NUMA there would require additional work)
> as well as 32-bit user space on 64-bit kernels, so that argument is no
> longer very strong.
>
> Assigning the number lets us use the system call on 64-bit kernels as well
> as providing a more consistent set of syscalls across architectures.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
For the arm64 part:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply
* Re: [PATCH v6 01/16] sched/core: Allow sched_setattr() to use the current policy
From: Alessio Balsini @ 2019-01-25 13:56 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Quentin Perret, Dietmar Eggemann, Morten Rasmussen,
Juri Lelli, Todd Kjos, Joel Fernandes, Steve Muckle,
Suren Baghdasaryan
In-Reply-To: <20190115101513.2822-2-patrick.bellasi@arm.com>
Hello Patrick,
What do you think about the following minor changes:
On Tue, Jan 15, 2019 at 10:14:58AM +0000, Patrick Bellasi wrote:
> /* SCHED_ISO: reserved but not implemented yet */
> #define SCHED_IDLE 5
> #define SCHED_DEADLINE 6
> +/* Must be the last entry: used to sanity check attr.policy values */
I would remove this comment:
- the meaning of SCHED_POLICY_MAX is evident, and
- should we hint on how the value is used? That comment will be removed
the next time SCHED_POLICY_MAX is used for something else.
This is what should also happen to the comment of SETPARAM_POLICY:
now sched_setparam() is no more the only code path accessing
SETPARAM_POLICY.
> +#define SCHED_POLICY_MAX 7
+#define SCHED_POLICY_MAX SCHED_DEADLINE
This would make it compliant with the definition of MAX.
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4560,8 +4560,17 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
> if (retval)
> return retval;
>
> - if ((int)attr.sched_policy < 0)
> + /*
> + * A valid policy is always required from userspace, unless
> + * SCHED_FLAG_KEEP_POLICY is set and the current policy
> + * is enforced for this call.
> + */
> + if (attr.sched_policy >= SCHED_POLICY_MAX &&
+ if (attr.sched_policy > SCHED_POLICY_MAX &&
In line with the previous update.
> + !(attr.sched_flags & SCHED_FLAG_KEEP_POLICY)) {
> return -EINVAL;
Thanks,
Alessio
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Richard Guy Briggs @ 2019-01-24 20:36 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, linux-audit, linux-kernel, ebiederm, luto,
dhowells, viro, simo, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhRU-Qss41CnV3mE32tXmZ3FQpy++t4yPUkh9CjuAzxYCA@mail.gmail.com>
On 2019-01-03 15:10, Paul Moore wrote:
> On Thu, Nov 1, 2018 at 6:07 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-10-19 19:15, Paul Moore wrote:
> > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > The audit-related parameters in struct task_struct should ideally be
> > > > collected together and accessed through a standard audit API.
> > > >
> > > > Collect the existing loginuid, sessionid and audit_context together in a
> > > > new struct audit_task_info called "audit" in struct task_struct.
> > > >
> > > > Use kmem_cache to manage this pool of memory.
> > > > Un-inline audit_free() to be able to always recover that memory.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/81
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> > > > include/linux/sched.h | 5 +----
> > > > init/init_task.c | 3 +--
> > > > init/main.c | 2 ++
> > > > kernel/auditsc.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
> > > > kernel/fork.c | 4 +++-
> > > > 6 files changed, 73 insertions(+), 26 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > index 87bf02d..e117272 100644
> > > > --- a/include/linux/sched.h
> > > > +++ b/include/linux/sched.h
> > > > @@ -873,10 +872,8 @@ struct task_struct {
> > > >
> > > > struct callback_head *task_works;
> > > >
> > > > - struct audit_context *audit_context;
> > > > #ifdef CONFIG_AUDITSYSCALL
> > > > - kuid_t loginuid;
> > > > - unsigned int sessionid;
> > > > + struct audit_task_info *audit;
> > > > #endif
> > > > struct seccomp seccomp;
> > >
> > > Prior to this patch audit_context was available regardless of
> > > CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context
> > > is only available when CONFIG_AUDITSYSCALL is defined.
> >
> > This was intentional since audit_context is not used when AUDITSYSCALL is
> > disabled. audit_alloc() was stubbed in that case to return 0. audit_context()
> > returned NULL.
> >
> > The fact that audit_context was still present in struct task_struct was an
> > oversight in the two patches already accepted:
> > ("audit: use inline function to get audit context")
> > ("audit: use inline function to get audit context")
> > that failed to hide or remove it from struct task_struct when it was no longer
> > relevant.
>
> Okay, in that case let's pull this out and fix this separately from
> the audit container ID patchset.
Ok, that should be addressed by ghak104.
> > On further digging, loginuid and sessionid (and audit_log_session_info) should
> > be part of CONFIG_AUDIT scope and not CONFIG_AUDITSYSCALL since it is used in
> > CONFIG_CHANGE, ANOM_LINK, FEATURE_CHANGE(, INTEGRITY_RULE), none of which are
> > otherwise dependent on AUDITSYSCALL.
>
> This looks like something else we should fix independently from this patchset.
Ok, this should be addressed by ghak105.
> > Looking ahead, contid should be treated like loginuid and sessionid, which are
> > currently only available when syscall auditting is.
>
> That seems reasonable. Eventually it would be great if we got rid of
> CONFIG_AUDITSYSCALL, but that is a separate issue, and something that
> is going to require work from the different arch/ABI folks to ensure
> everything is working properly.
So I'll plan to rebase on ghak104 and ghak105 once they are upstreamed.
I'll address the locking issues in the netns list and audit_sig_cid...
> > Converting records from standalone to syscall and checking audit_dummy_context
> > changes the nature of CONFIG_AUDIT/!CONFIG_AUDITSYSCALL separation.
> > eg: ANOM_LINK accompanied by PATH record (which needed CWD addition to be
> > complete anyways)
This has been addressed in ghak105, moving ANOM_LINK to auditsc.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Patrick Bellasi @ 2019-01-24 16:14 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190124153106.GQ13777@hirez.programming.kicks-ass.net>
On 24-Jan 16:31, Peter Zijlstra wrote:
> On Thu, Jan 24, 2019 at 12:30:09PM +0000, Patrick Bellasi wrote:
> > > So I'll have to go over the code again, but I'm wondering why you're
> > > changing uclamp_se::bucket_id on a runnable task.
> >
> > We change only the "requested" value, not the "effective" one.
> >
> > > Ideally you keep bucket_id invariant between enqueue and dequeue; then
> > > dequeue knows where we put it.
> >
> > Right, that's what we do for the "effective" value.
>
> So the problem I have is that you first introduce uclamp_se::value and
> use that all over the code, and then introduce effective and change all
> the usage sites.
Right, because the moment we introduce the combination/aggregation mechanism
is the moment "effective" value makes sense to have. That's when the
code show that:
a task cannot always get what it "request", an "effective" value is
computed by aggregation and that's the value we use now for actual
clamp enforcing.
> That seems daft. Why not keep all the code as-is and add orig_value.
If you prefer, I can use effective values since the beginning and then
add the "requested" values later... but I fear the patchset will not
be more clear to parse.
> > > Now I suppose actually determining bucket_id is 'expensive' (it
> > > certainly is with the whole mapping scheme, but even that integer
> > > division is not nice), so we'd like to precompute the bucket_id.
> >
> > Yes, although the complexity is mostly in the composition logic
> > described above not on mapping at all. We have "mapping" overheads
> > only when we change a "request" value and that's from slow-paths.
>
> It's weird though. Esp. when combined with that mapping logic, because
> then you get to use additional maps that are not in fact ever used.
Mmm... don't get this point...
AFAICS "mapping" and "effective" are two different concepts, that's
why I can probably get rid of the first by I would prefer to keep the
second.
> > > We can update uclamp_se::value and set uclamp_se::changed, and then the
> > > next enqueue will (unlikely) test-and-clear changed and recompute the
> > > bucket_id.
> >
> > This mean will lazy update the "requested" bucket_id by deferring its
> > computation at enqueue time. Which saves us a copy of the bucket_id,
> > i.e. we will have only the "effective" value updated at enqueue time.
> >
> > But...
> >
> > > Would that not be simpler?
> >
> > ... although being simpler it does not fully exploit the slow-path,
> > a syscall which is usually running from a different process context
> > (system management software).
> >
> > It also fits better for lazy updates but, in the cgroup case, where we
> > wanna enforce an update ASAP for RUNNABLE tasks, we will still have to
> > do the updates from the slow-path.
> >
> > Will look better into this simplification while working on v7, perhaps
> > the linear mapping can really help in that too.
>
> OK. So mostly my complaint is that it seems to do things odd for ill
> explained reasons.
:( I'm really sorry I'm not able to convey the overall design idea.
TBH however, despite being a quite "limited" feature it has many
different viewpoints: task-specific, cgroups and system defaults.
I've really tried my best to come up with something reasonable but I
understand that, looking at the single patches, the overall design
could be difficult to grasp... without considering that optimizations
are always possible of course.
If you like better, I can try to give a respin by just removing the
mapping part and then we go back and see if the reaming bits makes
more sense ?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Patrick Bellasi @ 2019-01-24 16:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190124151559.GP13777@hirez.programming.kicks-ass.net>
On 24-Jan 16:15, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> > Add uclamp_default_perf, a special set of clamp values to be used
> > for tasks requiring maximum performance, i.e. by default all the non
> > clamped RT tasks.
>
> Urgh... why though?
Because tasks without a task-specific clamp value assigned are subject
to system default... but those, by default, don't clamp at all:
uclamp_default = [0..1024]
For RT tasks however, we want to execute then always at max OPP, if
not otherwise requested (e.g. task-specific value). Thus we need a
different set of default clamps:
uclamp_perf_default = [1024..1024]
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Patrick Bellasi @ 2019-01-24 16:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190124151212.GO13777@hirez.programming.kicks-ass.net>
On 24-Jan 16:12, Peter Zijlstra wrote:
> On Thu, Jan 24, 2019 at 12:38:35PM +0000, Patrick Bellasi wrote:
> > On 24-Jan 12:30, Patrick Bellasi wrote:
> > > On 23-Jan 21:11, Peter Zijlstra wrote:
> > > > On Wed, Jan 23, 2019 at 02:40:11PM +0000, Patrick Bellasi wrote:
> > > > > On 23-Jan 11:49, Peter Zijlstra wrote:
> > > > > > On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> >
> > [...]
> >
> > > > I'm thikning that if we haz a single bit, say:
> > > >
> > > > struct uclamp_se {
> > > > ...
> > > > unsigned int changed : 1;
> > > > };
> > > >
> > > > We can update uclamp_se::value and set uclamp_se::changed, and then the
> > > > next enqueue will (unlikely) test-and-clear changed and recompute the
> > > > bucket_id.
> > >
> > > This mean will lazy update the "requested" bucket_id by deferring its
> > > computation at enqueue time. Which saves us a copy of the bucket_id,
> > > i.e. we will have only the "effective" value updated at enqueue time.
> > >
> > > But...
> > >
> > > > Would that not be simpler?
> > >
> > > ... although being simpler it does not fully exploit the slow-path,
> > > a syscall which is usually running from a different process context
> > > (system management software).
> > >
> > > It also fits better for lazy updates but, in the cgroup case, where we
> > > wanna enforce an update ASAP for RUNNABLE tasks, we will still have to
> > > do the updates from the slow-path.
> > >
> > > Will look better into this simplification while working on v7, perhaps
> > > the linear mapping can really help in that too.
> >
> > Actually, I forgot to mention that:
> >
> > uclamp_se::effective::{
> > value, bucket_id
> > }
> >
> > will be still required to proper support the cgroup delegation model,
> > where a child group could be restricted by the parent but we want to
> > keep track of the original "requested" value for when the parent
> > should relax the restriction.
> >
> > Thus, since effective values are already there, why not using them
> > also to pre-compute the new requested bucket_id from the slow path?
>
> Well, we need the orig_value; but I'm still not sure why you need more
> bucket_id's. Also, retaining orig_value is already required for the
> system limits, there's nothing cgroup-y about this afaict.
Sure, the "effective" values are just a very convenient way (IMHO) to
know exactly which value/bucket_id is currently in use by a task while
keeping them well separated from the "requested" values.
So, you propose to add "orig_value"... but the end effect will be the
same... it's just that if we look at uclamp_se you have two dual
information:
A) whatever a task or cgroup "request" is always in:
uclamp_se::value
uclamp_se::bucket_id
B) whatever a task or cgroup "gets" is always in:
uclamp_se::effective::value
uclamp_se::effective::bucket_id
I find this partitioning useful and easy to use:
1) the slow-path updates only data in A
2) the fast-path updates only data in B
by composing A data in uclamp_effective_get() @enqueue time.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Peter Zijlstra @ 2019-01-24 15:33 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190124123009.2yulcf25ld66popd@e110439-lin>
On Thu, Jan 24, 2019 at 12:30:09PM +0000, Patrick Bellasi wrote:
> On 23-Jan 21:11, Peter Zijlstra wrote:
> > Anyway, once you get range composition defined; that should be something
> > like:
> >
> > R_p \Compose_g R_g
> >
> > Where R_p is the range of task-p, and R_g is the range of the g'th
> > cgroup of p (where you can make an identity between the root cgroup and
> > the system default).
> >
> > Now; as per the other email; I think the straight forward composition:
> >
> > struct range compose(struct range a, struct range b)
> > {
> > return (range){.min = clamp(a.min, b.min, b.max),
> > .max = clamp(a.max, b.min, b.max), };
> > }
>
> This composition is done in uclamp_effective_get() but it's
> slightly different, since we want to support a "nice policy" where
> tasks can always ask less then what they have got assigned.
Not sure I follow..
> Thus, from an abstract standpoint, if a task is in a cgroup:
>
> task.min <= R_g.min
> task.max <= R_g.max
>
> While, for tasks in the root cgroup system default applies and we
> enforece:
>
> task.min >= R_0.min
> task.max <= R_0.max
>
> ... where the "nice policy" is currently not more supported, but
> perhaps we can/should use the same for system defaults too.
That seems inconsistent at best.
OK, I'll go have another look. I never recognised that function for
doing that.
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Peter Zijlstra @ 2019-01-24 15:31 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190124123009.2yulcf25ld66popd@e110439-lin>
On Thu, Jan 24, 2019 at 12:30:09PM +0000, Patrick Bellasi wrote:
> > So I'll have to go over the code again, but I'm wondering why you're
> > changing uclamp_se::bucket_id on a runnable task.
>
> We change only the "requested" value, not the "effective" one.
>
> > Ideally you keep bucket_id invariant between enqueue and dequeue; then
> > dequeue knows where we put it.
>
> Right, that's what we do for the "effective" value.
So the problem I have is that you first introduce uclamp_se::value and
use that all over the code, and then introduce effective and change all
the usage sites.
That seems daft. Why not keep all the code as-is and add orig_value.
> > Now I suppose actually determining bucket_id is 'expensive' (it
> > certainly is with the whole mapping scheme, but even that integer
> > division is not nice), so we'd like to precompute the bucket_id.
>
> Yes, although the complexity is mostly in the composition logic
> described above not on mapping at all. We have "mapping" overheads
> only when we change a "request" value and that's from slow-paths.
It's weird though. Esp. when combined with that mapping logic, because
then you get to use additional maps that are not in fact ever used.
> > We can update uclamp_se::value and set uclamp_se::changed, and then the
> > next enqueue will (unlikely) test-and-clear changed and recompute the
> > bucket_id.
>
> This mean will lazy update the "requested" bucket_id by deferring its
> computation at enqueue time. Which saves us a copy of the bucket_id,
> i.e. we will have only the "effective" value updated at enqueue time.
>
> But...
>
> > Would that not be simpler?
>
> ... although being simpler it does not fully exploit the slow-path,
> a syscall which is usually running from a different process context
> (system management software).
>
> It also fits better for lazy updates but, in the cgroup case, where we
> wanna enforce an update ASAP for RUNNABLE tasks, we will still have to
> do the updates from the slow-path.
>
> Will look better into this simplification while working on v7, perhaps
> the linear mapping can really help in that too.
OK. So mostly my complaint is that it seems to do things odd for ill
explained reasons.
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Peter Zijlstra @ 2019-01-24 15:15 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190115101513.2822-10-patrick.bellasi@arm.com>
On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> Add uclamp_default_perf, a special set of clamp values to be used
> for tasks requiring maximum performance, i.e. by default all the non
> clamped RT tasks.
Urgh... why though?
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Peter Zijlstra @ 2019-01-24 15:12 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190124123835.nliwk2onvrhe5aqf@e110439-lin>
On Thu, Jan 24, 2019 at 12:38:35PM +0000, Patrick Bellasi wrote:
> On 24-Jan 12:30, Patrick Bellasi wrote:
> > On 23-Jan 21:11, Peter Zijlstra wrote:
> > > On Wed, Jan 23, 2019 at 02:40:11PM +0000, Patrick Bellasi wrote:
> > > > On 23-Jan 11:49, Peter Zijlstra wrote:
> > > > > On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
>
> [...]
>
> > > I'm thikning that if we haz a single bit, say:
> > >
> > > struct uclamp_se {
> > > ...
> > > unsigned int changed : 1;
> > > };
> > >
> > > We can update uclamp_se::value and set uclamp_se::changed, and then the
> > > next enqueue will (unlikely) test-and-clear changed and recompute the
> > > bucket_id.
> >
> > This mean will lazy update the "requested" bucket_id by deferring its
> > computation at enqueue time. Which saves us a copy of the bucket_id,
> > i.e. we will have only the "effective" value updated at enqueue time.
> >
> > But...
> >
> > > Would that not be simpler?
> >
> > ... although being simpler it does not fully exploit the slow-path,
> > a syscall which is usually running from a different process context
> > (system management software).
> >
> > It also fits better for lazy updates but, in the cgroup case, where we
> > wanna enforce an update ASAP for RUNNABLE tasks, we will still have to
> > do the updates from the slow-path.
> >
> > Will look better into this simplification while working on v7, perhaps
> > the linear mapping can really help in that too.
>
> Actually, I forgot to mention that:
>
> uclamp_se::effective::{
> value, bucket_id
> }
>
> will be still required to proper support the cgroup delegation model,
> where a child group could be restricted by the parent but we want to
> keep track of the original "requested" value for when the parent
> should relax the restriction.
>
> Thus, since effective values are already there, why not using them
> also to pre-compute the new requested bucket_id from the slow path?
Well, we need the orig_value; but I'm still not sure why you need more
bucket_id's. Also, retaining orig_value is already required for the
system limits, there's nothing cgroup-y about this afaict.
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-24 14:25 UTC (permalink / raw)
To: Dominique Martinet
Cc: Linus Torvalds, Andy Lutomirski, Josh Snyder, Dave Chinner,
Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <20190124124501.GA18012@nautica>
[-- Attachment #1: Type: text/plain, Size: 591 bytes --]
On Thu, 24 Jan 2019, Dominique Martinet wrote:
> Jiri, you've offered resubmitting the last two patches properly, can you
> incorporate this change or should I just send this directly? (I'd take
> most of your commit message and add your name somewhere)
I've been running some basic smoke testing with the kernel from
https://git.kernel.org/pub/scm/linux/kernel/git/jikos/jikos.git/log/?h=pagecache-sidechannel-v2
(attaching the respective two patches to apply on top of latest Linus'
tree to this mail as well), and everything looks good so far.
Thanks,
--
Jiri Kosina
SUSE Labs
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch; name=0001-mm-mincore-make-mincore-more-conservative.patch, Size: 2335 bytes --]
From 9810565f1d5f966a84900cdcb85e33aa7571afbe Mon Sep 17 00:00:00 2001
From: Jiri Kosina <jkosina@suse.cz>
Date: Wed, 16 Jan 2019 20:53:17 +0100
Subject: [PATCH 1/2] mm/mincore: make mincore() more conservative
The semantics of what mincore() considers to be resident is not completely
clear, but Linux has always (since 2.3.52, which is when mincore() was
initially done) treated it as "page is available in page cache".
That's potentially a problem, as that [in]directly exposes meta-information
about pagecache / memory mapping state even about memory not strictly belonging
to the process executing the syscall, opening possibilities for sidechannel
attacks.
Change the semantics of mincore() so that it only reveals pagecache information
for non-anonymous mappings that belog to files that the calling process could
(if it tried to) successfully open for writing.
Originally-by: Linus Torvalds <torvalds@linux-foundation.org>
Originally-by: Dominique Martinet <asmadeus@codewreck.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
mm/mincore.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/mm/mincore.c b/mm/mincore.c
index 218099b5ed31..747a4907a3ac 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -169,6 +169,14 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
return 0;
}
+static inline bool can_do_mincore(struct vm_area_struct *vma)
+{
+ return vma_is_anonymous(vma) ||
+ (vma->vm_file &&
+ (inode_owner_or_capable(file_inode(vma->vm_file))
+ || inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0));
+}
+
/*
* Do a chunk of "sys_mincore()". We've already checked
* all the arguments, we hold the mmap semaphore: we should
@@ -189,8 +197,13 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
vma = find_vma(current->mm, addr);
if (!vma || addr < vma->vm_start)
return -ENOMEM;
- mincore_walk.mm = vma->vm_mm;
end = min(vma->vm_end, addr + (pages << PAGE_SHIFT));
+ if (!can_do_mincore(vma)) {
+ unsigned long pages = (end - addr) >> PAGE_SHIFT;
+ memset(vec, 1, pages);
+ return pages;
+ }
+ mincore_walk.mm = vma->vm_mm;
err = walk_page_range(addr, end, &mincore_walk);
if (err < 0)
return err;
--
2.12.3
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-patch; name=0002-mm-filemap-initiate-readahead-even-if-IOCB_NOWAIT-is.patch, Size: 1368 bytes --]
From f287185fc5e0ffbbb380f2d68dd19290715829a8 Mon Sep 17 00:00:00 2001
From: Jiri Kosina <jkosina@suse.cz>
Date: Wed, 16 Jan 2019 21:06:58 +0100
Subject: [PATCH 2/2] mm/filemap: initiate readahead even if IOCB_NOWAIT is set
for the I/O
preadv2(RWF_NOWAIT) can be used to open a side-channel to pagecache contents, as
it reveals metadata about residency of pages in pagecache.
If preadv2(RWF_NOWAIT) returns immediately, it provides a clear "page not
resident" information, and vice versa.
Close that sidechannel by always initiating readahead on the cache if we
encounter a cache miss for preadv2(RWF_NOWAIT); with that in place, probing
the pagecache residency itself will actually populate the cache, making the
sidechannel useless.
Originally-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
mm/filemap.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index 9f5e323e883e..7bcdd36e629d 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2075,8 +2075,6 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb,
page = find_get_page(mapping, index);
if (!page) {
- if (iocb->ki_flags & IOCB_NOWAIT)
- goto would_block;
page_cache_sync_readahead(mapping,
ra, filp,
index, last_index - index);
--
2.12.3
^ permalink raw reply related
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dominique Martinet @ 2019-01-24 12:45 UTC (permalink / raw)
To: Linus Torvalds, Jiri Kosina
Cc: Andy Lutomirski, Josh Snyder, Dave Chinner, Matthew Wilcox,
Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko,
Linux-MM, kernel list, Linux API
In-Reply-To: <20190124002455.GA23181@nautica>
Dominique Martinet wrote on Thu, Jan 24, 2019:
> I was thinking of something along the lines of:
> return vma_is_anonymous(vma) || (vma->vm_file &&
> (inode_owner_or_capable(file_inode(vma->vm_file))
> || inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0));
>
> I dropped the first f_mode check because none of the known mincore users
> open the files read-write, and the check is redundant with
> inode_permission() so while it would probably be an optimisation in some
> cases I do not think it is useful in practice.
> On the other hand, I have no idea how expensive the inode_permission and
> owner checks really are - do they try to refresh attributes on a
> networked filesystem or would it trust the cache or is it fs dependant?
>
> Honestly this is more a case of "the people who's be interested in
> seeing this have no idea what they're doing" than lack of interest.. I
> wouldn't mind if there were tests doing mincore on a bunch of special
> files/mappings but I just tried on a few regular files by hand, this
> isn't proper coverage; I'll try to take more time to test various
> mappings today (JST).
I've done some tests with this, it appears OK.
Obviously the tests I previously had done still work:
- user's own files are ok, even if read-only now.
- non-user writable files are ok.
- non-user non-writable files (e.g. system libs) aren't.
- root can still do anything.
On new tests:
- there are vmas with no file that aren't anonymous and come all the
way there (vvar and vdso), so factoring vma->vm_file check is definitely
needed.
- vsyscall doesn't reach can_do_mincore()
- [heap] [stack] and other fileless regular maps are anonymous
- I tried a char device (/dev/zero) and it was marked anonymous despite
mapping with MAP_SHARED, which is somewhat expected I guess?
- I couldn't map /proc or /sys files (no such device), so no mincore
there.
I'd post my test program but I actually added pr_info messages in
can_do_mincore to check what it returned because madvise dontneed isn't
guaranteed to evict pages so we can't rely on madvise dontneed + mincore
to return 0; not sure what to do for ltp... If anyone has a good idea of
how to check if mincore actually got granted permissions without
drop_caches I'll post to the ltp github.
Anything else to try?
Jiri, you've offered resubmitting the last two patches properly, can you
incorporate this change or should I just send this directly? (I'd take
most of your commit message and add your name somewhere)
Thanks,
--
Dominique
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Patrick Bellasi @ 2019-01-24 12:38 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190124123009.2yulcf25ld66popd@e110439-lin>
On 24-Jan 12:30, Patrick Bellasi wrote:
> On 23-Jan 21:11, Peter Zijlstra wrote:
> > On Wed, Jan 23, 2019 at 02:40:11PM +0000, Patrick Bellasi wrote:
> > > On 23-Jan 11:49, Peter Zijlstra wrote:
> > > > On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
[...]
> > I'm thikning that if we haz a single bit, say:
> >
> > struct uclamp_se {
> > ...
> > unsigned int changed : 1;
> > };
> >
> > We can update uclamp_se::value and set uclamp_se::changed, and then the
> > next enqueue will (unlikely) test-and-clear changed and recompute the
> > bucket_id.
>
> This mean will lazy update the "requested" bucket_id by deferring its
> computation at enqueue time. Which saves us a copy of the bucket_id,
> i.e. we will have only the "effective" value updated at enqueue time.
>
> But...
>
> > Would that not be simpler?
>
> ... although being simpler it does not fully exploit the slow-path,
> a syscall which is usually running from a different process context
> (system management software).
>
> It also fits better for lazy updates but, in the cgroup case, where we
> wanna enforce an update ASAP for RUNNABLE tasks, we will still have to
> do the updates from the slow-path.
>
> Will look better into this simplification while working on v7, perhaps
> the linear mapping can really help in that too.
Actually, I forgot to mention that:
uclamp_se::effective::{
value, bucket_id
}
will be still required to proper support the cgroup delegation model,
where a child group could be restricted by the parent but we want to
keep track of the original "requested" value for when the parent
should relax the restriction.
Thus, since effective values are already there, why not using them
also to pre-compute the new requested bucket_id from the slow path?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 05/16] sched/core: uclamp: Update CPU's refcount on clamp changes
From: Peter Zijlstra @ 2019-01-24 12:38 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190124112153.pwdsbxjynq6chmvl@e110439-lin>
On Thu, Jan 24, 2019 at 11:21:53AM +0000, Patrick Bellasi wrote:
> When a task-specific uclamp value is changed for a task, instead, a
> dequeue/enqueue is not needed. As long as we are doing a lazy update,
> that sounds just like not necessary overhead.
When that overhead is shown to be a problem, is when we'll look at that
:-)
> However, there could still be value in keeping code consistent and if
> you prefer it this way what should I do?
>
> ---8<---
> __sched_setscheduler()
> ...
> if (policy < 0)
> policy = oldpolicy = p->policy;
> ...
> if (unlikely(policy == p->policy)) {
> ...
> if (uclamp_changed()) // Force dequeue/enqueue
> goto change;
> }
> change:
> ...
>
> if (queued)
> dequeue_task(rq, p, queue_flags);
> if (running)
> put_prev_task(rq, p);
>
> __setscheduler_uclamp();
> __setscheduler(rq, p, attr, pi);
>
> if (queued)
> enqueue_task(rq, p, queue_flags);
> if (running)
> set_curr_task(rq, p);
> ...
> ---8<---
>
> Could be something like that ok with you?
Yes, that's about what I was expecting.
> Not sure about side-effects on p->prio(): for CFS seems to be reset to
> NORMAL in this case :/
That's what we need KEEP_PARAM for, right?
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Patrick Bellasi @ 2019-01-24 12:30 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123201146.GH17749@hirez.programming.kicks-ass.net>
On 23-Jan 21:11, Peter Zijlstra wrote:
> On Wed, Jan 23, 2019 at 02:40:11PM +0000, Patrick Bellasi wrote:
> > On 23-Jan 11:49, Peter Zijlstra wrote:
> > > On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> > > > @@ -858,16 +859,23 @@ static inline void
> > > > uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> > > > unsigned int *clamp_value, unsigned int *bucket_id)
> > > > {
> > > > + struct uclamp_se *default_clamp;
> > > > +
> > > > /* Task specific clamp value */
> > > > *clamp_value = p->uclamp[clamp_id].value;
> > > > *bucket_id = p->uclamp[clamp_id].bucket_id;
> > > >
> > > > + /* RT tasks have different default values */
> > > > + default_clamp = task_has_rt_policy(p)
> > > > + ? uclamp_default_perf
> > > > + : uclamp_default;
> > > > +
> > > > /* System default restriction */
> > > > - if (unlikely(*clamp_value < uclamp_default[UCLAMP_MIN].value ||
> > > > - *clamp_value > uclamp_default[UCLAMP_MAX].value)) {
> > > > + if (unlikely(*clamp_value < default_clamp[UCLAMP_MIN].value ||
> > > > + *clamp_value > default_clamp[UCLAMP_MAX].value)) {
> > > > /* Keep it simple: unconditionally enforce system defaults */
> > > > - *clamp_value = uclamp_default[clamp_id].value;
> > > > - *bucket_id = uclamp_default[clamp_id].bucket_id;
> > > > + *clamp_value = default_clamp[clamp_id].value;
> > > > + *bucket_id = default_clamp[clamp_id].bucket_id;
> > > > }
> > > > }
> > >
> > > So I still don't much like the whole effective thing;
> >
> > :/
> >
> > I find back-annotation useful in many cases since we have different
> > sources for possible clamp values:
> >
> > 1. task specific
> > 2. cgroup defined
> > 3. system defaults
> > 4. system power default
>
> (I'm not sure I've seen 4 happen yet, what's that?)
Typo: that's "system s/power/perf/ default", i.e. uclamp_default_perf
introduced by this patch.
> Anyway, once you get range composition defined; that should be something
> like:
>
> R_p \Compose_g R_g
>
> Where R_p is the range of task-p, and R_g is the range of the g'th
> cgroup of p (where you can make an identity between the root cgroup and
> the system default).
>
> Now; as per the other email; I think the straight forward composition:
>
> struct range compose(struct range a, struct range b)
> {
> return (range){.min = clamp(a.min, b.min, b.max),
> .max = clamp(a.max, b.min, b.max), };
> }
This composition is done in uclamp_effective_get() but it's
slightly different, since we want to support a "nice policy" where
tasks can always ask less then what they have got assigned.
Thus, from an abstract standpoint, if a task is in a cgroup:
task.min <= R_g.min
task.max <= R_g.max
While, for tasks in the root cgroup system default applies and we
enforece:
task.min >= R_0.min
task.max <= R_0.max
... where the "nice policy" is currently not more supported, but
perhaps we can/should use the same for system defaults too.
> (note that this is non-commutative, so we have to pay attention to
> get the order 'right')
>
> Works in this case; unlike the cpu/rq conposition where we resort to a
> pure max function for non-interference.
Right.
> > I don't think we can avoid to somehow back annotate on which bucket a
> > task has been refcounted... it makes dequeue so much easier, it helps
> > in ensuring that the refcouning is consistent and enable lazy updates.
>
> So I'll have to go over the code again, but I'm wondering why you're
> changing uclamp_se::bucket_id on a runnable task.
We change only the "requested" value, not the "effective" one.
> Ideally you keep bucket_id invariant between enqueue and dequeue; then
> dequeue knows where we put it.
Right, that's what we do for the "effective" value.
> Now I suppose actually determining bucket_id is 'expensive' (it
> certainly is with the whole mapping scheme, but even that integer
> division is not nice), so we'd like to precompute the bucket_id.
Yes, although the complexity is mostly in the composition logic
described above not on mapping at all. We have "mapping" overheads
only when we change a "request" value and that's from slow-paths.
The "effective" value is computed at each enqueue time by composing
precomputed bucket_id representing the current "requested" task's,
cgroup's and system default's values.
> This then leads to the problem of how to change uclamp_se::value while
> runnable (since per the other thread you don't want to always update all
> runnable tasks).
>
> So far so right?
Yes.
> I'm thikning that if we haz a single bit, say:
>
> struct uclamp_se {
> ...
> unsigned int changed : 1;
> };
>
> We can update uclamp_se::value and set uclamp_se::changed, and then the
> next enqueue will (unlikely) test-and-clear changed and recompute the
> bucket_id.
This mean will lazy update the "requested" bucket_id by deferring its
computation at enqueue time. Which saves us a copy of the bucket_id,
i.e. we will have only the "effective" value updated at enqueue time.
But...
> Would that not be simpler?
... although being simpler it does not fully exploit the slow-path,
a syscall which is usually running from a different process context
(system management software).
It also fits better for lazy updates but, in the cgroup case, where we
wanna enforce an update ASAP for RUNNABLE tasks, we will still have to
do the updates from the slow-path.
Will look better into this simplification while working on v7, perhaps
the linear mapping can really help in that too.
Cheers Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ 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