* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Joel Fernandes @ 2019-08-13 19:18 UTC (permalink / raw)
To: Daniel Gruss
Cc: Jann Horn, Michal Hocko, kernel list, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, Daniel Colascione, fmayer, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Kees Cook, kernel-team, Linux API,
linux-doc, linux-fsdevel, Linux-MM, Mike Rapoport, Minchan Kim,
namhyung, Paul E. McKenney, Robin Murphy, Roman Gushchin,
Stephen Rothwell, Suren Baghdasaryan, Thomas Gleixner, Todd Kjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <d6ae7f06-f0ef-ec00-a020-98e7cfada281@iaik.tugraz.at>
On Tue, Aug 13, 2019 at 05:34:16PM +0200, Daniel Gruss wrote:
> On 8/13/19 5:29 PM, Jann Horn wrote:
> > On Tue, Aug 13, 2019 at 12:09 PM Michal Hocko <mhocko@kernel.org> wrote:
> >> On Mon 12-08-19 20:14:38, Jann Horn wrote:
> >>> On Wed, Aug 7, 2019 at 7:16 PM Joel Fernandes (Google)
> >>> <joel@joelfernandes.org> wrote:
> >>>> The page_idle tracking feature currently requires looking up the pagemap
> >>>> for a process followed by interacting with /sys/kernel/mm/page_idle.
> >>>> Looking up PFN from pagemap in Android devices is not supported by
> >>>> unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
> >>>>
> >>>> This patch adds support to directly interact with page_idle tracking at
> >>>> the PID level by introducing a /proc/<pid>/page_idle file. It follows
> >>>> the exact same semantics as the global /sys/kernel/mm/page_idle, but now
> >>>> looking up PFN through pagemap is not needed since the interface uses
> >>>> virtual frame numbers, and at the same time also does not require
> >>>> SYS_ADMIN.
> >>>>
> >>>> In Android, we are using this for the heap profiler (heapprofd) which
> >>>> profiles and pin points code paths which allocates and leaves memory
> >>>> idle for long periods of time. This method solves the security issue
> >>>> with userspace learning the PFN, and while at it is also shown to yield
> >>>> better results than the pagemap lookup, the theory being that the window
> >>>> where the address space can change is reduced by eliminating the
> >>>> intermediate pagemap look up stage. In virtual address indexing, the
> >>>> process's mmap_sem is held for the duration of the access.
> >>>
> >>> What happens when you use this interface on shared pages, like memory
> >>> inherited from the zygote, library file mappings and so on? If two
> >>> profilers ran concurrently for two different processes that both map
> >>> the same libraries, would they end up messing up each other's data?
> >>
> >> Yup PageIdle state is shared. That is the page_idle semantic even now
> >> IIRC.
> >>
> >>> Can this be used to observe which library pages other processes are
> >>> accessing, even if you don't have access to those processes, as long
> >>> as you can map the same libraries? I realize that there are already a
> >>> bunch of ways to do that with side channels and such; but if you're
> >>> adding an interface that allows this by design, it seems to me like
> >>> something that should be gated behind some sort of privilege check.
> >>
> >> Hmm, you need to be priviledged to get the pfn now and without that you
> >> cannot get to any page so the new interface is weakening the rules.
> >> Maybe we should limit setting the idle state to processes with the write
> >> status. Or do you think that even observing idle status is useful for
> >> practical side channel attacks? If yes, is that a problem of the
> >> profiler which does potentially dangerous things?
> >
> > I suppose read-only access isn't a real problem as long as the
> > profiler isn't writing the idle state in a very tight loop... but I
> > don't see a usecase where you'd actually want that? As far as I can
> > tell, if you can't write the idle state, being able to read it is
> > pretty much useless.
> >
> > If the profiler only wants to profile process-private memory, then
> > that should be implementable in a safe way in principle, I think, but
> > since Joel said that they want to profile CoW memory as well, I think
> > that's inherently somewhat dangerous.
>
> I agree that allowing profiling of shared pages would leak information.
Will think more about it. If we limit it to private pages, then it could
become useless. Consider a scenario where:
A process allocates a some memory, then forks a bunch of worker processes
that read that memory and perform some work with them. Per-PID page idle
tracking is now run on the parent processes. Now it should appear that the
pages are actively accessed (not-idle). If we don't track shared pages, then
we cannot detect if those pages are really due to memory leaking, or if they
are there for a purpose and are actively used.
> To me the use case is not entirely clear. This is not a feature that
> would normally be run in everyday computer usage, right?
Generally, this to be used as a debugging feature that helps developers
detect memory leaks in their programs.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v3 1/2] rcu/tree: Add basic support for kfree_rcu batching
From: Paul E. McKenney @ 2019-08-13 19:07 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Rao Shoaib, max.byungchul.park, byungchul.park,
kernel-team, kernel-team, Andrew Morton, Davidlohr Bueso,
Jonathan Corbet, Josh Triplett, Kees Cook, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, Mauro Carvalho Chehab, rcu,
Steven Rostedt, Thomas Gleixner
In-Reply-To: <20190813170046.81707-1-joel@joelfernandes.org>
On Tue, Aug 13, 2019 at 01:00:45PM -0400, Joel Fernandes (Google) wrote:
> Recently a discussion about stability and performance of a system
> involving a high rate of kfree_rcu() calls surfaced on the list [1]
> which led to another discussion how to prepare for this situation.
Looks much improved! More questions and comments inline.
Thanx, Paul
> This patch adds basic batching support for kfree_rcu(). It is "basic"
> because we do none of the slab management, dynamic allocation, code
> moving or any of the other things, some of which previous attempts did
> [2]. These fancier improvements can be follow-up patches and there are
> different ideas being discussed in those regards.
>
> Torture tests follow in the next patch and show improvements of around
> 400% in reduction of number of grace periods on a 16 CPU system. More
s/400% in reduction/a 5x reduction/
Oddly enough, a reduction of more than 100% would require a negative
number of grace periods. RCU being what it is, although this seems
unlikely, I hesitate to rule it out. ;-)
> details and test data are in that patch.
>
> This is an effort to start simple, and build up from there. In the
> future, an extension to use kfree_bulk and possibly per-slab batching
> could be done to further improve performance due to cache-locality and
> slab-specific bulk free optimizations. By using an array of pointers,
> the worker thread processing the work would need to read lesser data
> since it does not need to deal with large rcu_head(s) any longer.
This should be combined with the second paragraph -- they both discuss
possible follow-on work.
> There is an implication with rcu_barrier() with this patch. Since the
> kfree_rcu() calls can be batched, and may not be handed yet to the RCU
> machinery in fact, the monitor may not have even run yet to do the
> queue_rcu_work(), there seems no easy way of implementing rcu_barrier()
> to wait for those kfree_rcu()s that are already made. So this means a
> kfree_rcu() followed by an rcu_barrier() does not imply that memory will
> be freed once rcu_barrier() returns.
The usual approach (should kfree_rcu_barrier() in fact be needed) is to
record the address of the most recent block being kfree_rcu()'d on
each CPU, count the number of recorded addresses, and have the
kfree() side check the address, and do the usual atomic_dec_and_test()
and wakeup dance. This gets a bit more crazy should the kfree_rcu()
requests be grouped per slab, of course! So yes, good to avoid in
the meantime.
> Another implication is higher active memory usage (although not
> run-away..) until the kfree_rcu() flooding ends, in comparison to
> without batching. More details about this are in the second patch which
> adds an rcuperf test.
But this is adjustable, at least via patching at build time, via
KFREE_DRAIN_JIFFIES, right?
> Finally, in the near future we will get rid of kfree_rcu special casing
> within RCU such as in rcu_do_batch and switch everything to just
> batching. Currently we don't do that since timer subsystem is not yet up
> and we cannot schedule the kfree_rcu() monitor as the timer subsystem's
> lock are not initialized. That would also mean getting rid of
> kfree_call_rcu_nobatch() entirely.
Please consistently put "()" after function names.
> [1] http://lore.kernel.org/lkml/20190723035725-mutt-send-email-mst@kernel.org
> [2] https://lkml.org/lkml/2017/12/19/824
>
> Cc: Rao Shoaib <rao.shoaib@oracle.com>
> Cc: max.byungchul.park@gmail.com
> Cc: byungchul.park@lge.com
> Cc: kernel-team@android.com
> Cc: kernel-team@lge.com
> Co-developed-by: Byungchul Park <byungchul.park@lge.com>
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> ---
> v2->v3: Just some code comment changes thanks to Byungchul.
>
> RFCv1->PATCH v2: Removed limits on the ->head list, just let it grow.
> Dropped KFREE_MAX_JIFFIES to HZ/50 from HZ/20 to reduce OOM occurrence.
> Removed sleeps in rcuperf test, just using cond_resched()in loop.
> Better code comments ;)
>
> .../admin-guide/kernel-parameters.txt | 17 ++
> include/linux/rcutiny.h | 5 +
> include/linux/rcutree.h | 1 +
> kernel/rcu/tree.c | 204 +++++++++++++++++-
> 4 files changed, 221 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 7ccd158b3894..a9156ca5de24 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3895,6 +3895,23 @@
> test until boot completes in order to avoid
> interference.
>
> + rcuperf.kfree_rcu_test= [KNL]
> + Set to measure performance of kfree_rcu() flooding.
> +
> + rcuperf.kfree_nthreads= [KNL]
> + The number of threads running loops of kfree_rcu().
> +
> + rcuperf.kfree_alloc_num= [KNL]
> + Number of allocations and frees done in an iteration.
> +
> + rcuperf.kfree_loops= [KNL]
> + Number of loops doing rcuperf.kfree_alloc_num number
> + of allocations and frees.
> +
> + rcuperf.kfree_no_batch= [KNL]
> + Use the non-batching (slower) version of kfree_rcu.
> + This is useful for comparing with the batched version.
> +
> rcuperf.nreaders= [KNL]
> Set number of RCU readers. The value -1 selects
> N, where N is the number of CPUs. A value
This change to kernel-parameters.txt really needs to be in the
rcuperf patch rather than this one.
> diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
> index 8e727f57d814..383f2481750f 100644
> --- a/include/linux/rcutiny.h
> +++ b/include/linux/rcutiny.h
> @@ -39,6 +39,11 @@ static inline void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> call_rcu(head, func);
> }
>
> +static inline void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func)
> +{
> + call_rcu(head, func);
> +}
> +
> void rcu_qs(void);
>
> static inline void rcu_softirq_qs(void)
> diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h
> index 735601ac27d3..7e38b39ec634 100644
> --- a/include/linux/rcutree.h
> +++ b/include/linux/rcutree.h
> @@ -34,6 +34,7 @@ static inline void rcu_virt_note_context_switch(int cpu)
>
> void synchronize_rcu_expedited(void);
> void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func);
> +void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func);
>
> void rcu_barrier(void);
> bool rcu_eqs_special_set(int cpu);
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index a14e5fbbea46..3e62bbb0e1fa 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2593,17 +2593,195 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func)
> }
> EXPORT_SYMBOL_GPL(call_rcu);
>
> +
> +/* Maximum number of jiffies to wait before draining a batch. */
> +#define KFREE_DRAIN_JIFFIES (HZ / 50)
> +
> /*
> - * Queue an RCU callback for lazy invocation after a grace period.
> - * This will likely be later named something like "call_rcu_lazy()",
> - * but this change will require some way of tagging the lazy RCU
> - * callbacks in the list of pending callbacks. Until then, this
> - * function may only be called from __kfree_rcu().
> + * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
> + * kfree(s) is queued for freeing after a grace period, right away.
> */
> -void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> +struct kfree_rcu_cpu {
> + /* The rcu_work node for queuing work with queue_rcu_work(). The work
> + * is done after a grace period.
> + */
> + struct rcu_work rcu_work;
> +
> + /* The list of objects being queued in a batch but are not yet
> + * scheduled to be freed.
> + */
> + struct rcu_head *head;
> +
> + /* The list of objects that have now left ->head and are queued for
> + * freeing after a grace period.
> + */
> + struct rcu_head *head_free;
So this is not yet the one that does multiple batches concurrently
awaiting grace periods, correct? Or am I missing something subtle?
If it is not doing the multiple-batch optimization, what does the
400% in the commit log refer to?
> + /* Protect concurrent access to this structure. */
> + spinlock_t lock;
> +
> + /* The delayed work that flushes ->head to ->head_free incase ->head
> + * within KFREE_DRAIN_JIFFIES. In case flushing cannot be done if RCU
> + * is busy, ->head just continues to grow and we retry flushing later.
> + */
> + struct delayed_work monitor_work;
> + bool monitor_todo; /* Is a delayed work pending execution? */
> +};
> +
> +static DEFINE_PER_CPU(struct kfree_rcu_cpu, krc);
> +
> +/*
> + * This function is invoked in workqueue context after a grace period.
> + * It frees all the objects queued on ->head_free.
> + */
> +static void kfree_rcu_work(struct work_struct *work)
> +{
> + unsigned long flags;
> + struct rcu_head *head, *next;
> + struct kfree_rcu_cpu *krcp = container_of(to_rcu_work(work),
> + struct kfree_rcu_cpu, rcu_work);
> +
> + spin_lock_irqsave(&krcp->lock, flags);
> + head = krcp->head_free;
> + krcp->head_free = NULL;
> + spin_unlock_irqrestore(&krcp->lock, flags);
> +
> + /*
> + * The head is detached and not referenced from anywhere, so lockless
> + * access is Ok.
> + */
> + for (; head; head = next) {
> + next = head->next;
> + head->next = NULL;
Why do we need to NULL ->next? Could produce an expensive cache miss,
and I don't see how it helps anything. What am I missing here?
> + /* Could be possible to optimize with kfree_bulk in future */
> + __rcu_reclaim(rcu_state.name, head);
> + cond_resched_tasks_rcu_qs();
> + }
> +}
> +
> +/*
> + * Schedule the kfree batch RCU work to run in workqueue context after a GP.
> + *
> + * This function is invoked by kfree_rcu_monitor() when the KFREE_DRAIN_JIFFIES
> + * timeout has been reached.
> + */
> +static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
> +{
> + lockdep_assert_held(&krcp->lock);
> +
> + /* If a previous RCU batch work is already in progress, we cannot queue
> + * another one, just refuse the optimization and it will be retried
> + * again in KFREE_DRAIN_JIFFIES time.
> + */
> + if (krcp->head_free)
> + return false;
> +
> + krcp->head_free = krcp->head;
> + krcp->head = NULL;
> + INIT_RCU_WORK(&krcp->rcu_work, kfree_rcu_work);
> + queue_rcu_work(system_wq, &krcp->rcu_work);
> +
> + return true;
> +}
> +
> +static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
> + unsigned long flags)
> +{
> + /* Flush ->head to ->head_free, all objects on ->head_free will be
> + * kfree'd after a grace period.
> + */
> + if (queue_kfree_rcu_work(krcp)) {
> + /* Success! Our job is done here. */
> + spin_unlock_irqrestore(&krcp->lock, flags);
> + return;
> + }
> +
> + /* Previous batch did not get free yet, let us try again soon. */
> + if (krcp->monitor_todo == false) {
> + schedule_delayed_work_on(smp_processor_id(),
> + &krcp->monitor_work, KFREE_DRAIN_JIFFIES);
Given that we are using a lock, do we need to restrict to the current
CPU? In any case, we do need to handle the case where the current
CPU has gone offline before we get around to sending a batch off to
wait for a grace period, correct?
> + krcp->monitor_todo = true;
> + }
> + spin_unlock_irqrestore(&krcp->lock, flags);
> +}
> +
> +/*
> + * This function is invoked after the KFREE_DRAIN_JIFFIES timeout has elapsed,
> + * so it drains the specified kfree_rcu_cpu structure's ->head list.
> + */
> +static void kfree_rcu_monitor(struct work_struct *work)
> +{
> + bool todo;
> + unsigned long flags;
> + struct kfree_rcu_cpu *krcp = container_of(work, struct kfree_rcu_cpu,
> + monitor_work.work);
> +
> + spin_lock_irqsave(&krcp->lock, flags);
> + todo = krcp->monitor_todo;
> + krcp->monitor_todo = false;
> + if (todo)
> + kfree_rcu_drain_unlock(krcp, flags);
> + else
> + spin_unlock_irqrestore(&krcp->lock, flags);
> +}
> +
> +/*
> + * This version of kfree_call_rcu does not do batching of kfree_rcu() requests.
> + * Used only by rcuperf torture test for comparison with kfree_rcu_batch().
> + */
> +void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func)
> {
> __call_rcu(head, func, -1, 1);
> }
> +EXPORT_SYMBOL_GPL(kfree_call_rcu_nobatch);
> +
> +/*
> + * Queue a request for lazy invocation of kfree() after a grace period.
> + *
> + * Each kfree_call_rcu() request is added to a batch. The batch will be drained
> + * every KFREE_DRAIN_JIFFIES number of jiffies. All the objects in the batch
> + * will be kfree'd in workqueue context. This allows us to:
> + *
> + * 1. Batch requests together to reduce the number of grace periods during
> + * heavy kfree_rcu() load.
> + *
> + * 2. In the future, makes it possible to use kfree_bulk() on a large number of
> + * kfree_rcu() requests thus reducing the per-object overhead of kfree() and
> + * also reducing cache misses.
> + */
> +void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> +{
> + unsigned long flags;
> + struct kfree_rcu_cpu *krcp;
> + bool monitor_todo;
> +
> + /* kfree_call_rcu() batching requires timers to be up. If the scheduler
> + * is not yet up, just skip batching and do non-batched kfree_call_rcu().
> + */
> + if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING)
> + return kfree_call_rcu_nobatch(head, func);
> +
> + local_irq_save(flags);
> + krcp = this_cpu_ptr(&krc);
> +
> + spin_lock(&krcp->lock);
> +
> + /* Queue the kfree but don't yet schedule the batch. */
> + head->func = func;
> + head->next = krcp->head;
> + krcp->head = head;
> +
> + /* Schedule monitor for timely drain after KFREE_DRAIN_JIFFIES. */
> + monitor_todo = krcp->monitor_todo;
> + krcp->monitor_todo = true;
> + spin_unlock(&krcp->lock);
> +
> + if (!monitor_todo) {
> + schedule_delayed_work_on(smp_processor_id(),
> + &krcp->monitor_work, KFREE_DRAIN_JIFFIES);
> + }
> + local_irq_restore(flags);
> +}
> EXPORT_SYMBOL_GPL(kfree_call_rcu);
>
> /*
> @@ -3455,10 +3633,24 @@ static void __init rcu_dump_rcu_node_tree(void)
> struct workqueue_struct *rcu_gp_wq;
> struct workqueue_struct *rcu_par_gp_wq;
>
> +static void __init kfree_rcu_batch_init(void)
> +{
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
> +
> + spin_lock_init(&krcp->lock);
> + INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
> + }
> +}
> +
> void __init rcu_init(void)
> {
> int cpu;
>
> + kfree_rcu_batch_init();
> +
> rcu_early_boot_tests();
>
> rcu_bootup_announce();
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
^ permalink raw reply
* [PATCH] devicetree: Expose dtbs_check and dt_binding_check some more
From: Stephen Boyd @ 2019-08-13 18:38 UTC (permalink / raw)
To: Rob Herring
Cc: linux-kernel, Masahiro Yamada, linux-kbuild, devicetree,
linux-doc
It wasn't obvious that this was a command to run based on 'make help',
so add it to the top-level help for devicetree builds. Also, add an
example to the documentation to show that db_binding_check can be run
with DT_SCHEMA_FILES= to only check one schema file instead of all of
them.
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: <linux-kbuild@vger.kernel.org>
Cc: <devicetree@vger.kernel.org>
Cc: <linux-doc@vger.kernel.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
I didn't find this sent, so sending again!
Documentation/devicetree/writing-schema.md | 1 +
Makefile | 6 ++++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/writing-schema.md b/Documentation/devicetree/writing-schema.md
index dc032db36262..17ad67887fde 100644
--- a/Documentation/devicetree/writing-schema.md
+++ b/Documentation/devicetree/writing-schema.md
@@ -120,6 +120,7 @@ This will first run the `dt_binding_check` which generates the processed schema.
It is also possible to run checks with a single schema file by setting the
'DT_SCHEMA_FILES' variable to a specific schema file.
+`make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/trivial-devices.yaml`
`make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/trivial-devices.yaml`
diff --git a/Makefile b/Makefile
index 9be5834073f8..96bb28aa1c46 100644
--- a/Makefile
+++ b/Makefile
@@ -1503,8 +1503,10 @@ help:
@echo ''
@$(if $(dtstree), \
echo 'Devicetree:'; \
- echo '* dtbs - Build device tree blobs for enabled boards'; \
- echo ' dtbs_install - Install dtbs to $(INSTALL_DTBS_PATH)'; \
+ echo '* dtbs - Build device tree blobs for enabled boards'; \
+ echo ' dtbs_install - Install dtbs to $(INSTALL_DTBS_PATH)'; \
+ echo ' dt_binding_check - Validate device tree binding documents'; \
+ echo ' dtbs_check - Validate device tree source files';\
echo '')
@echo 'Userspace tools targets:'
--
Sent by a computer through tubes
^ permalink raw reply related
* Re: [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree
From: Brendan Higgins @ 2019-08-13 17:50 UTC (permalink / raw)
To: Knut Omang
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
open list:DOCUMENTATION, linux-kbuild, Shuah Khan,
Jonathan Corbet, Masahiro Yamada, Michal Marek,
Greg Kroah-Hartman, Shreyans Devendra Doshi, Alan Maguire,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <b4d2cf8635103e30313773761b095f3a1cfd9a82.camel@oracle.com>
On Tue, Aug 13, 2019 at 4:29 AM Knut Omang <knut.omang@oracle.com> wrote:
>
> On Tue, 2019-08-13 at 01:17 -0700, Brendan Higgins wrote:
> > On Mon, Aug 12, 2019 at 11:11 PM Knut Omang <knut.omang@oracle.com> wrote:
> > [...]
> > > Alan Maguire (3):
> > > ktf: Implementation of ktf support for overriding function entry and return.
> > > ktf: A simple debugfs interface to test results
> > > ktf: Simple coverage support
> > >
> > > Knut Omang (16):
> > > kbuild: Fixes to rules for host-cshlib and host-cxxshlib
> > > ktf: Introduce the main part of the kernel side of ktf
> > > ktf: Introduce a generic netlink protocol for test result communication
> > > ktf: An implementation of a generic associative array container
> > > ktf: Configurable context support for network info setup
> > > ktf: resolve: A helper utility to aid in exposing private kernel symbols to KTF tests.
> > > ktf: Add documentation for Kernel Test Framework (KTF)
> > > ktf: Add a small test suite with a few tests to test KTF itself
> > > ktf: Main part of user land library for executing tests
> > > ktf: Integration logic for running ktf tests from googletest
> > > ktf: Internal debugging facilities
> > > ktf: Some simple examples
> > > ktf: Some user applications to run tests
> > > ktf: Toplevel ktf Makefile/makefile includes and scripts to run from kselftest
> > > kselftests: Enable building ktf
> > > Documentation/dev-tools: Add index entry for KTF documentation
> > >
> > > Documentation/dev-tools/index.rst | 1 +-
> > > Documentation/dev-tools/ktf/concepts.rst | 242 +++-
> > > Documentation/dev-tools/ktf/debugging.rst | 248 +++-
> > > Documentation/dev-tools/ktf/examples.rst | 26 +-
> > > Documentation/dev-tools/ktf/features.rst | 307 ++++-
> > > Documentation/dev-tools/ktf/implementation.rst | 70 +-
> > > Documentation/dev-tools/ktf/index.rst | 14 +-
> > > Documentation/dev-tools/ktf/installation.rst | 73 +-
> > > Documentation/dev-tools/ktf/introduction.rst | 134 ++-
> > > Documentation/dev-tools/ktf/progref.rst | 144 ++-
> > > scripts/Makefile.host | 17 +-
> > > tools/testing/selftests/Makefile | 1 +-
> > > tools/testing/selftests/ktf/Makefile | 21 +-
> > > tools/testing/selftests/ktf/examples/Makefile | 17 +-
> > > tools/testing/selftests/ktf/examples/h2.c | 45 +-
> > > tools/testing/selftests/ktf/examples/h3.c | 84 +-
> > > tools/testing/selftests/ktf/examples/h4.c | 62 +-
> > > tools/testing/selftests/ktf/examples/hello.c | 38 +-
> > > tools/testing/selftests/ktf/examples/kgdemo.c | 61 +-
> > > tools/testing/selftests/ktf/kernel/Makefile | 15 +-
> > > tools/testing/selftests/ktf/kernel/ktf.h | 604 +++++++-
> > > tools/testing/selftests/ktf/kernel/ktf_context.c | 409 +++++-
> > > tools/testing/selftests/ktf/kernel/ktf_cov.c | 690 ++++++++-
> > > tools/testing/selftests/ktf/kernel/ktf_cov.h | 94 +-
> > > tools/testing/selftests/ktf/kernel/ktf_debugfs.c | 356 ++++-
> > > tools/testing/selftests/ktf/kernel/ktf_debugfs.h | 34 +-
> > > tools/testing/selftests/ktf/kernel/ktf_map.c | 261 +++-
> > > tools/testing/selftests/ktf/kernel/ktf_map.h | 154 ++-
> > > tools/testing/selftests/ktf/kernel/ktf_netctx.c | 132 ++-
> > > tools/testing/selftests/ktf/kernel/ktf_netctx.h | 64 +-
> > > tools/testing/selftests/ktf/kernel/ktf_nl.c | 516 ++++++-
> > > tools/testing/selftests/ktf/kernel/ktf_nl.h | 15 +-
> > > tools/testing/selftests/ktf/kernel/ktf_override.c | 45 +-
> > > tools/testing/selftests/ktf/kernel/ktf_override.h | 15 +-
> > > tools/testing/selftests/ktf/kernel/ktf_test.c | 397 +++++-
> > > tools/testing/selftests/ktf/kernel/ktf_test.h | 381 ++++-
> > > tools/testing/selftests/ktf/kernel/ktf_unlproto.h | 105 +-
> > > tools/testing/selftests/ktf/lib/Makefile | 21 +-
> > > tools/testing/selftests/ktf/lib/ktf.h | 114 +-
> > > tools/testing/selftests/ktf/lib/ktf_debug.cc | 20 +-
> > > tools/testing/selftests/ktf/lib/ktf_debug.h | 59 +-
> > > tools/testing/selftests/ktf/lib/ktf_int.cc | 1031 ++++++++++++-
> > > tools/testing/selftests/ktf/lib/ktf_int.h | 84 +-
> > > tools/testing/selftests/ktf/lib/ktf_run.cc | 177 ++-
> > > tools/testing/selftests/ktf/lib/ktf_unlproto.c | 21 +-
> > > tools/testing/selftests/ktf/scripts/ktf_syms.mk | 16 +-
> > > tools/testing/selftests/ktf/scripts/resolve | 188 ++-
> > > tools/testing/selftests/ktf/scripts/runtests.mk | 3 +-
> > > tools/testing/selftests/ktf/scripts/runtests.sh | 100 +-
> > > tools/testing/selftests/ktf/scripts/top_make.mk | 14 +-
> > > tools/testing/selftests/ktf/selftest/Makefile | 17 +-
> > > tools/testing/selftests/ktf/selftest/context.c | 149 ++-
> > > tools/testing/selftests/ktf/selftest/context.h | 15 +-
> > > tools/testing/selftests/ktf/selftest/context_self.h | 34 +-
> > > tools/testing/selftests/ktf/selftest/hybrid.c | 35 +-
> > > tools/testing/selftests/ktf/selftest/hybrid.h | 24 +-
> > > tools/testing/selftests/ktf/selftest/hybrid_self.h | 27 +-
> > > tools/testing/selftests/ktf/selftest/ktf_syms.txt | 17 +-
> > > tools/testing/selftests/ktf/selftest/self.c | 661 ++++++++-
> > > tools/testing/selftests/ktf/user/Makefile | 26 +-
> > > tools/testing/selftests/ktf/user/hybrid.cc | 39 +-
> > > tools/testing/selftests/ktf/user/ktfcov.cc | 68 +-
> > > tools/testing/selftests/ktf/user/ktfrun.cc | 20 +-
> > > tools/testing/selftests/ktf/user/ktftest.cc | 46 +-
> > > 64 files changed, 8909 insertions(+), 9 deletions(-)
> >
> > It also looks like all your test code lives outside of the kernel
> > source dir. I take it you intend for tests to live in
> > tools/testing/selftests/ktf/ ?
>
> Good point,
> with this RFC it would be just to add another directory under ktf/
> but this was just to find a simple way to integrate it below selftests,
> without interfering with the current structure.
>
> I imagine a tighter integration/unification between normal Kbuild targets and selftests
> targets that also took kernel module building into consideration would be a better solution.
I think tests should live alongside the code that they test, so if I
understand what you are saying, then I agree. Where do you think KTF
should go then?
> I think this is a good topic for discussion.
> As I indicate above, I think it is problematic that test code has to be explicitly
> configured in as we configure code features of the kernel, which changes the "logical"
> kernel we build.
Yep, Luis Chamberlain and I have been discussing this for a while (I
should probably try to open up that conversation and add you to it);
it's a very tricky problem. I think that in the long term a good goal
is to have a way to express code dependencies separate from the
configuration system, but that would probably mean some substantial
changes to Kbuild and friends. Even then, we have all these macros
that generate different code (not just on/off) depending on how the
kernel is configured.
> So some more "native" support for test modules are desired, IMHO.
What do you mean by "native"? Just having the tests be more aware of the code?
^ permalink raw reply
* Re: [PATCH v12 12/18] kunit: test: add tests for KUnit managed resources
From: Stephen Boyd @ 2019-08-13 17:07 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg,
Avinash Kondareddy
In-Reply-To: <CAFd5g44Es4emKyQSxUkqckGJ02_o3sAcDLwUCW8ZFGX14j5=xg@mail.gmail.com>
Quoting Brendan Higgins (2019-08-13 00:57:33)
> On Mon, Aug 12, 2019 at 9:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > BTW, maybe kunit allocation APIs should
> > fail the test if they fail to allocate in general. Unless we're unit
> > testing failure to allocate problems.
>
> Yeah, I thought about that. I wasn't sure how people would feel about
> it, and I thought it would be a pain to tease out all the issues
> arising from aborting in different contexts when someone might not
> expect it.
>
> I am thinking later we can have kunit_kmalloc_or_abort variants? And
> then we can punt this issue to a later time?
>
Sure. Sounds good.
^ permalink raw reply
* Re: [PATCH v12 09/18] kunit: test: add support for test abort
From: Stephen Boyd @ 2019-08-13 17:06 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g4415URtJBKPhsEw98GxiExJr-fstW6SQ6nmV9ts9ggK-g@mail.gmail.com>
Quoting Brendan Higgins (2019-08-13 00:52:03)
> On Mon, Aug 12, 2019 at 10:56 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 21:57:55)
> > > On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Brendan Higgins (2019-08-12 11:24:12)
> > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > > > index 2625bcfeb19ac..93381f841e09f 100644
> > > > > --- a/include/kunit/test.h
> > > > > +++ b/include/kunit/test.h
> > > > > @@ -184,6 +191,13 @@ struct kunit {
> > > > > struct list_head resources; /* Protected by lock. */
> > > > > };
> > > > >
> > > > > +static inline void kunit_set_death_test(struct kunit *test, bool death_test)
> > > > > +{
> > > > > + spin_lock(&test->lock);
> > > > > + test->death_test = death_test;
> > > > > + spin_unlock(&test->lock);
> > > > > +}
> > > >
> > > > These getters and setters are using spinlocks again. It doesn't make any
> > > > sense. It probably needs a rework like was done for the other bool
> > > > member, success.
> > >
> > > No, this is intentional. death_test can transition from false to true
> > > and then back to false within the same test. Maybe that deserves a
> > > comment?
> >
> > Yes. How does it transition from true to false again?
>
> The purpose is to tell try_catch that it was expected for the test to
> bail out. Given the default implementation there is no way for this to
> happen aside from abort() being called, but in some implementations it
> is possible to implement a fault catcher which allows a test suite to
> recover from an unexpected failure.
>
> Maybe it would be best to drop this until I add one of those
> alternative implementations.
Ok.
>
> > Either way, having a spinlock around a read/write API doesn't make sense
> > because it just makes sure that two writes don't overlap, but otherwise
> > does nothing to keep things synchronized. For example a set to true
> > after a set to false when the two calls to set true or false aren't
> > synchronized means they can happen in any order. So I don't see how it
> > needs a spinlock. The lock needs to be one level higher.
>
> There shouldn't be any cases where one thread is trying to set it
> while another is trying to unset it. The thing I am worried about here
> is making sure all the cores see the write, and making sure no reads
> or writes get reordered before it. So I guess I just want a fence. So
> I take it I should probably have is a WRITE_ONCE here and READ_ONCE in
> the getter?
>
Are the gets and sets in program order? If so, WRITE_ONCE and READ_ONCE
aren't required. Otherwise, if it's possible for one thread to write it
and another to read it but the threads are ordered with some other
barrier like a completion or lock, then again the macros aren't needed.
It would be good to read memory-barriers.txt to understand when to use
the read/write macros.
^ permalink raw reply
* Re: [RFC 00/19] Integration of Kernel Test Framework (KTF) into the kernel tree
From: Brendan Higgins @ 2019-08-13 17:02 UTC (permalink / raw)
To: Knut Omang
Cc: Greg Kroah-Hartman, open list:KERNEL SELFTEST FRAMEWORK,
Linux Kernel Mailing List, open list:DOCUMENTATION, linux-kbuild,
Shuah Khan, Jonathan Corbet, Masahiro Yamada, Michal Marek,
Shreyans Devendra Doshi, Alan Maguire, Kevin Hilman,
Hidenori Yamaji, Frank Rowand, Timothy Bird, Luis Chamberlain,
Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <f0e1a6efa8f21ab93935c4c19e13b904d4a71f98.camel@oracle.com>
On Tue, Aug 13, 2019 at 2:51 AM Knut Omang <knut.omang@oracle.com> wrote:
>
> On Tue, 2019-08-13 at 10:23 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Aug 13, 2019 at 08:09:15AM +0200, Knut Omang wrote:
> > > and in the making::
> > >
> > > kunit/ (kernel only (UML))
> >
> > You are going to have to integrate this with kunit, to come up with a
> > superset of both in the end.
>
> Yes, I agree - getting to a unified approach has been my intention since I first brought this
> up at LPC'17.
>
> > And I do not think that kunit is only UML, it's just that seems to be
> > what Brendan tests with, but should work with other arches as well.
>
> If I get Brendan right, it is UML only now but can be extended to also support
> kernels running on real hardware. Still it is kernel only, while KTF also has the
> hybrid mode, where a test can have code and assertions both in user mode and kernel mode.
> This is made easier and more streamlined by letting all reporting happen from user mode.
Nope, the KUnit patchset currently under review *does* support any
architecture; we have tested it on x86, ARM, and UML, but it should
work on any architecture.
I added support for that a while ago due to popular demand.
^ permalink raw reply
* [PATCH v3 2/2] rcuperf: Add kfree_rcu performance Tests
From: Joel Fernandes (Google) @ 2019-08-13 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Andrew Morton, byungchul.park,
Davidlohr Bueso, Jonathan Corbet, Josh Triplett, Kees Cook,
kernel-team, kernel-team, Lai Jiangshan, linux-doc,
Mathieu Desnoyers, Mauro Carvalho Chehab, max.byungchul.park,
Paul E. McKenney, Rao Shoaib, rcu, Steven Rostedt,
Thomas Gleixner
In-Reply-To: <20190813170046.81707-1-joel@joelfernandes.org>
This test runs kfree_rcu in a loop to measure performance of the new
kfree_rcu batching functionality.
The following table shows results when booting with arguments:
rcuperf.kfree_loops=200000 rcuperf.kfree_alloc_num=1000 rcuperf.kfree_rcu_test=1
In addition, rcuperf.kfree_no_batch is used to toggle the batching of
kfree_rcu()s for a test run.
rcuperf.kfree_no_batch GPs time (seconds)
0 (default) 1732 15.9
1 9133 14.5
Note that the results are the same for the case:
1. Patch is not applied and rcuperf.kfree_no_batch=0
2. Patch is applied and rcuperf.kfree_no_batch=1
On a 16 CPU system with the above boot parameters, we see that the total
number of grace periods that elapse during the test drops from 9133 when
not batching to 1732 when batching (a ~427% improvement). The
kfree_rcu() flood itself slows down a bit when batching, though, as
shown. This is likely due to rcuperf threads contending with the
additional worker threads that are now running both before (the monitor)
and after (the work done to kfree()) the grace period.
Note that the active memory consumption during the kfree_rcu() flood
does increase to around 300-400MB due to the batching (from around 50MB
without batching). However, this memory consumption is relatively
constant and is just an effect of the buffering. In other words, the
system is able to keep up with the kfree_rcu() load.
Also, when running the test, please disable CONFIG_DEBUG_PREEMPT and
CONFIG_PROVE_RCU for realistic comparisons with/without batching.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/rcu/rcuperf.c | 189 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 181 insertions(+), 8 deletions(-)
diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
index 7a6890b23c5f..70d6ac19cbff 100644
--- a/kernel/rcu/rcuperf.c
+++ b/kernel/rcu/rcuperf.c
@@ -86,6 +86,7 @@ torture_param(bool, shutdown, RCUPERF_SHUTDOWN,
"Shutdown at end of performance tests.");
torture_param(int, verbose, 1, "Enable verbose debugging printk()s");
torture_param(int, writer_holdoff, 0, "Holdoff (us) between GPs, zero to disable");
+torture_param(int, kfree_rcu_test, 0, "Do we run a kfree_rcu perf test?");
static char *perf_type = "rcu";
module_param(perf_type, charp, 0444);
@@ -105,8 +106,8 @@ static atomic_t n_rcu_perf_writer_finished;
static wait_queue_head_t shutdown_wq;
static u64 t_rcu_perf_writer_started;
static u64 t_rcu_perf_writer_finished;
-static unsigned long b_rcu_perf_writer_started;
-static unsigned long b_rcu_perf_writer_finished;
+static unsigned long b_rcu_gp_test_started;
+static unsigned long b_rcu_gp_test_finished;
static DEFINE_PER_CPU(atomic_t, n_async_inflight);
static int rcu_perf_writer_state;
@@ -379,10 +380,10 @@ rcu_perf_writer(void *arg)
if (atomic_inc_return(&n_rcu_perf_writer_started) >= nrealwriters) {
t_rcu_perf_writer_started = t;
if (gp_exp) {
- b_rcu_perf_writer_started =
+ b_rcu_gp_test_started =
cur_ops->exp_completed() / 2;
} else {
- b_rcu_perf_writer_started = cur_ops->get_gp_seq();
+ b_rcu_gp_test_started = cur_ops->get_gp_seq();
}
}
@@ -435,10 +436,10 @@ rcu_perf_writer(void *arg)
PERFOUT_STRING("Test complete");
t_rcu_perf_writer_finished = t;
if (gp_exp) {
- b_rcu_perf_writer_finished =
+ b_rcu_gp_test_finished =
cur_ops->exp_completed() / 2;
} else {
- b_rcu_perf_writer_finished =
+ b_rcu_gp_test_finished =
cur_ops->get_gp_seq();
}
if (shutdown) {
@@ -523,8 +524,8 @@ rcu_perf_cleanup(void)
t_rcu_perf_writer_finished -
t_rcu_perf_writer_started,
ngps,
- rcuperf_seq_diff(b_rcu_perf_writer_finished,
- b_rcu_perf_writer_started));
+ rcuperf_seq_diff(b_rcu_gp_test_finished,
+ b_rcu_gp_test_started));
for (i = 0; i < nrealwriters; i++) {
if (!writer_durations)
break;
@@ -592,6 +593,175 @@ rcu_perf_shutdown(void *arg)
return -EINVAL;
}
+/*
+ * kfree_rcu performance tests: Start a kfree_rcu loop on all CPUs for number
+ * of iterations and measure total time and number of GP for all iterations to complete.
+ */
+
+torture_param(int, kfree_nthreads, -1, "Number of threads running loops of kfree_rcu().");
+torture_param(int, kfree_alloc_num, 8000, "Number of allocations and frees done in an iteration.");
+torture_param(int, kfree_loops, 10, "Number of loops doing kfree_alloc_num allocations and frees.");
+torture_param(int, kfree_no_batch, 0, "Use the non-batching (slower) version of kfree_rcu.");
+
+static struct task_struct **kfree_reader_tasks;
+static int kfree_nrealthreads;
+static atomic_t n_kfree_perf_thread_started;
+static atomic_t n_kfree_perf_thread_ended;
+
+struct kfree_obj {
+ char kfree_obj[8];
+ struct rcu_head rh;
+};
+
+static int
+kfree_perf_thread(void *arg)
+{
+ int i, loop = 0;
+ long me = (long)arg;
+ struct kfree_obj **alloc_ptrs;
+ u64 start_time, end_time;
+
+ VERBOSE_PERFOUT_STRING("kfree_perf_thread task started");
+ set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids));
+ set_user_nice(current, MAX_NICE);
+
+ alloc_ptrs = (struct kfree_obj **)kmalloc(sizeof(struct kfree_obj *) * kfree_alloc_num,
+ GFP_KERNEL);
+ if (!alloc_ptrs)
+ return -ENOMEM;
+
+ start_time = ktime_get_mono_fast_ns();
+
+ if (atomic_inc_return(&n_kfree_perf_thread_started) >= kfree_nrealthreads) {
+ if (gp_exp)
+ b_rcu_gp_test_started = cur_ops->exp_completed() / 2;
+ else
+ b_rcu_gp_test_started = cur_ops->get_gp_seq();
+ }
+
+ do {
+ for (i = 0; i < kfree_alloc_num; i++) {
+ alloc_ptrs[i] = kmalloc(sizeof(struct kfree_obj), GFP_KERNEL);
+ if (!alloc_ptrs[i])
+ return -ENOMEM;
+ }
+
+ for (i = 0; i < kfree_alloc_num; i++) {
+ if (!kfree_no_batch) {
+ kfree_rcu(alloc_ptrs[i], rh);
+ } else {
+ rcu_callback_t cb;
+
+ cb = (rcu_callback_t)(unsigned long)offsetof(struct kfree_obj, rh);
+ kfree_call_rcu_nobatch(&(alloc_ptrs[i]->rh), cb);
+ }
+ }
+
+ cond_resched();
+ } while (!torture_must_stop() && ++loop < kfree_loops);
+
+ if (atomic_inc_return(&n_kfree_perf_thread_ended) >= kfree_nrealthreads) {
+ end_time = ktime_get_mono_fast_ns();
+
+ if (gp_exp)
+ b_rcu_gp_test_finished = cur_ops->exp_completed() / 2;
+ else
+ b_rcu_gp_test_finished = cur_ops->get_gp_seq();
+
+ pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld\n",
+ (unsigned long long)(end_time - start_time), kfree_loops,
+ rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started));
+ if (shutdown) {
+ smp_mb(); /* Assign before wake. */
+ wake_up(&shutdown_wq);
+ }
+ }
+
+ kfree(alloc_ptrs);
+ torture_kthread_stopping("kfree_perf_thread");
+ return 0;
+}
+
+static void
+kfree_perf_cleanup(void)
+{
+ int i;
+
+ if (torture_cleanup_begin())
+ return;
+
+ if (kfree_reader_tasks) {
+ for (i = 0; i < kfree_nrealthreads; i++)
+ torture_stop_kthread(kfree_perf_thread,
+ kfree_reader_tasks[i]);
+ kfree(kfree_reader_tasks);
+ }
+
+ torture_cleanup_end();
+}
+
+/*
+ * shutdown kthread. Just waits to be awakened, then shuts down system.
+ */
+static int
+kfree_perf_shutdown(void *arg)
+{
+ do {
+ wait_event(shutdown_wq,
+ atomic_read(&n_kfree_perf_thread_ended) >=
+ kfree_nrealthreads);
+ } while (atomic_read(&n_kfree_perf_thread_ended) < kfree_nrealthreads);
+
+ smp_mb(); /* Wake before output. */
+
+ kfree_perf_cleanup();
+ kernel_power_off();
+ return -EINVAL;
+}
+
+static int __init
+kfree_perf_init(void)
+{
+ long i;
+ int firsterr = 0;
+
+ kfree_nrealthreads = compute_real(kfree_nthreads);
+ /* Start up the kthreads. */
+ if (shutdown) {
+ init_waitqueue_head(&shutdown_wq);
+ firsterr = torture_create_kthread(kfree_perf_shutdown, NULL,
+ shutdown_task);
+ if (firsterr)
+ goto unwind;
+ schedule_timeout_uninterruptible(1);
+ }
+
+ kfree_reader_tasks = kcalloc(kfree_nrealthreads, sizeof(kfree_reader_tasks[0]),
+ GFP_KERNEL);
+ if (kfree_reader_tasks == NULL) {
+ firsterr = -ENOMEM;
+ goto unwind;
+ }
+
+ for (i = 0; i < kfree_nrealthreads; i++) {
+ firsterr = torture_create_kthread(kfree_perf_thread, (void *)i,
+ kfree_reader_tasks[i]);
+ if (firsterr)
+ goto unwind;
+ }
+
+ while (atomic_read(&n_kfree_perf_thread_started) < kfree_nrealthreads)
+ schedule_timeout_uninterruptible(1);
+
+ torture_init_end();
+ return 0;
+
+unwind:
+ torture_init_end();
+ kfree_perf_cleanup();
+ return firsterr;
+}
+
static int __init
rcu_perf_init(void)
{
@@ -624,6 +794,9 @@ rcu_perf_init(void)
if (cur_ops->init)
cur_ops->init();
+ if (kfree_rcu_test)
+ return kfree_perf_init();
+
nrealwriters = compute_real(nwriters);
nrealreaders = compute_real(nreaders);
atomic_set(&n_rcu_perf_reader_started, 0);
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH v3 1/2] rcu/tree: Add basic support for kfree_rcu batching
From: Joel Fernandes (Google) @ 2019-08-13 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Rao Shoaib, max.byungchul.park,
byungchul.park, kernel-team, kernel-team, Andrew Morton,
Davidlohr Bueso, Jonathan Corbet, Josh Triplett, Kees Cook,
Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, Paul E. McKenney, rcu, Steven Rostedt,
Thomas Gleixner
Recently a discussion about stability and performance of a system
involving a high rate of kfree_rcu() calls surfaced on the list [1]
which led to another discussion how to prepare for this situation.
This patch adds basic batching support for kfree_rcu(). It is "basic"
because we do none of the slab management, dynamic allocation, code
moving or any of the other things, some of which previous attempts did
[2]. These fancier improvements can be follow-up patches and there are
different ideas being discussed in those regards.
Torture tests follow in the next patch and show improvements of around
400% in reduction of number of grace periods on a 16 CPU system. More
details and test data are in that patch.
This is an effort to start simple, and build up from there. In the
future, an extension to use kfree_bulk and possibly per-slab batching
could be done to further improve performance due to cache-locality and
slab-specific bulk free optimizations. By using an array of pointers,
the worker thread processing the work would need to read lesser data
since it does not need to deal with large rcu_head(s) any longer.
There is an implication with rcu_barrier() with this patch. Since the
kfree_rcu() calls can be batched, and may not be handed yet to the RCU
machinery in fact, the monitor may not have even run yet to do the
queue_rcu_work(), there seems no easy way of implementing rcu_barrier()
to wait for those kfree_rcu()s that are already made. So this means a
kfree_rcu() followed by an rcu_barrier() does not imply that memory will
be freed once rcu_barrier() returns.
Another implication is higher active memory usage (although not
run-away..) until the kfree_rcu() flooding ends, in comparison to
without batching. More details about this are in the second patch which
adds an rcuperf test.
Finally, in the near future we will get rid of kfree_rcu special casing
within RCU such as in rcu_do_batch and switch everything to just
batching. Currently we don't do that since timer subsystem is not yet up
and we cannot schedule the kfree_rcu() monitor as the timer subsystem's
lock are not initialized. That would also mean getting rid of
kfree_call_rcu_nobatch() entirely.
[1] http://lore.kernel.org/lkml/20190723035725-mutt-send-email-mst@kernel.org
[2] https://lkml.org/lkml/2017/12/19/824
Cc: Rao Shoaib <rao.shoaib@oracle.com>
Cc: max.byungchul.park@gmail.com
Cc: byungchul.park@lge.com
Cc: kernel-team@android.com
Cc: kernel-team@lge.com
Co-developed-by: Byungchul Park <byungchul.park@lge.com>
Signed-off-by: Byungchul Park <byungchul.park@lge.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
v2->v3: Just some code comment changes thanks to Byungchul.
RFCv1->PATCH v2: Removed limits on the ->head list, just let it grow.
Dropped KFREE_MAX_JIFFIES to HZ/50 from HZ/20 to reduce OOM occurrence.
Removed sleeps in rcuperf test, just using cond_resched()in loop.
Better code comments ;)
.../admin-guide/kernel-parameters.txt | 17 ++
include/linux/rcutiny.h | 5 +
include/linux/rcutree.h | 1 +
kernel/rcu/tree.c | 204 +++++++++++++++++-
4 files changed, 221 insertions(+), 6 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7ccd158b3894..a9156ca5de24 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3895,6 +3895,23 @@
test until boot completes in order to avoid
interference.
+ rcuperf.kfree_rcu_test= [KNL]
+ Set to measure performance of kfree_rcu() flooding.
+
+ rcuperf.kfree_nthreads= [KNL]
+ The number of threads running loops of kfree_rcu().
+
+ rcuperf.kfree_alloc_num= [KNL]
+ Number of allocations and frees done in an iteration.
+
+ rcuperf.kfree_loops= [KNL]
+ Number of loops doing rcuperf.kfree_alloc_num number
+ of allocations and frees.
+
+ rcuperf.kfree_no_batch= [KNL]
+ Use the non-batching (slower) version of kfree_rcu.
+ This is useful for comparing with the batched version.
+
rcuperf.nreaders= [KNL]
Set number of RCU readers. The value -1 selects
N, where N is the number of CPUs. A value
diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
index 8e727f57d814..383f2481750f 100644
--- a/include/linux/rcutiny.h
+++ b/include/linux/rcutiny.h
@@ -39,6 +39,11 @@ static inline void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
call_rcu(head, func);
}
+static inline void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func)
+{
+ call_rcu(head, func);
+}
+
void rcu_qs(void);
static inline void rcu_softirq_qs(void)
diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h
index 735601ac27d3..7e38b39ec634 100644
--- a/include/linux/rcutree.h
+++ b/include/linux/rcutree.h
@@ -34,6 +34,7 @@ static inline void rcu_virt_note_context_switch(int cpu)
void synchronize_rcu_expedited(void);
void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func);
+void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func);
void rcu_barrier(void);
bool rcu_eqs_special_set(int cpu);
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index a14e5fbbea46..3e62bbb0e1fa 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2593,17 +2593,195 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func)
}
EXPORT_SYMBOL_GPL(call_rcu);
+
+/* Maximum number of jiffies to wait before draining a batch. */
+#define KFREE_DRAIN_JIFFIES (HZ / 50)
+
/*
- * Queue an RCU callback for lazy invocation after a grace period.
- * This will likely be later named something like "call_rcu_lazy()",
- * but this change will require some way of tagging the lazy RCU
- * callbacks in the list of pending callbacks. Until then, this
- * function may only be called from __kfree_rcu().
+ * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
+ * kfree(s) is queued for freeing after a grace period, right away.
*/
-void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
+struct kfree_rcu_cpu {
+ /* The rcu_work node for queuing work with queue_rcu_work(). The work
+ * is done after a grace period.
+ */
+ struct rcu_work rcu_work;
+
+ /* The list of objects being queued in a batch but are not yet
+ * scheduled to be freed.
+ */
+ struct rcu_head *head;
+
+ /* The list of objects that have now left ->head and are queued for
+ * freeing after a grace period.
+ */
+ struct rcu_head *head_free;
+
+ /* Protect concurrent access to this structure. */
+ spinlock_t lock;
+
+ /* The delayed work that flushes ->head to ->head_free incase ->head
+ * within KFREE_DRAIN_JIFFIES. In case flushing cannot be done if RCU
+ * is busy, ->head just continues to grow and we retry flushing later.
+ */
+ struct delayed_work monitor_work;
+ bool monitor_todo; /* Is a delayed work pending execution? */
+};
+
+static DEFINE_PER_CPU(struct kfree_rcu_cpu, krc);
+
+/*
+ * This function is invoked in workqueue context after a grace period.
+ * It frees all the objects queued on ->head_free.
+ */
+static void kfree_rcu_work(struct work_struct *work)
+{
+ unsigned long flags;
+ struct rcu_head *head, *next;
+ struct kfree_rcu_cpu *krcp = container_of(to_rcu_work(work),
+ struct kfree_rcu_cpu, rcu_work);
+
+ spin_lock_irqsave(&krcp->lock, flags);
+ head = krcp->head_free;
+ krcp->head_free = NULL;
+ spin_unlock_irqrestore(&krcp->lock, flags);
+
+ /*
+ * The head is detached and not referenced from anywhere, so lockless
+ * access is Ok.
+ */
+ for (; head; head = next) {
+ next = head->next;
+ head->next = NULL;
+ /* Could be possible to optimize with kfree_bulk in future */
+ __rcu_reclaim(rcu_state.name, head);
+ cond_resched_tasks_rcu_qs();
+ }
+}
+
+/*
+ * Schedule the kfree batch RCU work to run in workqueue context after a GP.
+ *
+ * This function is invoked by kfree_rcu_monitor() when the KFREE_DRAIN_JIFFIES
+ * timeout has been reached.
+ */
+static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
+{
+ lockdep_assert_held(&krcp->lock);
+
+ /* If a previous RCU batch work is already in progress, we cannot queue
+ * another one, just refuse the optimization and it will be retried
+ * again in KFREE_DRAIN_JIFFIES time.
+ */
+ if (krcp->head_free)
+ return false;
+
+ krcp->head_free = krcp->head;
+ krcp->head = NULL;
+ INIT_RCU_WORK(&krcp->rcu_work, kfree_rcu_work);
+ queue_rcu_work(system_wq, &krcp->rcu_work);
+
+ return true;
+}
+
+static inline void kfree_rcu_drain_unlock(struct kfree_rcu_cpu *krcp,
+ unsigned long flags)
+{
+ /* Flush ->head to ->head_free, all objects on ->head_free will be
+ * kfree'd after a grace period.
+ */
+ if (queue_kfree_rcu_work(krcp)) {
+ /* Success! Our job is done here. */
+ spin_unlock_irqrestore(&krcp->lock, flags);
+ return;
+ }
+
+ /* Previous batch did not get free yet, let us try again soon. */
+ if (krcp->monitor_todo == false) {
+ schedule_delayed_work_on(smp_processor_id(),
+ &krcp->monitor_work, KFREE_DRAIN_JIFFIES);
+ krcp->monitor_todo = true;
+ }
+ spin_unlock_irqrestore(&krcp->lock, flags);
+}
+
+/*
+ * This function is invoked after the KFREE_DRAIN_JIFFIES timeout has elapsed,
+ * so it drains the specified kfree_rcu_cpu structure's ->head list.
+ */
+static void kfree_rcu_monitor(struct work_struct *work)
+{
+ bool todo;
+ unsigned long flags;
+ struct kfree_rcu_cpu *krcp = container_of(work, struct kfree_rcu_cpu,
+ monitor_work.work);
+
+ spin_lock_irqsave(&krcp->lock, flags);
+ todo = krcp->monitor_todo;
+ krcp->monitor_todo = false;
+ if (todo)
+ kfree_rcu_drain_unlock(krcp, flags);
+ else
+ spin_unlock_irqrestore(&krcp->lock, flags);
+}
+
+/*
+ * This version of kfree_call_rcu does not do batching of kfree_rcu() requests.
+ * Used only by rcuperf torture test for comparison with kfree_rcu_batch().
+ */
+void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func)
{
__call_rcu(head, func, -1, 1);
}
+EXPORT_SYMBOL_GPL(kfree_call_rcu_nobatch);
+
+/*
+ * Queue a request for lazy invocation of kfree() after a grace period.
+ *
+ * Each kfree_call_rcu() request is added to a batch. The batch will be drained
+ * every KFREE_DRAIN_JIFFIES number of jiffies. All the objects in the batch
+ * will be kfree'd in workqueue context. This allows us to:
+ *
+ * 1. Batch requests together to reduce the number of grace periods during
+ * heavy kfree_rcu() load.
+ *
+ * 2. In the future, makes it possible to use kfree_bulk() on a large number of
+ * kfree_rcu() requests thus reducing the per-object overhead of kfree() and
+ * also reducing cache misses.
+ */
+void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
+{
+ unsigned long flags;
+ struct kfree_rcu_cpu *krcp;
+ bool monitor_todo;
+
+ /* kfree_call_rcu() batching requires timers to be up. If the scheduler
+ * is not yet up, just skip batching and do non-batched kfree_call_rcu().
+ */
+ if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING)
+ return kfree_call_rcu_nobatch(head, func);
+
+ local_irq_save(flags);
+ krcp = this_cpu_ptr(&krc);
+
+ spin_lock(&krcp->lock);
+
+ /* Queue the kfree but don't yet schedule the batch. */
+ head->func = func;
+ head->next = krcp->head;
+ krcp->head = head;
+
+ /* Schedule monitor for timely drain after KFREE_DRAIN_JIFFIES. */
+ monitor_todo = krcp->monitor_todo;
+ krcp->monitor_todo = true;
+ spin_unlock(&krcp->lock);
+
+ if (!monitor_todo) {
+ schedule_delayed_work_on(smp_processor_id(),
+ &krcp->monitor_work, KFREE_DRAIN_JIFFIES);
+ }
+ local_irq_restore(flags);
+}
EXPORT_SYMBOL_GPL(kfree_call_rcu);
/*
@@ -3455,10 +3633,24 @@ static void __init rcu_dump_rcu_node_tree(void)
struct workqueue_struct *rcu_gp_wq;
struct workqueue_struct *rcu_par_gp_wq;
+static void __init kfree_rcu_batch_init(void)
+{
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
+
+ spin_lock_init(&krcp->lock);
+ INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
+ }
+}
+
void __init rcu_init(void)
{
int cpu;
+ kfree_rcu_batch_init();
+
rcu_early_boot_tests();
rcu_bootup_announce();
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* Re: [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder
From: Stephen Boyd @ 2019-08-13 16:48 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g452+-6m1eiVK0ccTDkJ2wH8GBwxRDw5owwC8h3NscE1ag@mail.gmail.com>
Quoting Brendan Higgins (2019-08-13 02:12:54)
> On Tue, Aug 13, 2019 at 2:04 AM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-08-12 22:02:59)
> > > > However, now that I added the kunit_resource_destroy, I thought it
> > > > might be good to free the string_stream after I use it in each call to
> > > > kunit_assert->format(...) in which case I will be using this logic.
> > > >
> > > > So I think the right thing to do is to expose string_stream_destroy so
> > > > kunit_do_assert can clean up when it's done, and then demote
> > > > string_stream_clear to static. Sound good?
> > >
> > > Ok, sure. I don't really see how clearing it explicitly when the
> > > assertion prints vs. never allocating it to begin with is really any
> > > different. Maybe I've missed something though.
> >
> > It's for the case that we *do* print something out. Once we are doing
> > printing, we don't want the fragments anymore.
>
> Oops, sorry fat fingered: s/doing/done
Yes, but when we print something out we've run into some sort of problem
and then the test is over. So freeing the memory when it fails vs. when
the test is over seems like a minor difference. Or is it also used to
print other informational messages while the test is running?
I'm not particularly worried here, just trying to see if less code is
possible.
^ permalink raw reply
* Re: [RFC 01/19] kbuild: Fixes to rules for host-cshlib and host-cxxshlib
From: Knut Omang @ 2019-08-13 16:19 UTC (permalink / raw)
To: Masahiro Yamada
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
open list:DOCUMENTATION, Linux Kbuild mailing list, Shuah Khan,
Jonathan Corbet, Michal Marek, Greg Kroah-Hartman,
Shreyans Devendra Doshi, Alan Maguire, Brendan Higgins,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <CAK7LNASX4jPRxRxD+JafAfKqjck=x27HuHZgPV1VFfW8MzcwZA@mail.gmail.com>
On Tue, 2019-08-13 at 23:01 +0900, Masahiro Yamada wrote:
> On Tue, Aug 13, 2019 at 3:13 PM Knut Omang <knut.omang@oracle.com> wrote:
> >
> > C++ libraries interfacing to C APIs might sometimes need some glue
> > logic more easily written in C.
> > Allow a C++ library to also contain 0 or more C objects.
> >
> > Also fix rules for both C and C++ shared libraries:
> > - C++ shared libraries depended on .c instead of .cc files
> > - Rules were referenced as -objs instead of the intended
> > -cobjs and -cxxobjs following the pattern from hostprogs*.
> >
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
>
>
> How is this patch related to the rest of this series?
This is just my (likely naive) way I to get what I had working
using autotools in the Github version of KTF) translated into something
comparable using kbuild only. We need to build a shared library consisting
of a few C++ files and a very simple C file, and a couple of simple binaries,
and the rule in there does seem to take .c files and subject them to the
C++ compiler, which makes this difficult to achieve?
> This patch breaks GCC-plugins.
> Did you really compile-test this patch before the submission?
Sorry for my ignorance here:
I ran through the kernel build and installed the resulting kernel
on a VM that I used to test this, if that's what you are asking
about?
Do I need some unusual .config options or run a special make target
to trigger the problem you see?
I used a recent Fedora config with default values for new options,
and ran the normal default make target (also with O=) and make selftests
to test the patch itself.
Thanks,
Knut
> --
> Best Regards
>
> Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Jann Horn @ 2019-08-13 15:40 UTC (permalink / raw)
To: Joel Fernandes
Cc: kernel list, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen,
Daniel Colascione, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, Linux API, linux-doc,
linux-fsdevel, Linux-MM, Michal Hocko, Mike Rapoport, Minchan Kim,
namhyung, Paul E. McKenney, Robin Murphy, Roman Gushchin,
Stephen Rothwell, Suren Baghdasaryan, Thomas Gleixner, Todd Kjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190813153020.GC14622@google.com>
On Tue, Aug 13, 2019 at 5:30 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> On Mon, Aug 12, 2019 at 08:14:38PM +0200, Jann Horn wrote:
> [snip]
> > > +/* Helper to get the start and end frame given a pos and count */
> > > +static int page_idle_get_frames(loff_t pos, size_t count, struct mm_struct *mm,
> > > + unsigned long *start, unsigned long *end)
> > > +{
> > > + unsigned long max_frame;
> > > +
> > > + /* If an mm is not given, assume we want physical frames */
> > > + max_frame = mm ? (mm->task_size >> PAGE_SHIFT) : max_pfn;
> > > +
> > > + if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
> > > + return -EINVAL;
> > > +
> > > + *start = pos * BITS_PER_BYTE;
> > > + if (*start >= max_frame)
> > > + return -ENXIO;
> > > +
> > > + *end = *start + count * BITS_PER_BYTE;
> > > + if (*end > max_frame)
> > > + *end = max_frame;
> > > + return 0;
> > > +}
> >
> > You could add some overflow checks for the multiplications. I haven't
> > seen any place where it actually matters, but it seems unclean; and in
> > particular, on a 32-bit architecture where the maximum user address is
> > very high (like with a 4G:4G split), it looks like this function might
> > theoretically return with `*start > *end`, which could be confusing to
> > callers.
>
> I could store the multiplication result in unsigned long long (since we are
> bounds checking with max_frame, start > end would not occur). Something like
> the following (with extraneous casts). But I'll think some more about the
> point you are raising.
check_mul_overflow() exists and could make that a bit cleaner.
> > This means that BITMAP_CHUNK_SIZE is UAPI on big-endian systems,
> > right? My opinion is that it would be slightly nicer to design the
> > UAPI such that incrementing virtual addresses are mapped to
> > incrementing offsets in the buffer (iow, either use bytewise access or
> > use little-endian), but I'm not going to ask you to redesign the UAPI
> > this late.
>
> That would also be slow and consume more memory in userspace buffers.
> Currently, a 64-bit (8 byte) chunk accounts for 64 pages worth or 256KB.
I still wanted to use one bit per page; I just wanted to rearrange the
bits. So the first byte would always contain 8 bits corresponding to
the first 8 pages, instead of corresponding to pages 56-63 on some
systems depending on endianness. Anyway, this is a moot point, since
as you said...
> Also I wanted to keep the interface consistent with the global
> /sys/kernel/mm/page_idle interface.
Sorry, I missed that this is already UAPI in the global interface. I
agree, using a different API for the per-process interface would be a
bad idea.
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Daniel Gruss @ 2019-08-13 15:34 UTC (permalink / raw)
To: Jann Horn, Michal Hocko, Joel Fernandes (Google)
Cc: kernel list, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen,
Daniel Colascione, fmayer, H. Peter Anvin, Ingo Molnar,
Joel Fernandes, Jonathan Corbet, Kees Cook, kernel-team,
Linux API, linux-doc, linux-fsdevel, Linux-MM, Mike Rapoport,
Minchan Kim, namhyung, Paul E. McKenney, Robin Murphy,
Roman Gushchin, Stephen Rothwell, Suren Baghdasaryan,
Thomas Gleixner, Todd Kjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <CAG48ez2cuqe_VYhhaqw8Hcyswv47cmz2XmkqNdvkXEhokMVaXg@mail.gmail.com>
On 8/13/19 5:29 PM, Jann Horn wrote:
> On Tue, Aug 13, 2019 at 12:09 PM Michal Hocko <mhocko@kernel.org> wrote:
>> On Mon 12-08-19 20:14:38, Jann Horn wrote:
>>> On Wed, Aug 7, 2019 at 7:16 PM Joel Fernandes (Google)
>>> <joel@joelfernandes.org> wrote:
>>>> The page_idle tracking feature currently requires looking up the pagemap
>>>> for a process followed by interacting with /sys/kernel/mm/page_idle.
>>>> Looking up PFN from pagemap in Android devices is not supported by
>>>> unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
>>>>
>>>> This patch adds support to directly interact with page_idle tracking at
>>>> the PID level by introducing a /proc/<pid>/page_idle file. It follows
>>>> the exact same semantics as the global /sys/kernel/mm/page_idle, but now
>>>> looking up PFN through pagemap is not needed since the interface uses
>>>> virtual frame numbers, and at the same time also does not require
>>>> SYS_ADMIN.
>>>>
>>>> In Android, we are using this for the heap profiler (heapprofd) which
>>>> profiles and pin points code paths which allocates and leaves memory
>>>> idle for long periods of time. This method solves the security issue
>>>> with userspace learning the PFN, and while at it is also shown to yield
>>>> better results than the pagemap lookup, the theory being that the window
>>>> where the address space can change is reduced by eliminating the
>>>> intermediate pagemap look up stage. In virtual address indexing, the
>>>> process's mmap_sem is held for the duration of the access.
>>>
>>> What happens when you use this interface on shared pages, like memory
>>> inherited from the zygote, library file mappings and so on? If two
>>> profilers ran concurrently for two different processes that both map
>>> the same libraries, would they end up messing up each other's data?
>>
>> Yup PageIdle state is shared. That is the page_idle semantic even now
>> IIRC.
>>
>>> Can this be used to observe which library pages other processes are
>>> accessing, even if you don't have access to those processes, as long
>>> as you can map the same libraries? I realize that there are already a
>>> bunch of ways to do that with side channels and such; but if you're
>>> adding an interface that allows this by design, it seems to me like
>>> something that should be gated behind some sort of privilege check.
>>
>> Hmm, you need to be priviledged to get the pfn now and without that you
>> cannot get to any page so the new interface is weakening the rules.
>> Maybe we should limit setting the idle state to processes with the write
>> status. Or do you think that even observing idle status is useful for
>> practical side channel attacks? If yes, is that a problem of the
>> profiler which does potentially dangerous things?
>
> I suppose read-only access isn't a real problem as long as the
> profiler isn't writing the idle state in a very tight loop... but I
> don't see a usecase where you'd actually want that? As far as I can
> tell, if you can't write the idle state, being able to read it is
> pretty much useless.
>
> If the profiler only wants to profile process-private memory, then
> that should be implementable in a safe way in principle, I think, but
> since Joel said that they want to profile CoW memory as well, I think
> that's inherently somewhat dangerous.
I agree that allowing profiling of shared pages would leak information.
To me the use case is not entirely clear. This is not a feature that
would normally be run in everyday computer usage, right?
^ permalink raw reply
* Re: [PATCH v5 2/6] mm/page_idle: Add support for handling swapped PG_Idle pages
From: Joel Fernandes @ 2019-08-13 15:36 UTC (permalink / raw)
To: Michal Hocko, khlebnikov
Cc: linux-kernel, Minchan Kim, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Kees Cook, kernel-team, linux-api, linux-doc, linux-fsdevel,
linux-mm, Mike Rapoport, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190813150450.GN17933@dhcp22.suse.cz>
On Tue, Aug 13, 2019 at 05:04:50PM +0200, Michal Hocko wrote:
> On Wed 07-08-19 13:15:55, Joel Fernandes (Google) wrote:
> > Idle page tracking currently does not work well in the following
> > scenario:
> > 1. mark page-A idle which was present at that time.
> > 2. run workload
> > 3. page-A is not touched by workload
> > 4. *sudden* memory pressure happen so finally page A is finally swapped out
> > 5. now see the page A - it appears as if it was accessed (pte unmapped
> > so idle bit not set in output) - but it's incorrect.
> >
> > To fix this, we store the idle information into a new idle bit of the
> > swap PTE during swapping of anonymous pages.
> >
> > Also in the future, madvise extensions will allow a system process
> > manager (like Android's ActivityManager) to swap pages out of a process
> > that it knows will be cold. To an external process like a heap profiler
> > that is doing idle tracking on another process, this procedure will
> > interfere with the idle page tracking similar to the above steps.
>
> This could be solved by checking the !present/swapped out pages
> right? Whoever decided to put the page out to the swap just made it
> idle effectively. So the monitor can make some educated guess for
> tracking. If that is fundamentally not possible then please describe
> why.
But the monitoring process (profiler) does not have control over the 'whoever
made it effectively idle' process.
As you said it will be a guess, it will not be accurate.
I am curious what is your concern with using a bit in the swap PTE?
(Adding Konstantin as well since we may be interested in this, since we also
suggested this idea).
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Joel Fernandes @ 2019-08-13 15:30 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen,
Daniel Colascione, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, Linux API, linux-doc,
linux-fsdevel, Linux-MM, Michal Hocko, Mike Rapoport, Minchan Kim,
namhyung, Paul E. McKenney, Robin Murphy, Roman Gushchin,
Stephen Rothwell, Suren Baghdasaryan, Thomas Gleixner, Todd Kjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <CAG48ez0ysprvRiENhBkLeV9YPTN_MB18rbu2HDa2jsWo5FYR8g@mail.gmail.com>
On Mon, Aug 12, 2019 at 08:14:38PM +0200, Jann Horn wrote:
[snip]
> > +/* Helper to get the start and end frame given a pos and count */
> > +static int page_idle_get_frames(loff_t pos, size_t count, struct mm_struct *mm,
> > + unsigned long *start, unsigned long *end)
> > +{
> > + unsigned long max_frame;
> > +
> > + /* If an mm is not given, assume we want physical frames */
> > + max_frame = mm ? (mm->task_size >> PAGE_SHIFT) : max_pfn;
> > +
> > + if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
> > + return -EINVAL;
> > +
> > + *start = pos * BITS_PER_BYTE;
> > + if (*start >= max_frame)
> > + return -ENXIO;
> > +
> > + *end = *start + count * BITS_PER_BYTE;
> > + if (*end > max_frame)
> > + *end = max_frame;
> > + return 0;
> > +}
>
> You could add some overflow checks for the multiplications. I haven't
> seen any place where it actually matters, but it seems unclean; and in
> particular, on a 32-bit architecture where the maximum user address is
> very high (like with a 4G:4G split), it looks like this function might
> theoretically return with `*start > *end`, which could be confusing to
> callers.
I could store the multiplication result in unsigned long long (since we are
bounds checking with max_frame, start > end would not occur). Something like
the following (with extraneous casts). But I'll think some more about the
point you are raising.
diff --git a/mm/page_idle.c b/mm/page_idle.c
index 1ea21bbb36cb..8fd7a5559986 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -135,6 +135,7 @@ static int page_idle_get_frames(loff_t pos, size_t count, struct mm_struct *mm,
unsigned long *start, unsigned long *end)
{
unsigned long max_frame;
+ unsigned long long tmp;
/* If an mm is not given, assume we want physical frames */
max_frame = mm ? (mm->task_size >> PAGE_SHIFT) : max_pfn;
@@ -142,13 +143,16 @@ static int page_idle_get_frames(loff_t pos, size_t count, struct mm_struct *mm,
if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
return -EINVAL;
- *start = pos * BITS_PER_BYTE;
- if (*start >= max_frame)
+ tmp = pos * BITS_PER_BYTE;
+ if (tmp >= (unsigned long long)max_frame)
return -ENXIO;
+ *start = (unsigned long)tmp;
- *end = *start + count * BITS_PER_BYTE;
- if (*end > max_frame)
+ tmp = *start + count * BITS_PER_BYTE;
+ if (tmp > (unsigned long long)max_frame)
*end = max_frame;
+ else
+ *end = (unsigned long)tmp;
return 0;
}
> [...]
> > for (; pfn < end_pfn; pfn++) {
> > bit = pfn % BITMAP_CHUNK_BITS;
> > if (!bit)
> > *out = 0ULL;
> > - page = page_idle_get_page(pfn);
> > - if (page) {
> > - if (page_is_idle(page)) {
> > - /*
> > - * The page might have been referenced via a
> > - * pte, in which case it is not idle. Clear
> > - * refs and recheck.
> > - */
> > - page_idle_clear_pte_refs(page);
> > - if (page_is_idle(page))
> > - *out |= 1ULL << bit;
> > - }
> > + page = page_idle_get_page_pfn(pfn);
> > + if (page && page_idle_pte_check(page)) {
> > + *out |= 1ULL << bit;
> > put_page(page);
> > }
>
> The `page && !page_idle_pte_check(page)` case looks like it's missing
> a put_page(); you probably intended to write something like this?
>
> page = page_idle_get_page_pfn(pfn);
> if (page) {
> if (page_idle_pte_check(page))
> *out |= 1ULL << bit;
> put_page(page);
> }
Ah, you're right. Will fix, good eyes and thank you!
> [...]
> > +/* page_idle tracking for /proc/<pid>/page_idle */
> > +
> > +struct page_node {
> > + struct page *page;
> > + unsigned long addr;
> > + struct list_head list;
> > +};
> > +
> > +struct page_idle_proc_priv {
> > + unsigned long start_addr;
> > + char *buffer;
> > + int write;
> > +
> > + /* Pre-allocate and provide nodes to pte_page_idle_proc_add() */
> > + struct page_node *page_nodes;
> > + int cur_page_node;
> > + struct list_head *idle_page_list;
> > +};
>
> A linked list is a weird data structure to use if the list elements
> are just consecutive array elements.
Not all of the pages will be considered in the later parts of the code, some
pages are skipped.
However, in v4 I added an array to allocate all the page_node structures,
since Andrew did not want GFP_ATOMIC allocations of individual list nodes.
So I think I could get rid of the linked list and leave the unused
page_node(s) as NULL, but I don't know I have to check if that is possible. I
could be missing a corner case, regardless I'll make a note of this and try!
> > +/*
> > + * Add page to list to be set as idle later.
> > + */
> > +static void pte_page_idle_proc_add(struct page *page,
> > + unsigned long addr, struct mm_walk *walk)
> > +{
> > + struct page *page_get = NULL;
> > + struct page_node *pn;
> > + int bit;
> > + unsigned long frames;
> > + struct page_idle_proc_priv *priv = walk->private;
> > + u64 *chunk = (u64 *)priv->buffer;
> > +
> > + if (priv->write) {
> > + VM_BUG_ON(!page);
> > +
> > + /* Find whether this page was asked to be marked */
> > + frames = (addr - priv->start_addr) >> PAGE_SHIFT;
> > + bit = frames % BITMAP_CHUNK_BITS;
> > + chunk = &chunk[frames / BITMAP_CHUNK_BITS];
> > + if (((*chunk >> bit) & 1) == 0)
> > + return;
>
> This means that BITMAP_CHUNK_SIZE is UAPI on big-endian systems,
> right? My opinion is that it would be slightly nicer to design the
> UAPI such that incrementing virtual addresses are mapped to
> incrementing offsets in the buffer (iow, either use bytewise access or
> use little-endian), but I'm not going to ask you to redesign the UAPI
> this late.
That would also be slow and consume more memory in userspace buffers.
Currently, a 64-bit (8 byte) chunk accounts for 64 pages worth or 256KB.
Also I wanted to keep the interface consistent with the global
/sys/kernel/mm/page_idle interface.
> [...]
> > +ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
> > + size_t count, loff_t *pos, int write)
> > +{
> [...]
> > + down_read(&mm->mmap_sem);
> [...]
> > +
> > + if (!write && !walk_error)
> > + ret = copy_to_user(ubuff, buffer, count);
> > +
> > + up_read(&mm->mmap_sem);
>
> I'd move the up_read() above the copy_to_user(); copy_to_user() can
> block, and there's no reason to hold the mmap_sem across
> copy_to_user().
Will do.
> Sorry about only chiming in at v5 with all this.
No problem, I'm glad you did!
thanks,
- Joel
^ permalink raw reply related
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Jann Horn @ 2019-08-13 15:29 UTC (permalink / raw)
To: Michal Hocko, Daniel Gruss, Joel Fernandes (Google)
Cc: kernel list, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen,
Daniel Colascione, fmayer, H. Peter Anvin, Ingo Molnar,
Joel Fernandes, Jonathan Corbet, Kees Cook, kernel-team,
Linux API, linux-doc, linux-fsdevel, Linux-MM, Mike Rapoport,
Minchan Kim, namhyung, Paul E. McKenney, Robin Murphy,
Roman Gushchin, Stephen Rothwell, Suren Baghdasaryan,
Thomas Gleixner, Todd Kjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <20190813100856.GF17933@dhcp22.suse.cz>
On Tue, Aug 13, 2019 at 12:09 PM Michal Hocko <mhocko@kernel.org> wrote:
> On Mon 12-08-19 20:14:38, Jann Horn wrote:
> > On Wed, Aug 7, 2019 at 7:16 PM Joel Fernandes (Google)
> > <joel@joelfernandes.org> wrote:
> > > The page_idle tracking feature currently requires looking up the pagemap
> > > for a process followed by interacting with /sys/kernel/mm/page_idle.
> > > Looking up PFN from pagemap in Android devices is not supported by
> > > unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
> > >
> > > This patch adds support to directly interact with page_idle tracking at
> > > the PID level by introducing a /proc/<pid>/page_idle file. It follows
> > > the exact same semantics as the global /sys/kernel/mm/page_idle, but now
> > > looking up PFN through pagemap is not needed since the interface uses
> > > virtual frame numbers, and at the same time also does not require
> > > SYS_ADMIN.
> > >
> > > In Android, we are using this for the heap profiler (heapprofd) which
> > > profiles and pin points code paths which allocates and leaves memory
> > > idle for long periods of time. This method solves the security issue
> > > with userspace learning the PFN, and while at it is also shown to yield
> > > better results than the pagemap lookup, the theory being that the window
> > > where the address space can change is reduced by eliminating the
> > > intermediate pagemap look up stage. In virtual address indexing, the
> > > process's mmap_sem is held for the duration of the access.
> >
> > What happens when you use this interface on shared pages, like memory
> > inherited from the zygote, library file mappings and so on? If two
> > profilers ran concurrently for two different processes that both map
> > the same libraries, would they end up messing up each other's data?
>
> Yup PageIdle state is shared. That is the page_idle semantic even now
> IIRC.
>
> > Can this be used to observe which library pages other processes are
> > accessing, even if you don't have access to those processes, as long
> > as you can map the same libraries? I realize that there are already a
> > bunch of ways to do that with side channels and such; but if you're
> > adding an interface that allows this by design, it seems to me like
> > something that should be gated behind some sort of privilege check.
>
> Hmm, you need to be priviledged to get the pfn now and without that you
> cannot get to any page so the new interface is weakening the rules.
> Maybe we should limit setting the idle state to processes with the write
> status. Or do you think that even observing idle status is useful for
> practical side channel attacks? If yes, is that a problem of the
> profiler which does potentially dangerous things?
I suppose read-only access isn't a real problem as long as the
profiler isn't writing the idle state in a very tight loop... but I
don't see a usecase where you'd actually want that? As far as I can
tell, if you can't write the idle state, being able to read it is
pretty much useless.
If the profiler only wants to profile process-private memory, then
that should be implementable in a safe way in principle, I think, but
since Joel said that they want to profile CoW memory as well, I think
that's inherently somewhat dangerous.
^ permalink raw reply
* Re: [PATCH v2] driver/core: Fix build error when SRCU and lockdep disabled
From: Greg Kroah-Hartman @ 2019-08-13 15:25 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, kernel-team, kbuild test robot, Josh Triplett,
Lai Jiangshan, linux-doc, Mathieu Desnoyers, Paul E. McKenney,
Rafael J. Wysocki, rcu, Steven Rostedt
In-Reply-To: <20190813134014.GB258732@google.com>
On Tue, Aug 13, 2019 at 09:40:14AM -0400, Joel Fernandes wrote:
> On Tue, Aug 13, 2019 at 09:39:05AM -0400, Joel Fernandes wrote:
> [snip]
> > > > drivers/base/core.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 32cf83d1c744..c22271577c84 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -97,10 +97,12 @@ void device_links_read_unlock(int not_used)
> > > > up_read(&device_links_lock);
> > > > }
> > > >
> > > > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > > int device_links_read_lock_held(void)
> > > > {
> > > > - return lock_is_held(&device_links_lock);
> > > > + return lock_is_held(&(device_links_lock.dep_map));
> > > > }
> > > > +#endif
> > >
> > > I don't know what the original code looks like here, but I'm guessing
> > > that some .h file will need to be fixed up as you are just preventing
> > > this function from ever being present without that option enabled?
> >
> > No, it doesn't. I already thought about that and it is not an issue. I know
> > this may be confusing because the patch I am fixing is not yet in mainline
> > but in -rcu dev branch, however I did CC you on that RCU patch before but
> > understandably it is not in the series so it was harder to review.
> >
> > Let me explain, the lock checking facility that this patch uses is here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=28875945ba98d1b47a8a706812b6494d165bb0a0
> >
> > If you see, the CONFIG_PROVE_RCU_LIST defines an alternate __list_check_rcu()
> > which is just a NOOP if CONFIG_PROVE_RCU_LIST=n.
> >
> > CONFIG_PROVE_RCU_LIST depends on CONFIG_PROVE_RCU which is def_bool on
> > CONFIG_PROVE_LOCKING which selects CONFIG_DEBUG_LOCK_ALLOC.
> >
> > So there cannot be a scenario where CONFIG_PROVE_RCU_LIST is enabled but
> > CONFIG_DEBUG_LOCK_ALLOC is disabled.
> >
> > To verify this, one could clone the RCU tree's dev branch and do this:
> >
> > Initially PROVE_RCU_LIST is not in config:
> >
> > joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> >
> > Neither is DEBUG_LOCK_ALLOC:
> >
> > joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
> > # CONFIG_DEBUG_LOCK_ALLOC is not set
> >
> > Enable all configs:
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_RCU_EXPERT
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_LOCKING
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -e CONFIG_PROVE_RCU_LIST
> > joelaf@joelaf:~/repo/linux-master$ make olddefconfig
> >
> > DEBUG_LOCK_ALLOC shows up:
> >
> > joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock_all .config
> > CONFIG_DEBUG_LOCK_ALLOC=y
> >
> > So does PROVE_RCU options:
> > joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> > CONFIG_PROVE_RCU=y
> > CONFIG_PROVE_RCU_LIST=y
> >
> > Now, force disable DEBUG_LOCK_ALLOC:
> >
> > joelaf@joelaf:~/repo/linux-master$ ./scripts/config -d CONFIG_DEBUG_LOCK_ALLOC
> >
> > joelaf@joelaf:~/repo/linux-master$ make olddefconfig
> >
> > Options are still enabled:
> >
> > joelaf@joelaf:~/repo/linux-master$ grep -i debug_lock .config
> > CONFIG_DEBUG_LOCK_ALLOC=y
> > joelaf@joelaf:~/repo/linux-master$ grep -i prove_rcu .config
> > CONFIG_PROVE_RCU=y
> > CONFIG_PROVE_RCU_LIST=y
>
> Also, appreciate if you could Ack the fix ;-) (assuming the newlines in
> commit message are fixed).
I don't know enough here to ack it, given that I can't even remember the
original patch...
If you think it's ok, that's fine with me, you can deal with the fallout
if it fails later :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Jann Horn @ 2019-08-13 15:19 UTC (permalink / raw)
To: Joel Fernandes, Daniel Gruss
Cc: Michal Hocko, kernel list, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
Daniel Colascione, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, Linux API, linux-doc,
linux-fsdevel, Linux-MM, Mike Rapoport, Minchan Kim, namhyung,
Paul E. McKenney, Robin Murphy, Roman Gushchin, Stephen Rothwell,
Suren Baghdasaryan, Thomas Gleixner, Todd Kjos, Vladimir Davydov,
Vlastimil Babka, Will Deacon
In-Reply-To: <20190813142527.GD258732@google.com>
On Tue, Aug 13, 2019 at 4:25 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> On Tue, Aug 13, 2019 at 12:08:56PM +0200, Michal Hocko wrote:
> > On Mon 12-08-19 20:14:38, Jann Horn wrote:
> > > On Wed, Aug 7, 2019 at 7:16 PM Joel Fernandes (Google)
> > > <joel@joelfernandes.org> wrote:
> > > > The page_idle tracking feature currently requires looking up the pagemap
> > > > for a process followed by interacting with /sys/kernel/mm/page_idle.
> > > > Looking up PFN from pagemap in Android devices is not supported by
> > > > unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
> > > >
> > > > This patch adds support to directly interact with page_idle tracking at
> > > > the PID level by introducing a /proc/<pid>/page_idle file. It follows
> > > > the exact same semantics as the global /sys/kernel/mm/page_idle, but now
> > > > looking up PFN through pagemap is not needed since the interface uses
> > > > virtual frame numbers, and at the same time also does not require
> > > > SYS_ADMIN.
> > > >
> > > > In Android, we are using this for the heap profiler (heapprofd) which
> > > > profiles and pin points code paths which allocates and leaves memory
> > > > idle for long periods of time. This method solves the security issue
> > > > with userspace learning the PFN, and while at it is also shown to yield
> > > > better results than the pagemap lookup, the theory being that the window
> > > > where the address space can change is reduced by eliminating the
> > > > intermediate pagemap look up stage. In virtual address indexing, the
> > > > process's mmap_sem is held for the duration of the access.
> > >
> > > What happens when you use this interface on shared pages, like memory
> > > inherited from the zygote, library file mappings and so on? If two
> > > profilers ran concurrently for two different processes that both map
> > > the same libraries, would they end up messing up each other's data?
> >
> > Yup PageIdle state is shared. That is the page_idle semantic even now
> > IIRC.
>
> Yes, that's right. This patch doesn't change that semantic. Idle page
> tracking at the core is a global procedure which is based on pages that can
> be shared.
>
> One of the usecases of the heap profiler is to enable profiling of pages that
> are shared between zygote and any processes that are forked. In this case,
> I am told by our team working on the heap profiler, that the monitoring of
> shared pages will help.
>
> > > Can this be used to observe which library pages other processes are
> > > accessing, even if you don't have access to those processes, as long
> > > as you can map the same libraries? I realize that there are already a
> > > bunch of ways to do that with side channels and such; but if you're
> > > adding an interface that allows this by design, it seems to me like
> > > something that should be gated behind some sort of privilege check.
> >
> > Hmm, you need to be priviledged to get the pfn now and without that you
> > cannot get to any page so the new interface is weakening the rules.
> > Maybe we should limit setting the idle state to processes with the write
> > status. Or do you think that even observing idle status is useful for
> > practical side channel attacks? If yes, is that a problem of the
> > profiler which does potentially dangerous things?
>
> The heap profiler is currently unprivileged. Would it help the concern Jann
> raised, if the new interface was limited to only anonymous private/shared
> pages and not to file pages? Or, is this even a real concern?
+Daniel Gruss in case he wants to provide some more detail; he has
been involved in a lot of the public research around this topic.
It is a bit of a concern when code that wasn't hardened as rigorously
as cryptographic library code operates on secret values.
A paper was published this year that abused mincore() in combination
with tricks for flushing the page cache to obtain information about
when shared read-only memory was accessed:
<https://arxiv.org/pdf/1901.01161.pdf>. In response to that, the
semantics of mincore() were changed to prevent that information from
leaking (see commit 134fca9063ad4851de767d1768180e5dede9a881).
On the other hand, an attacker could also use things like cache timing
attacks instead of abusing operating system features; that is more
hardware-specific, but it has a higher spatial granularity (typically
64 bytes instead of 4096 bytes). Timing-granularity-wise, I'm not sure
whether the proposed interface would be more or less bad than existing
cache side-channels on common architectures. There are papers that
demonstrate things like being able to distinguish some classes of
keyboard keys from others on an Android phone:
<https://www.usenix.org/system/files/conference/usenixsecurity16/sec16_paper_lipp.pdf>
I don't think limiting it to anonymous pages is necessarily enough to
completely solve this; in a normal Linux environment, it might be good
enough, but on Android, I'm worried about the CoW private memory from
the zygote.
^ permalink raw reply
* Re: [PATCH v5 2/6] mm/page_idle: Add support for handling swapped PG_Idle pages
From: Michal Hocko @ 2019-08-13 15:04 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Minchan Kim, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, joelaf,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Mike Rapoport, namhyung, paulmck,
Robin Murphy, Roman Gushchin, Stephen Rothwell, surenb,
Thomas Gleixner, tkjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <20190807171559.182301-2-joel@joelfernandes.org>
On Wed 07-08-19 13:15:55, Joel Fernandes (Google) wrote:
> Idle page tracking currently does not work well in the following
> scenario:
> 1. mark page-A idle which was present at that time.
> 2. run workload
> 3. page-A is not touched by workload
> 4. *sudden* memory pressure happen so finally page A is finally swapped out
> 5. now see the page A - it appears as if it was accessed (pte unmapped
> so idle bit not set in output) - but it's incorrect.
>
> To fix this, we store the idle information into a new idle bit of the
> swap PTE during swapping of anonymous pages.
>
> Also in the future, madvise extensions will allow a system process
> manager (like Android's ActivityManager) to swap pages out of a process
> that it knows will be cold. To an external process like a heap profiler
> that is doing idle tracking on another process, this procedure will
> interfere with the idle page tracking similar to the above steps.
This could be solved by checking the !present/swapped out pages
right? Whoever decided to put the page out to the swap just made it
idle effectively. So the monitor can make some educated guess for
tracking. If that is fundamentally not possible then please describe
why.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Michal Hocko @ 2019-08-13 14:57 UTC (permalink / raw)
To: Joel Fernandes
Cc: Andrew Morton, linux-kernel, Alexey Dobriyan, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190813144517.GE258732@google.com>
On Tue 13-08-19 10:45:17, Joel Fernandes wrote:
> On Tue, Aug 13, 2019 at 04:14:32PM +0200, Michal Hocko wrote:
> [snip]
> > > > If the API is flawed then this is likely going
> > > > to kick us later and will be hard to fix. I am still not convinced about
> > > > the swap part of the thing TBH.
> > >
> > > Ok, then let us discuss it. As I mentioned before, without this we lose the
> > > access information due to MADVISE or swapping. Minchan and Konstantin both
> > > suggested it that's why I also added it (other than me also realizing that it
> > > is neeed).
> >
> > I have described my concerns about the general idle bit behavior after
> > unmapping pointing to discrepancy with !anon pages. And I believe those
> > haven't been addressed yet.
>
> You are referring to this post right?
> https://lkml.org/lkml/2019/8/6/637
>
> Specifically your question was:
> How are you going to handle situation when the page is unmapped and refaulted again (e.g. a normal reclaim of a pagecache)?
>
> Currently I don't know how to implement that. Would it work if I stored the
> page-idle bit information in the pte of the file page (after the page is
> unmapped by reclaim?).
It would work as long as we keep page tables around after unmap. As they
are easily reconstructable this is a good candidate for reclaim as well.
> Also, this could be a future extension - the Android heap profiler does not
> need it right now. I know that's not a good argument but it is useful to say
> that it doesn't affect a real world usecase.. the swap issue on the other
> hand, is a real usecase. Since the profiler should not get affected by
> swapping or MADVISE_COLD hints.
>
> > Besides that I am still not seeing any
> > description of the usecase that would suffer from the lack of the
> > functionality in changelogs.
>
> You are talking about the swap usecase? The usecase is well layed out in v5
> 2/6. Did you see it? https://lore.kernel.org/patchwork/patch/1112283/
For some reason I've missed it. I will coment on that.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2] dma-mapping: Fix filename references
From: Christoph Hellwig @ 2019-08-13 14:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Tony Luck, Fenghua Yu, Konrad Rzeszutek Wilk,
linux-doc, Christoph Hellwig, Bjorn Helgaas
In-Reply-To: <20190813144222.GF30120@smile.fi.intel.com>
On Tue, Aug 13, 2019 at 05:42:22PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 19, 2019 at 05:19:55PM +0300, Andy Shevchenko wrote:
> > After the commit cf65a0f6f6ff
> >
> > ("dma-mapping: move all DMA mapping code to kernel/dma")
> >
> > some of the files are referring to outdated information, i.e. old file names
> > of DMA mapping sources.
> >
> > Fix it here.
> >
> > Note, the lines with "Glue code for..." have been removed completely.
>
> Any comment on this?
Fine with me, and I also agree with the glue code comment.
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Joel Fernandes @ 2019-08-13 14:45 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, linux-kernel, Alexey Dobriyan, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190813141432.GL17933@dhcp22.suse.cz>
On Tue, Aug 13, 2019 at 04:14:32PM +0200, Michal Hocko wrote:
[snip]
> > > If the API is flawed then this is likely going
> > > to kick us later and will be hard to fix. I am still not convinced about
> > > the swap part of the thing TBH.
> >
> > Ok, then let us discuss it. As I mentioned before, without this we lose the
> > access information due to MADVISE or swapping. Minchan and Konstantin both
> > suggested it that's why I also added it (other than me also realizing that it
> > is neeed).
>
> I have described my concerns about the general idle bit behavior after
> unmapping pointing to discrepancy with !anon pages. And I believe those
> haven't been addressed yet.
You are referring to this post right?
https://lkml.org/lkml/2019/8/6/637
Specifically your question was:
How are you going to handle situation when the page is unmapped and refaulted again (e.g. a normal reclaim of a pagecache)?
Currently I don't know how to implement that. Would it work if I stored the
page-idle bit information in the pte of the file page (after the page is
unmapped by reclaim?).
Also, this could be a future extension - the Android heap profiler does not
need it right now. I know that's not a good argument but it is useful to say
that it doesn't affect a real world usecase.. the swap issue on the other
hand, is a real usecase. Since the profiler should not get affected by
swapping or MADVISE_COLD hints.
> Besides that I am still not seeing any
> description of the usecase that would suffer from the lack of the
> functionality in changelogs.
You are talking about the swap usecase? The usecase is well layed out in v5
2/6. Did you see it? https://lore.kernel.org/patchwork/patch/1112283/
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v2] dma-mapping: Fix filename references
From: Andy Shevchenko @ 2019-08-13 14:42 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Tony Luck, Fenghua Yu, Konrad Rzeszutek Wilk,
linux-doc, Christoph Hellwig, Bjorn Helgaas
In-Reply-To: <20190619141956.65696-1-andriy.shevchenko@linux.intel.com>
On Wed, Jun 19, 2019 at 05:19:55PM +0300, Andy Shevchenko wrote:
> After the commit cf65a0f6f6ff
>
> ("dma-mapping: move all DMA mapping code to kernel/dma")
>
> some of the files are referring to outdated information, i.e. old file names
> of DMA mapping sources.
>
> Fix it here.
>
> Note, the lines with "Glue code for..." have been removed completely.
Any comment on this?
>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> - address Bjorn's comments
> Documentation/x86/x86_64/boot-options.rst | 2 +-
> arch/ia64/kernel/setup.c | 2 +-
> arch/x86/kernel/pci-swiotlb.c | 1 -
> arch/x86/kernel/setup.c | 2 +-
> arch/x86/pci/sta2x11-fixup.c | 4 +---
> 5 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/x86/x86_64/boot-options.rst b/Documentation/x86/x86_64/boot-options.rst
> index 6a4285a3c7a4..2b98efb5ba7f 100644
> --- a/Documentation/x86/x86_64/boot-options.rst
> +++ b/Documentation/x86/x86_64/boot-options.rst
> @@ -230,7 +230,7 @@ IOMMU (input/output memory management unit)
> ===========================================
> Multiple x86-64 PCI-DMA mapping implementations exist, for example:
>
> - 1. <lib/dma-direct.c>: use no hardware/software IOMMU at all
> + 1. <kernel/dma/direct.c>: use no hardware/software IOMMU at all
> (e.g. because you have < 3 GB memory).
> Kernel boot message: "PCI-DMA: Disabling IOMMU"
>
> diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
> index c9cfa760cd57..ab8d25d3e358 100644
> --- a/arch/ia64/kernel/setup.c
> +++ b/arch/ia64/kernel/setup.c
> @@ -256,7 +256,7 @@ __initcall(register_memory);
> * This function checks if the reserved crashkernel is allowed on the specific
> * IA64 machine flavour. Machines without an IO TLB use swiotlb and require
> * some memory below 4 GB (i.e. in 32 bit area), see the implementation of
> - * lib/swiotlb.c. The hpzx1 architecture has an IO TLB but cannot use that
> + * kernel/dma/swiotlb.c. The hpzx1 architecture has an IO TLB but cannot use that
> * in kdump case. See the comment in sba_init() in sba_iommu.c.
> *
> * So, the only machvec that really supports loading the kdump kernel
> diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
> index 5f5302028a9a..c2cfa5e7c152 100644
> --- a/arch/x86/kernel/pci-swiotlb.c
> +++ b/arch/x86/kernel/pci-swiotlb.c
> @@ -1,5 +1,4 @@
> // SPDX-License-Identifier: GPL-2.0
> -/* Glue code to lib/swiotlb.c */
>
> #include <linux/pci.h>
> #include <linux/cache.h>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 08a5f4a131f5..8655bf374893 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -477,7 +477,7 @@ static int __init reserve_crashkernel_low(void)
> ret = parse_crashkernel_low(boot_command_line, total_low_mem, &low_size, &base);
> if (ret) {
> /*
> - * two parts from lib/swiotlb.c:
> + * two parts from kernel/dma/swiotlb.c:
> * -swiotlb size: user-specified with swiotlb= or default.
> *
> * -swiotlb overflow buffer: now hardcoded to 32k. We round it
> diff --git a/arch/x86/pci/sta2x11-fixup.c b/arch/x86/pci/sta2x11-fixup.c
> index 97bbc12dd6b2..6269a175385d 100644
> --- a/arch/x86/pci/sta2x11-fixup.c
> +++ b/arch/x86/pci/sta2x11-fixup.c
> @@ -1,8 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> - * arch/x86/pci/sta2x11-fixup.c
> - * glue code for lib/swiotlb.c and DMA translation between STA2x11
> - * AMBA memory mapping and the X86 memory mapping
> + * DMA translation between STA2x11 AMBA memory mapping and the x86 memory mapping
> *
> * ST Microelectronics ConneXt (STA2X11/STA2X10)
> *
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Joel Fernandes @ 2019-08-13 14:25 UTC (permalink / raw)
To: Michal Hocko
Cc: Jann Horn, kernel list, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
Daniel Colascione, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, Linux API, linux-doc,
linux-fsdevel, Linux-MM, Mike Rapoport, Minchan Kim, namhyung,
Paul E. McKenney, Robin Murphy, Roman Gushchin, Stephen Rothwell,
Suren Baghdasaryan, Thomas Gleixner, Todd Kjos, Vladimir Davydov,
Vlastimil Babka, Will Deacon
In-Reply-To: <20190813100856.GF17933@dhcp22.suse.cz>
On Tue, Aug 13, 2019 at 12:08:56PM +0200, Michal Hocko wrote:
> On Mon 12-08-19 20:14:38, Jann Horn wrote:
> > On Wed, Aug 7, 2019 at 7:16 PM Joel Fernandes (Google)
> > <joel@joelfernandes.org> wrote:
> > > The page_idle tracking feature currently requires looking up the pagemap
> > > for a process followed by interacting with /sys/kernel/mm/page_idle.
> > > Looking up PFN from pagemap in Android devices is not supported by
> > > unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
> > >
> > > This patch adds support to directly interact with page_idle tracking at
> > > the PID level by introducing a /proc/<pid>/page_idle file. It follows
> > > the exact same semantics as the global /sys/kernel/mm/page_idle, but now
> > > looking up PFN through pagemap is not needed since the interface uses
> > > virtual frame numbers, and at the same time also does not require
> > > SYS_ADMIN.
> > >
> > > In Android, we are using this for the heap profiler (heapprofd) which
> > > profiles and pin points code paths which allocates and leaves memory
> > > idle for long periods of time. This method solves the security issue
> > > with userspace learning the PFN, and while at it is also shown to yield
> > > better results than the pagemap lookup, the theory being that the window
> > > where the address space can change is reduced by eliminating the
> > > intermediate pagemap look up stage. In virtual address indexing, the
> > > process's mmap_sem is held for the duration of the access.
> >
> > What happens when you use this interface on shared pages, like memory
> > inherited from the zygote, library file mappings and so on? If two
> > profilers ran concurrently for two different processes that both map
> > the same libraries, would they end up messing up each other's data?
>
> Yup PageIdle state is shared. That is the page_idle semantic even now
> IIRC.
Yes, that's right. This patch doesn't change that semantic. Idle page
tracking at the core is a global procedure which is based on pages that can
be shared.
One of the usecases of the heap profiler is to enable profiling of pages that
are shared between zygote and any processes that are forked. In this case,
I am told by our team working on the heap profiler, that the monitoring of
shared pages will help.
> > Can this be used to observe which library pages other processes are
> > accessing, even if you don't have access to those processes, as long
> > as you can map the same libraries? I realize that there are already a
> > bunch of ways to do that with side channels and such; but if you're
> > adding an interface that allows this by design, it seems to me like
> > something that should be gated behind some sort of privilege check.
>
> Hmm, you need to be priviledged to get the pfn now and without that you
> cannot get to any page so the new interface is weakening the rules.
> Maybe we should limit setting the idle state to processes with the write
> status. Or do you think that even observing idle status is useful for
> practical side channel attacks? If yes, is that a problem of the
> profiler which does potentially dangerous things?
The heap profiler is currently unprivileged. Would it help the concern Jann
raised, if the new interface was limited to only anonymous private/shared
pages and not to file pages? Or, is this even a real concern?
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v5 1/6] mm/page_idle: Add per-pid idle page tracking using virtual index
From: Michal Hocko @ 2019-08-13 14:14 UTC (permalink / raw)
To: Joel Fernandes
Cc: Andrew Morton, linux-kernel, Alexey Dobriyan, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190813135152.GC258732@google.com>
On Tue 13-08-19 09:51:52, Joel Fernandes wrote:
> On Tue, Aug 13, 2019 at 11:14:30AM +0200, Michal Hocko wrote:
> > On Mon 12-08-19 10:56:20, Joel Fernandes wrote:
> > > On Thu, Aug 08, 2019 at 10:00:44AM +0200, Michal Hocko wrote:
> > > > On Wed 07-08-19 17:31:05, Joel Fernandes wrote:
> > > > > On Wed, Aug 07, 2019 at 01:58:40PM -0700, Andrew Morton wrote:
> > > > > > On Wed, 7 Aug 2019 16:45:30 -0400 Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > > >
> > > > > > > On Wed, Aug 07, 2019 at 01:04:02PM -0700, Andrew Morton wrote:
> > > > > > > > On Wed, 7 Aug 2019 13:15:54 -0400 "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> > > > > > > >
> > > > > > > > > In Android, we are using this for the heap profiler (heapprofd) which
> > > > > > > > > profiles and pin points code paths which allocates and leaves memory
> > > > > > > > > idle for long periods of time. This method solves the security issue
> > > > > > > > > with userspace learning the PFN, and while at it is also shown to yield
> > > > > > > > > better results than the pagemap lookup, the theory being that the window
> > > > > > > > > where the address space can change is reduced by eliminating the
> > > > > > > > > intermediate pagemap look up stage. In virtual address indexing, the
> > > > > > > > > process's mmap_sem is held for the duration of the access.
> > > > > > > >
> > > > > > > > So is heapprofd a developer-only thing? Is heapprofd included in
> > > > > > > > end-user android loads? If not then, again, wouldn't it be better to
> > > > > > > > make the feature Kconfigurable so that Android developers can enable it
> > > > > > > > during development then disable it for production kernels?
> > > > > > >
> > > > > > > Almost all of this code is already configurable with
> > > > > > > CONFIG_IDLE_PAGE_TRACKING. If you disable it, then all of this code gets
> > > > > > > disabled.
> > > > > > >
> > > > > > > Or are you referring to something else that needs to be made configurable?
> > > > > >
> > > > > > Yes - the 300+ lines of code which this patchset adds!
> > > > > >
> > > > > > The impacted people will be those who use the existing
> > > > > > idle-page-tracking feature but who will not use the new feature. I
> > > > > > guess we can assume this set is small...
> > > > >
> > > > > Yes, I think this set should be small. The code size increase of page_idle.o
> > > > > is from ~1KB to ~2KB. Most of the extra space is consumed by
> > > > > page_idle_proc_generic() function which this patch adds. I don't think adding
> > > > > another CONFIG option to disable this while keeping existing
> > > > > CONFIG_IDLE_PAGE_TRACKING enabled, is worthwhile but I am open to the
> > > > > addition of such an option if anyone feels strongly about it. I believe that
> > > > > once this patch is merged, most like this new interface being added is what
> > > > > will be used more than the old interface (for some of the usecases) so it
> > > > > makes sense to keep it alive with CONFIG_IDLE_PAGE_TRACKING.
> > > >
> > > > I would tend to agree with Joel here. The functionality falls into an
> > > > existing IDLE_PAGE_TRACKING config option quite nicely. If there really
> > > > are users who want to save some space and this is standing in the way
> > > > then they can easily add a new config option with some justification so
> > > > the savings are clear. Without that an additional config simply adds to
> > > > the already existing configurability complexity and balkanization.
> > >
> > > Michal, Andrew, Minchan,
> > >
> > > Would you have any other review comments on the v5 series? This is just a new
> > > interface that does not disrupt existing users of the older page-idle
> > > tracking, so as such it is a safe change (as in, doesn't change existing
> > > functionality except for the draining bug fix).
> >
> > I hope to find some more time to finish the review but let me point out
> > that "it's new it is regression safe" is not really a great argument for
> > a new user visible API.
>
> Actually, I think you misunderstood me and took it out of context. I never
> intended to say "it is regression safe". I meant to say it is "low risk", as
> in that in all likelihood should not be hurting *existing users* of the *old
> interface*. Also as you know, it has been tested.
Yeah, misreading on my end.
> > If the API is flawed then this is likely going
> > to kick us later and will be hard to fix. I am still not convinced about
> > the swap part of the thing TBH.
>
> Ok, then let us discuss it. As I mentioned before, without this we lose the
> access information due to MADVISE or swapping. Minchan and Konstantin both
> suggested it that's why I also added it (other than me also realizing that it
> is neeed).
I have described my concerns about the general idle bit behavior after
unmapping pointing to discrepancy with !anon pages. And I believe those
haven't been addressed yet. Besides that I am still not seeing any
description of the usecase that would suffer from the lack of the
functionality in changelogs.
--
Michal Hocko
SUSE Labs
^ 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