* Re: [PATCH 0/5] kfree_rcu() additions for -rcu
From: Paul E. McKenney @ 2019-08-28 20:28 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, byungchul.park, Josh Triplett, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team
In-Reply-To: <5d657e30.1c69fb81.54250.01dc@mx.google.com>
On Tue, Aug 27, 2019 at 03:01:54PM -0400, Joel Fernandes (Google) wrote:
> Hi,
>
> This is a series on top of the patch "rcu/tree: Add basic support for kfree_rcu() batching".
>
> Link: http://lore.kernel.org/r/20190814160411.58591-1-joel@joelfernandes.org
>
> It adds performance tests, some clean ups and removal of "lazy" RCU callbacks.
>
> Now that kfree_rcu() is handled separately from call_rcu(), we also get rid of
> kfree "lazy" handling from tree RCU as suggested by Paul which will be unused.
> This also results in a nice negative delta as well.
>
> Joel Fernandes (Google) (5):
> rcu/rcuperf: Add kfree_rcu() performance Tests
> rcu/tree: Add multiple in-flight batches of kfree_rcu work
> rcu/tree: Add support for debug_objects debugging for kfree_rcu()
> rcu: Remove kfree_rcu() special casing and lazy handling
> rcu: Remove kfree_call_rcu_nobatch()
>
> Documentation/RCU/stallwarn.txt | 13 +-
> .../admin-guide/kernel-parameters.txt | 13 ++
> include/linux/rcu_segcblist.h | 2 -
> include/linux/rcutiny.h | 5 -
> include/linux/rcutree.h | 1 -
> include/trace/events/rcu.h | 32 ++--
> kernel/rcu/rcu.h | 27 ---
> kernel/rcu/rcu_segcblist.c | 25 +--
> kernel/rcu/rcu_segcblist.h | 25 +--
> kernel/rcu/rcuperf.c | 173 +++++++++++++++++-
> kernel/rcu/srcutree.c | 4 +-
> kernel/rcu/tiny.c | 29 ++-
> kernel/rcu/tree.c | 145 ++++++++++-----
> kernel/rcu/tree.h | 1 -
> kernel/rcu/tree_plugin.h | 42 +----
> kernel/rcu/tree_stall.h | 6 +-
> 16 files changed, 337 insertions(+), 206 deletions(-)
Looks like a 131-line positive delta to me. ;-)
Thanx, Paul
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Paul E. McKenney @ 2019-08-28 20:23 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <5d648897.1c69fb81.5e60a.fc70@mx.google.com>
On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> The dynticks_nmi_nesting counter serves 4 purposes:
>
> (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> interrupt nesting level.
>
> (b) We need to detect half-interrupts till we are sure they're not an
> issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
>
> (c) When a quiescent state report is needed from a nohz_full CPU.
> The nesting counter detects we are a first level interrupt.
>
> For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> outermost interrupt that interrupted an RCU-idle state can set it to 1.
>
> For (b), this warning condition has not occurred for several kernel
> releases. But we still keep the warning but change it to use
> in_interrupt() instead of the nesting counter. In a later year, we can
> remove the warning.
>
> For (c), the nest check is not really necessary since forced_tick would
> have been set to true in the outermost interrupt, so the nested/NMI
> interrupts will check forced_tick anyway, and bail.
Skipping the commit log and documentation for this pass.
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> .../Data-Structures/Data-Structures.rst | 31 +++------
> Documentation/RCU/stallwarn.txt | 6 +-
> kernel/rcu/tree.c | 64 +++++++------------
> kernel/rcu/tree.h | 4 +-
> kernel/rcu/tree_stall.h | 4 +-
> 5 files changed, 41 insertions(+), 68 deletions(-)
[ . . . ]
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 255cd6835526..1465a3e406f8 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -81,7 +81,6 @@
>
> static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> .dynticks_nesting = 1,
> - .dynticks_nmi_nesting = 0,
This should be in the previous patch, give or take naming.
> .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> };
> struct rcu_state rcu_state = {
> @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> /* Check for counter underflows */
> RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> "RCU dynticks_nesting counter underflow!");
> - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> - "RCU dynticks_nmi_nesting counter underflow/zero!");
>
> - /* Are we at first interrupt nesting level? */
> - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> - return false;
> -
> - /* Does CPU appear to be idle from an RCU standpoint? */
> - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> + /* Are we the outermost interrupt that arrived when RCU was idle? */
> + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> }
>
> #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
> /* Entering usermode/idle from interrupt is not handled. These would
> - * mean usermode upcalls or idle entry happened from interrupts. But,
> - * reset the counter if we warn.
> + * mean usermode upcalls or idle exit happened from interrupts. Remove
> + * the warning by 2020.
> */
> - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> + WARN_ON_ONCE(in_interrupt());
And this is a red flag. Bad things happen should some common code
that disables BH be invoked from the idle loop. This might not be
happening now, but we need to avoid this sort of constraint.
How about instead merging ->dyntick_nesting into the low-order bits
of ->dyntick_nmi_nesting?
Yes, this assumes that we don't enter process level twice, but it should
be easy to add a WARN_ON() to test for that. Except that we don't have
to because there is already this near the end of rcu_eqs_exit():
WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
So the low-order bit of the combined counter could indicate process-level
non-idle, the next three bits could be unused to make interpretation
of hex printouts easier, and then the rest of the bits could be used in
the same way as currently.
This would allow a single read to see the full state, so that 0x1 means
at process level in the kernel, 0x11 is interrupt (or NMI) from process
level, 0x10 is interrupt/NMI from idle/user, and so on.
What am I missing here? Why wouldn't this work, and without adding yet
another RCU-imposed constraint on some other subsystem?
Thanx, Paul
^ permalink raw reply
* Re: [RFC v1 1/2] rcu/tree: Clean up dynticks counter usage
From: Paul E. McKenney @ 2019-08-28 20:13 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <5d648895.1c69fb81.5e60a.fc6f@mx.google.com>
On Mon, Aug 26, 2019 at 09:33:53PM -0400, Joel Fernandes (Google) wrote:
> The dynticks counter are confusing due to crowbar writes of
> DYNTICK_IRQ_NONIDLE whose purpose is to detect half-interrupts (i.e. we
> see rcu_irq_enter() but not rcu_irq_exit() due to a usermode upcall) and
> if so then do a reset of the dyntick_nmi_nesting counters. This patch
> tries to get rid of DYNTICK_IRQ_NONIDLE while still keeping the code
> working, fully functional, and less confusing. The confusion recently
> has even led to patches forgetting that DYNTICK_IRQ_NONIDLE was written
> to which wasted lots of time.
>
> The patch has the following changes:
>
> (1) Use dynticks_nesting instead of dynticks_nmi_nesting for determining
> outer most "EQS exit". This is needed to detect in
> rcu_nmi_enter_common() if we have already EQS-exited, such as because of
> a syscall. Currently we rely on a forced write of DYNTICK_IRQ_NONIDLE
> from rcu_eqs_exit() for this purpose. This is one purpose of the
> DYNTICK_IRQ_NONIDLE write (other than detecting half-interrupts).
> However, we do not need to do that. dyntick_nesting already tells us that
> we have EQS-exited so just use that thus removing the dependence of
> dynticks_nmi_nesting for this purpose.
>
> (2) Keep dynticks_nmi_nesting around because:
>
> (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> interrupt nesting level.
>
> (b) We need to detect half-interrupts till we are sure they're not an
> issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
>
> (3) Since we got rid of DYNTICK_IRQ_NONIDLE, we also do cheaper
> comparisons with zero instead for the code that keeps the tick on in
> rcu_nmi_enter_common().
>
> In the next patch, both of the concerns of (2) will be addressed and
> then we can get rid of dynticks_nmi_nesting, however one step at a time.
Postponing discussion of the commit log for the moment.
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> kernel/rcu/rcu.h | 4 ----
> kernel/rcu/tree.c | 60 ++++++++++++++++++++++++++++-------------------
> 2 files changed, 36 insertions(+), 28 deletions(-)
>
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index aeec70fda82c..046833f3784b 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -12,10 +12,6 @@
>
> #include <trace/events/rcu.h>
>
> -/* Offset to allow distinguishing irq vs. task-based idle entry/exit. */
> -#define DYNTICK_IRQ_NONIDLE ((LONG_MAX / 2) + 1)
> -
> -
OK.
> /*
> * Grace-period counter management.
> */
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 68ebf0eb64c8..255cd6835526 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -81,7 +81,7 @@
>
> static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> .dynticks_nesting = 1,
> - .dynticks_nmi_nesting = DYNTICK_IRQ_NONIDLE,
> + .dynticks_nmi_nesting = 0,
C initializes to zero by default, so this can simply be deleted.
> .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> };
> struct rcu_state rcu_state = {
> @@ -558,17 +558,18 @@ EXPORT_SYMBOL_GPL(rcutorture_get_gp_data);
> /*
> * Enter an RCU extended quiescent state, which can be either the
> * idle loop or adaptive-tickless usermode execution.
> - *
> - * We crowbar the ->dynticks_nmi_nesting field to zero to allow for
> - * the possibility of usermode upcalls having messed up our count
> - * of interrupt nesting level during the prior busy period.
> */
> static void rcu_eqs_enter(bool user)
> {
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>
> - WARN_ON_ONCE(rdp->dynticks_nmi_nesting != DYNTICK_IRQ_NONIDLE);
> - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> + /* Entering usermode/idle from interrupt is not handled. These would
> + * mean usermode upcalls or idle entry happened from interrupts. But,
> + * reset the counter if we warn.
> + */
Please either put the "/*" on its own line or use "//"-style comments.
> + if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> + WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> +
@@@
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> rdp->dynticks_nesting == 0);
> if (rdp->dynticks_nesting != 1) {
> @@ -642,23 +643,27 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
> * (We are exiting an NMI handler, so RCU better be paying attention
> * to us!)
> */
> + WARN_ON_ONCE(rdp->dynticks_nesting <= 0);
This is fine.
> WARN_ON_ONCE(rdp->dynticks_nmi_nesting <= 0);
> WARN_ON_ONCE(rcu_dynticks_curr_cpu_in_eqs());
>
> + WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> + rdp->dynticks_nmi_nesting - 1);
This is problematic. The +/-1 and +/-2 dance is specifically for NMIs, so...
> /*
> * If the nesting level is not 1, the CPU wasn't RCU-idle, so
> * leave it in non-RCU-idle state.
> */
> - if (rdp->dynticks_nmi_nesting != 1) {
> - trace_rcu_dyntick(TPS("--="), rdp->dynticks_nmi_nesting, rdp->dynticks_nmi_nesting - 2, rdp->dynticks);
> - WRITE_ONCE(rdp->dynticks_nmi_nesting, /* No store tearing. */
> - rdp->dynticks_nmi_nesting - 2);
> + if (rdp->dynticks_nesting != 1) {
> + trace_rcu_dyntick(TPS("--="), rdp->dynticks_nesting,
> + rdp->dynticks_nesting - 2, rdp->dynticks);
> + WRITE_ONCE(rdp->dynticks_nesting, /* No store tearing. */
> + rdp->dynticks_nesting - 2);
Making the dancer's name be ->dynticks_nesting instead of
->dynticks_nmi_nesting is going to be trouble. (Yes, I did
take a quick look at the next patch, more on that when I get
there.)
> return;
> }
>
> /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> - trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, rdp->dynticks);
> - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> + trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nesting, 0, rdp->dynticks);
> + WRITE_ONCE(rdp->dynticks_nesting, 0); /* Avoid store tearing. */
Same here...
> if (irq)
> rcu_prepare_for_idle();
> @@ -723,10 +728,6 @@ void rcu_irq_exit_irqson(void)
> /*
> * Exit an RCU extended quiescent state, which can be either the
> * idle loop or adaptive-tickless usermode execution.
> - *
> - * We crowbar the ->dynticks_nmi_nesting field to DYNTICK_IRQ_NONIDLE to
> - * allow for the possibility of usermode upcalls messing up our count of
> - * interrupt nesting level during the busy period that is just now starting.
> */
> static void rcu_eqs_exit(bool user)
> {
> @@ -747,8 +748,13 @@ static void rcu_eqs_exit(bool user)
> trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, rdp->dynticks);
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
> WRITE_ONCE(rdp->dynticks_nesting, 1);
> - WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> - WRITE_ONCE(rdp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
> +
> + /* Exiting usermode/idle from interrupt is not handled. These would
> + * mean usermode upcalls or idle exit happened from interrupts. But,
> + * reset the counter if we warn.
> + */
> + if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> + WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
And here. Plus this is adding a test and branch in the common case.
Given that the location being written to should be hot in the cache,
it is not clear that this is a win.
> }
>
> /**
> @@ -804,6 +810,7 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
> long incby = 2;
>
> /* Complain about underflow. */
> + WARN_ON_ONCE(rdp->dynticks_nesting < 0);
OK.
> WARN_ON_ONCE(rdp->dynticks_nmi_nesting < 0);
>
> /*
> @@ -826,16 +833,21 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
>
> incby = 1;
> } else if (tick_nohz_full_cpu(rdp->cpu) &&
> - rdp->dynticks_nmi_nesting == DYNTICK_IRQ_NONIDLE &&
> - rdp->rcu_urgent_qs && !rdp->rcu_forced_tick) {
> + !rdp->dynticks_nmi_nesting && rdp->rcu_urgent_qs &&
> + !rdp->rcu_forced_tick) {
OK. Though you should be able to save a line by pulling the
"rdp->rcu_urgent_qs &&" onto the first line.
> rdp->rcu_forced_tick = true;
> tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU);
> }
> +
Not clear that the added blank line is a win, here or below.
> trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> - rdp->dynticks_nmi_nesting,
> - rdp->dynticks_nmi_nesting + incby, rdp->dynticks);
> + rdp->dynticks_nesting,
> + rdp->dynticks_nesting + incby, rdp->dynticks);
> +
> + WRITE_ONCE(rdp->dynticks_nesting, /* Prevent store tearing. */
> + rdp->dynticks_nesting + incby);
> +
> WRITE_ONCE(rdp->dynticks_nmi_nesting, /* Prevent store tearing. */
> - rdp->dynticks_nmi_nesting + incby);
> + rdp->dynticks_nmi_nesting + 1);
And same naming issue here.
Thanx, Paul
> barrier();
> }
>
> --
> 2.23.0.187.g17f5b7556c-goog
>
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Uwe Kleine-König @ 2019-08-28 19:59 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Andrew Morton, Jani Nikula,
Enrico Weigelt, metux IT consult, Jonathan Corbet, linux-doc,
linux-kernel, Alexandre Belloni, Mark Brown
In-Reply-To: <20190828113216.p2yiha4xyupkbcbs@pathway.suse.cz>
[-- Attachment #1.1: Type: text/plain, Size: 1292 bytes --]
On 8/28/19 1:32 PM, Petr Mladek wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König wrote:
>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
>
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.
For now I asked in the arm linux irc channel and got two people replying
(both added to Cc:):
Mark Brown (maintainer of SPI, regmap, ASoC and regulator) said:
1567019926 < broonie> ukleinek: I think that's a great idea and have
thought about trying to implement it in the past.
1567019937 < broonie> ukleinek: Making the logs more directly readable
is enormously helpful.
and Alexandre Belloni (arm/at91, mips/microsemi, rtc) said:
1567021451 < abelloni> ukleinek: seems good to me but it would probably
be better to be able to generate the list
(I fully agree to the wish to generate the list, as I already wrote
before, I don't have a good idea how to do that without generating
C-Code by some means which is ugly and also complicated by the fact that
there are several locations (at least for now) that have definitions for
error codes.)
Best regards
Uwe
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Uwe Kleine-König @ 2019-08-28 19:18 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Andrew Morton, Jani Nikula,
Enrico Weigelt, metux IT consult, Jonathan Corbet, linux-doc,
linux-kernel
In-Reply-To: <20190828113216.p2yiha4xyupkbcbs@pathway.suse.cz>
[-- Attachment #1.1: Type: text/plain, Size: 5889 bytes --]
Hello Petr,
On 8/28/19 1:32 PM, Petr Mladek wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König wrote:
>> Petr Mladek had some concerns:
>>> There are ideas to make the code even more tricky to reduce
>>> the size, keep it fast.
>>
>> I think Enrico Weigelt's suggestion to use a case is the best
>> performance-wise so that's what I picked up. Also I hope that
>> performance isn't that important because the need to print an error
>> should not be so common that it really hurts in production.
>
> I personally do not like switch/case. It is a lot of code.
> I wonder if it even saved some space.
I guess we have to die either way. Either it is quick or it is space
efficient. With the big case I trust the compiler to pick something
sensible expecting that it adapts for example to -Osize.
> If you want to safe space, I would use u16 to store the numbers.
> Or I would use array of strings. There will be only few holes.
>
> You might also consider handling only the most commonly
> used codes from errno.h and errno-base.h (1..133). There will
> be no holes and the codes are stable.
I'd like to postpone the discussion about "how" until we agreed about
the "if at all".
>>> Both, %dE modifier and the output format (ECODE) is non-standard.
>>
>> Yeah, obviously right. The problem is that the new modifier does
>> something that wasn't implemented before, so it cannot match any
>> standard. %pI is only known on Linux either, so I think being
>> non-standard is a weak argument.
>
> I am not completely sure that %p modifiers were a good idea.
> They came before I started maintaining printk(). They add more
> complex algorithms into paths where we could not report problems
> easily (printk recursion). Also they are causing problems with
> unit testing that might be done in userspace. These non-standard
> formats cause that printk() can't be simply substituted by printf().
In my eyes the wish to have printk() and userspace's printf
feature-identical isn't helpful because they have similar but not equal
purposes. And if a driver author cares about being able to use their
code 1:1 in userspace they could just not use %dE, %pI and whatever
there is additionally.
> I am not keen to spread these problems over more formats.
> Also %d format is more complicated. It is often used with
> already existing modifiers.
I don't understand what you want to tell me here.
>>> Upper letters gain a lot of attention. But the error code is
>>> only helper information. Also many error codes are misleading because
>>> they are used either wrongly or there was no better available.
>>
>> This isn't really an argument against the patch I think. Sure, if a
>> function returned (say) EIO while ETIMEOUT would be better, my patch
>> doesn't improve that detail. Still
>>
>> mydev: Failed to initialize blablub: EIO
>>
>> is more expressive than
>>
>> mydev: Failed to initialize blablub: -5
>
> OK, upper letters probably are not a problem.
>
> But what about EWOULDBLOCK and EDEADLOCK? They have the same
> error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
>
> People might spend a lot of time searching for EAGAIN before they
> notice that EWOULDBLOCK was used in the code instead.
It already does now. If you start to debug an error message that tells
you the error is -11, you might well come to the conclusion you have to
look for EDEADLK. IMHO the right approach here should be to declare one
of the duplicates as the "right" one and replace the wrong one in the
complete tree (apart from the definition for user space of course).
After that the problem with these duplicates is completely orthogonal
(it is already now mostly orthogonal in my eyes) and also solved for
those who picked the wrong one by hand.
> Also you still did not answer the question where the idea came from.
> Did it just look nice? Anyone asked for it? Who? Why?
It was my idea, and I didn't talk about it before creating a patch. You
asked in reply to v1: "Did it look like a good idea?
Did anyone got tired by searching for the error codes many
times a day? Did the idea came from a developer, support, or user, please?"
So yes, having to lookup the right error code from messages is something
that annoys me, especially because there are at least two files you have
to check for the definition. I consider myself to have all three hats
(developer, supporter and user), and your feedback set apart my
impression is that all people replying to this thread consider it a good
idea, too.
>>> There is no proof that this approach would be widely acceptable for
>>> subsystem maintainers. Some might not like mass and "blind" code
>>> changes. Some might not like the output at all.
>>
>> I don't intend to mass convert existing code. I would restrict myself to
>> updating the documentation and then maybe send a patch per subsystem as an
>> example to let maintainers know and judge for themselves if they like it or
>> not. And if it doesn't get picked up, we can just remove the feature again next
>> year (or so).
>
> It looks like a lot of potentially useless work.
If the idea will not gain speed, removing the then few users is straight
forward (just a question of calling sed s/%dE/%d/ on all affected
files). So at least it shouldn't be your useless work and time.
>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
>
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.
>
> I personally think that this feature is not worth the code, data,
> and bikeshedding.
I agree about the bike shedding ;-)
Best regards
Uwe
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2] doc: kselftest: update for clarity on running kselftests in CI rings
From: shuah @ 2019-08-28 19:17 UTC (permalink / raw)
To: Shuah Khan, corbet
Cc: linux-kselftest, linux-doc, linux-kernel, dan.rue, anders.roxell,
shuah
In-Reply-To: <20190827003709.26950-1-skhan@linuxfoundation.org>
On 8/26/19 6:37 PM, Shuah Khan wrote:
> Update to add clarity and recommendations on running newer kselftests
> on older kernels vs. matching the kernel and kselftest revisions.
>
> The recommendation is "Match kernel revision and kselftest."
>
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
> Changes since v1: Fixed "WARNING: Title underline too short."
I have a few more changes and would like to make and send a v3 after
the LPC's Testing and Fuzzing kselftest discussion.
Holding off on this patch for now.
thanks,
-- Shuah
^ permalink raw reply
* [PATCH] rcu/dyntick-idle: Add better tracing
From: Joel Fernandes (Google) @ 2019-08-28 18:26 UTC (permalink / raw)
To: linux-kernel, paulmck
Cc: Joel Fernandes (Google), Frederic Weisbecker, Jonathan Corbet,
Josh Triplett, kernel-team, Lai Jiangshan, linux-doc,
Mathieu Desnoyers, Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828001146.GM26530@linux.ibm.com>
The dyntick-idle traces are a bit confusing. This patch makes it simpler
and adds some missing cases such as EQS-enter because user vs idle mode.
Following are the changes:
(1) Add a new context field to trace_rcu_dyntick tracepoint. This
context field can be "USER", "IDLE" or "IRQ".
(2) Remove the "++=" and "--=" strings and replace them with
"StillNonIdle". This is much easier on the eyes, and the -- and ++
are easily apparent in the dynticks_nesting counters we are printing
anyway.
This patch is based on the previous patches to simplify rcu_dyntick
counters [1] and with these traces, I have verified the counters are
working properly.
[1]
Link: https://lore.kernel.org/patchwork/patch/1120021/
Link: https://lore.kernel.org/patchwork/patch/1120022/
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
include/trace/events/rcu.h | 13 ++++++++-----
kernel/rcu/tree.c | 17 +++++++++++------
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
index 66122602bd08..474c1f7e7104 100644
--- a/include/trace/events/rcu.h
+++ b/include/trace/events/rcu.h
@@ -449,12 +449,14 @@ TRACE_EVENT_RCU(rcu_fqs,
*/
TRACE_EVENT_RCU(rcu_dyntick,
- TP_PROTO(const char *polarity, long oldnesting, long newnesting, atomic_t dynticks),
+ TP_PROTO(const char *polarity, const char *context, long oldnesting,
+ long newnesting, atomic_t dynticks),
- TP_ARGS(polarity, oldnesting, newnesting, dynticks),
+ TP_ARGS(polarity, context, oldnesting, newnesting, dynticks),
TP_STRUCT__entry(
__field(const char *, polarity)
+ __field(const char *, context)
__field(long, oldnesting)
__field(long, newnesting)
__field(int, dynticks)
@@ -462,14 +464,15 @@ TRACE_EVENT_RCU(rcu_dyntick,
TP_fast_assign(
__entry->polarity = polarity;
+ __entry->context = context;
__entry->oldnesting = oldnesting;
__entry->newnesting = newnesting;
__entry->dynticks = atomic_read(&dynticks);
),
- TP_printk("%s %lx %lx %#3x", __entry->polarity,
- __entry->oldnesting, __entry->newnesting,
- __entry->dynticks & 0xfff)
+ TP_printk("%s %s %lx %lx %#3x", __entry->polarity,
+ __entry->context, __entry->oldnesting, __entry->newnesting,
+ __entry->dynticks & 0xfff)
);
/*
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1465a3e406f8..1a65919ec800 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -570,7 +570,8 @@ static void rcu_eqs_enter(bool user)
}
lockdep_assert_irqs_disabled();
- trace_rcu_dyntick(TPS("Start"), rdp->dynticks_nesting, 0, rdp->dynticks);
+ trace_rcu_dyntick(TPS("Start"), (user ? TPS("USER") : TPS("IDLE")),
+ rdp->dynticks_nesting, 0, rdp->dynticks);
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
rdp = this_cpu_ptr(&rcu_data);
do_nocb_deferred_wakeup(rdp);
@@ -642,15 +643,17 @@ static __always_inline void rcu_nmi_exit_common(bool irq)
* leave it in non-RCU-idle state.
*/
if (rdp->dynticks_nesting != 1) {
- trace_rcu_dyntick(TPS("--="), rdp->dynticks_nesting,
- rdp->dynticks_nesting - 2, rdp->dynticks);
+ trace_rcu_dyntick(TPS("StillNonIdle"), TPS("IRQ"),
+ rdp->dynticks_nesting, rdp->dynticks_nesting - 2,
+ rdp->dynticks);
WRITE_ONCE(rdp->dynticks_nesting, /* No store tearing. */
rdp->dynticks_nesting - 2);
return;
}
/* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
- trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nesting, 0, rdp->dynticks);
+ trace_rcu_dyntick(TPS("Start"), TPS("IRQ"), rdp->dynticks_nesting, 0,
+ rdp->dynticks);
WRITE_ONCE(rdp->dynticks_nesting, 0); /* Avoid store tearing. */
if (irq)
@@ -733,7 +736,8 @@ static void rcu_eqs_exit(bool user)
rcu_dynticks_task_exit();
rcu_dynticks_eqs_exit();
rcu_cleanup_after_idle();
- trace_rcu_dyntick(TPS("End"), rdp->dynticks_nesting, 1, rdp->dynticks);
+ trace_rcu_dyntick(TPS("End"), (user ? TPS("USER") : TPS("IDLE")),
+ rdp->dynticks_nesting, 1, rdp->dynticks);
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user && !is_idle_task(current));
WRITE_ONCE(rdp->dynticks_nesting, 1);
@@ -825,7 +829,8 @@ static __always_inline void rcu_nmi_enter_common(bool irq)
tick_dep_set_cpu(rdp->cpu, TICK_DEP_BIT_RCU);
}
- trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
+ trace_rcu_dyntick(incby == 1 ? TPS("End") : TPS("StillNonIdle"),
+ TPS("IRQ"),
rdp->dynticks_nesting,
rdp->dynticks_nesting + incby, rdp->dynticks);
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* Re: [PATCH 1/2] docs: kbuild: fix invalid ReST syntax
From: Masahiro Yamada @ 2019-08-28 16:39 UTC (permalink / raw)
To: Jonathan Corbet, open list:DOCUMENTATION
Cc: Michal Marek, Linux Kbuild mailing list,
Linux Kernel Mailing List
In-Reply-To: <20190814105400.1339-2-yamada.masahiro@socionext.com>
On Wed, Aug 14, 2019 at 7:54 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> I see the following warnings when I open this document with a ReST
> viewer, retext:
>
> /home/masahiro/ref/linux/Documentation/kbuild/makefiles.rst:1142: (WARNING/2) Inline emphasis start-string without end-string.
> /home/masahiro/ref/linux/Documentation/kbuild/makefiles.rst:1152: (WARNING/2) Inline emphasis start-string without end-string.
> /home/masahiro/ref/linux/Documentation/kbuild/makefiles.rst:1154: (WARNING/2) Inline emphasis start-string without end-string.
>
> These hunks were added by commit e846f0dc57f4 ("kbuild: add support
> for ensuring headers are self-contained") and commit 1e21cbfada87
> ("kbuild: support header-test-pattern-y"), respectively. They were
> written not for ReST but for the plain text, and merged via the
> kbuild tree.
>
> In the same development cycle, this document was converted to ReST
> by commit cd238effefa2 ("docs: kbuild: convert docs to ReST and rename
> to *.rst"), and merged via the doc sub-system.
>
> Merging them together into Linus' tree resulted in the current situation.
>
> To fix the syntax, surround the asterisks with back-quotes, and
> use :: for the code sample.
>
> Fixes: 39ceda5ce1b0 ("Merge tag 'kbuild-v5.3' of git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild")
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
Both applied to linux-kbuild.
> Documentation/kbuild/makefiles.rst | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/kbuild/makefiles.rst b/Documentation/kbuild/makefiles.rst
> index f4f0f7ffde2b..b4c28c543d72 100644
> --- a/Documentation/kbuild/makefiles.rst
> +++ b/Documentation/kbuild/makefiles.rst
> @@ -1139,7 +1139,7 @@ When kbuild executes, the following steps are followed (roughly):
>
> header-test-y
>
> - header-test-y specifies headers (*.h) in the current directory that
> + header-test-y specifies headers (`*.h`) in the current directory that
> should be compile tested to ensure they are self-contained,
> i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled,
> this builds them as part of extra-y.
> @@ -1147,11 +1147,11 @@ When kbuild executes, the following steps are followed (roughly):
> header-test-pattern-y
>
> This works as a weaker version of header-test-y, and accepts wildcard
> - patterns. The typical usage is:
> + patterns. The typical usage is::
>
> - header-test-pattern-y += *.h
> + header-test-pattern-y += *.h
>
> - This specifies all the files that matches to '*.h' in the current
> + This specifies all the files that matches to `*.h` in the current
> directory, but the files in 'header-test-' are excluded.
>
> 6.7 Commands useful for building a boot image
> --
> 2.17.1
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Uwe Kleine-König @ 2019-08-28 16:22 UTC (permalink / raw)
To: Sergey Senozhatsky, Rasmus Villemoes
Cc: Jani Nikula, Petr Mladek, Steven Rostedt, Andrew Morton,
Enrico Weigelt, metux IT consult, Jonathan Corbet, linux-doc,
linux-kernel
In-Reply-To: <20190828125951.GA12653@jagdpanzerIV>
[-- Attachment #1.1: Type: text/plain, Size: 1878 bytes --]
On 8/28/19 2:59 PM, Sergey Senozhatsky wrote:
> On (08/28/19 14:49), Rasmus Villemoes wrote:
>> On 28/08/2019 14.02, Sergey Senozhatsky wrote:
>>> On (08/28/19 14:54), Jani Nikula wrote:
>>> [..]
>>>>> I personally think that this feature is not worth the code, data,
>>>>> and bikeshedding.
>>>>
>>>> The obvious alternative, I think already mentioned, is to just add
>>>> strerror() or similar as a function. I doubt there'd be much opposition
>>>> to that. Folks could use %s and strerr(ret). And a follow-up could add
>>>> the special format specifier if needed.
>>>
>>> Yeah, I'd say that strerror() would be a better alternative
>>> to vsprintf() specifier. (if we decide to add such functionality).
>>
>> Please no. The .text footprint of the changes at the call sites to do
>> pr_err("...%s...", errcode(err)) instead of the current
>> pr_err("...%d...", err) would very soon dwarf whatever is necessary to
>> implement %pE or %dE.
Yeah, that's what I think, too. I cannot imagine a user of strerror()
who needs the string representation for something different than to feed
it to one of the family members of printk. That's also why I think that
the other already existing format specifier are a good idea.
It might not be the nicest part of the printk code, but this way it is
at least concentrated in one place only.
> New vsprintf() specifiers have some downsides as well. Should %dE
> accidentally (via backport) make it to the -stable kernel, which
> does not support %dE, and we are going to lose the actual error
> code value as well.
That is wrong. When you do
pr_err("There are no round tuits to give out: %dE\n", -ENOENT);
in a kernel that doesn't support %dE you get:
There are no round tuits to give out: -2E
That's a bit ugly but I can still work out what the original value was.
Best regards
Uwe
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v8 11/27] x86/mm: Introduce _PAGE_DIRTY_SW
From: Yu-cheng Yu @ 2019-08-28 14:57 UTC (permalink / raw)
To: Peter Zijlstra
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <20190828070308.GJ2332@hirez.programming.kicks-ass.net>
On Wed, 2019-08-28 at 09:03 +0200, Peter Zijlstra wrote:
> On Tue, Aug 27, 2019 at 03:37:12PM -0700, Yu-cheng Yu wrote:
> > On Fri, 2019-08-23 at 16:02 +0200, Peter Zijlstra wrote:
> > > On Tue, Aug 13, 2019 at 01:52:09PM -0700, Yu-cheng Yu wrote:
> > >
> > > > +static inline pte_t pte_move_flags(pte_t pte, pteval_t from, pteval_t
> > > > to)
> > > > +{
> > > > + if (pte_flags(pte) & from)
> > > > + pte = pte_set_flags(pte_clear_flags(pte, from), to);
> > > > + return pte;
> > > > +}
> > >
> > > Aside of the whole conditional thing (I agree it would be better to have
> > > this unconditionally); the function doesn't really do as advertised.
> > >
> > > That is, if @from is clear, it doesn't endeavour to make sure @to is
> > > also clear.
> > >
> > > Now it might be sufficient, but in that case it really needs a comment
> > > and or different name.
> > >
> > > An implementation that actually moves the bit is something like:
> > >
> > > pteval_t a,b;
> > >
> > > a = native_pte_value(pte);
> > > b = (a >> from_bit) & 1;
> > > a &= ~((1ULL << from_bit) | (1ULL << to_bit));
> > > a |= b << to_bit;
> > > return make_native_pte(a);
> >
> > There can be places calling pte_wrprotect() on a PTE that is already RO +
> > DIRTY_SW. Then in pte_move_flags(pte, _PAGE_DIRTY_HW, _PAGE_DIRTY_SW) we do
> > not
> > want to clear _PAGE_DIRTY_SW. But, I will look into this and make it more
> > obvious.
>
> Well, then the name 'move' is just wrong, because that is not the
> semantics you're looking for.
>
> So the thing is; if you provide a generic function that 'munges' two
> bits, then it's name had better be accurate. But AFAICT you only ever
> used this for the DIRTY bits, so it might be better to have a function
> specifically for that and with a comment that spells out the exact
> semantics and reasons for them.
Yes, I will work on that.
Yu-cheng
^ permalink raw reply
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Thomas Gleixner @ 2019-08-28 14:15 UTC (permalink / raw)
To: Pavel Machek
Cc: Borislav Petkov, Greg Kroah-Hartman, linux-kernel, stable,
Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828133713.GF8052@amd>
On Wed, 28 Aug 2019, Pavel Machek wrote:
> On Wed 2019-08-28 14:46:21, Borislav Petkov wrote:
> > On Wed, Aug 28, 2019 at 02:29:13PM +0200, Pavel Machek wrote:
> > > This is not a way to have an inteligent conversation.
> >
> > No, this *is* the way to keep the conversation sane, without veering
> > off into some absurd claims.
> >
> > So, to cut to the chase: you can simply add "rdrand=force" to your
> > cmdline parameters and get back to using RDRAND.
> >
> > And yet if you still feel this fix does not meet your expectations,
> > you were told already to either produce patches or who to contact. I'm
> > afraid complaining on this thread won't get you anywhere but that's your
> > call.
>
> No, this does not meet my expectations, it violates stable kernel
> rules, and will cause regression to some users, while better solution
> is known to be available.
Your unqualified ranting does not meet my expectation either and it
violates any rule of common sense.
For the record:
Neither AMD nor we have any idea which particular machines have a fixed
BIOS and which have not. There is no technical indicator either at boot
time as the wreckage manifests itself only after resume.
So in the interest of users the only sensible decision is to disable
RDRAND for this class of CPUs.
If you have a list of machines which have a fixed BIOS, then provide it
in form of patches. If not then stop claiming that there is a better
solution available.
Anyway, I'm done with that and further rants of yours go directly to
/dev/null.
Thanks for wasting everyones time
tglx
^ permalink raw reply
* [PATCH v2] rcu/tree: Add multiple in-flight batches of kfree_rcu work
From: Joel Fernandes (Google) @ 2019-08-28 14:09 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Paul E . McKenney, byungchul.park,
Josh Triplett, Lai Jiangshan, linux-doc, Mathieu Desnoyers, rcu,
Steven Rostedt, kernel-team
In-Reply-To: <5d657e35.1c69fb81.54250.01de@mx.google.com>
During testing, it was observed that amount of memory consumed due
kfree_rcu() batching is 300-400MB. Previously we had only a single
head_free pointer pointing to the list of rcu_head(s) that are to be
freed after a grace period. Until this list is drained, we cannot queue
any more objects on it since such objects may not be ready to be
reclaimed when the worker thread eventually gets to drainin g the
head_free list.
We can do better by maintaining multiple lists as done by this patch.
Testing shows that memory consumption came down by around 100-150MB with
just adding another list. Adding more than 1 additional list did not
show any improvement.
Suggested-by: Paul E. McKenney <paulmck@linux.ibm.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/rcu/tree.c | 61 ++++++++++++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 19 deletions(-)
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 4f7c3096d786..5bf8f7e793ea 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2688,28 +2688,37 @@ EXPORT_SYMBOL_GPL(call_rcu);
/* Maximum number of jiffies to wait before draining a batch. */
#define KFREE_DRAIN_JIFFIES (HZ / 50)
+#define KFREE_N_BATCHES 2
+
+struct kfree_rcu_work {
+ /* 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 that have now left ->head and are queued for
+ * freeing after a grace period.
+ */
+ struct rcu_head *head_free;
+
+ struct kfree_rcu_cpu *krcp;
+};
/*
* 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.
*/
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;
+ /* Pointer to the per-cpu array of kfree_rcu_work structures */
+ struct kfree_rcu_work krw_arr[KFREE_N_BATCHES];
- /* Protect concurrent access to this structure. */
+ /* Protect concurrent access to this structure and kfree_rcu_work. */
spinlock_t lock;
/* The delayed work that flushes ->head to ->head_free incase ->head
@@ -2730,12 +2739,14 @@ 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);
+ struct kfree_rcu_work *krwp = container_of(to_rcu_work(work),
+ struct kfree_rcu_work, rcu_work);
+ struct kfree_rcu_cpu *krcp;
+
+ krcp = krwp->krcp;
spin_lock_irqsave(&krcp->lock, flags);
- head = krcp->head_free;
- krcp->head_free = NULL;
+ head = xchg(&krwp->head_free, NULL);
spin_unlock_irqrestore(&krcp->lock, flags);
/*
@@ -2758,19 +2769,28 @@ static void kfree_rcu_work(struct work_struct *work)
*/
static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
{
+ int i = 0;
+ struct kfree_rcu_work *krwp = NULL;
+
lockdep_assert_held(&krcp->lock);
+ while (i < KFREE_N_BATCHES) {
+ if (!krcp->krw_arr[i].head_free) {
+ krwp = &(krcp->krw_arr[i]);
+ break;
+ }
+ i++;
+ }
- /* If a previous RCU batch work is already in progress, we cannot queue
+ /* If both RCU batches are 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)
+ if (!krwp)
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);
+ krwp->head_free = xchg(&krcp->head, NULL);
+ INIT_RCU_WORK(&krwp->rcu_work, kfree_rcu_work);
+ queue_rcu_work(system_wq, &krwp->rcu_work);
return true;
}
@@ -3736,8 +3756,11 @@ static void __init kfree_rcu_batch_init(void)
for_each_possible_cpu(cpu) {
struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
+ int i = KFREE_N_BATCHES;
spin_lock_init(&krcp->lock);
+ while (i--)
+ krcp->krw_arr[i].krcp = krcp;
INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
}
}
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* Re: [PATCH 2/5] rcu/tree: Add multiple in-flight batches of kfree_rcu work
From: Joel Fernandes @ 2019-08-28 14:02 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-kernel, Paul E . McKenney, byungchul.park, Josh Triplett,
Lai Jiangshan, linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt,
kernel-team
In-Reply-To: <20190827235253.GB30253@tardis>
On Wed, Aug 28, 2019 at 07:52:53AM +0800, Boqun Feng wrote:
> Hi Joel,
>
> On Tue, Aug 27, 2019 at 03:01:56PM -0400, Joel Fernandes (Google) wrote:
> > During testing, it was observed that amount of memory consumed due
> > kfree_rcu() batching is 300-400MB. Previously we had only a single
> > head_free pointer pointing to the list of rcu_head(s) that are to be
> > freed after a grace period. Until this list is drained, we cannot queue
> > any more objects on it since such objects may not be ready to be
> > reclaimed when the worker thread eventually gets to drainin g the
> > head_free list.
> >
> > We can do better by maintaining multiple lists as done by this patch.
> > Testing shows that memory consumption came down by around 100-150MB with
> > just adding another list. Adding more than 1 additional list did not
> > show any improvement.
> >
> > Suggested-by: Paul E. McKenney <paulmck@linux.ibm.com>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> > kernel/rcu/tree.c | 64 +++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 45 insertions(+), 19 deletions(-)
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 4f7c3096d786..9b9ae4db1c2d 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -2688,28 +2688,38 @@ EXPORT_SYMBOL_GPL(call_rcu);
> >
> > /* Maximum number of jiffies to wait before draining a batch. */
> > #define KFREE_DRAIN_JIFFIES (HZ / 50)
> > +#define KFREE_N_BATCHES 2
> > +
> > +struct kfree_rcu_work {
> > + /* 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 that have now left ->head and are queued for
> > + * freeing after a grace period.
> > + */
> > + struct rcu_head *head_free;
> > +
> > + struct kfree_rcu_cpu *krcp;
> > +};
> > +static DEFINE_PER_CPU(__typeof__(struct kfree_rcu_work)[KFREE_N_BATCHES], krw);
> >
>
> Why not
>
> static DEFINE_PER_CPU(struct kfree_rcu_work[KFREE_N_BATCHES], krw);
>
> here? Am I missing something?
Yes, that's better.
> Further, given "struct kfree_rcu_cpu" is only for defining percpu
> variables, how about orginazing the data structure like:
>
> struct kfree_rcu_cpu {
> ...
> struct kfree_rcu_work krws[KFREE_N_BATCHES];
> ...
> }
>
> This could save one pointer in kfree_rcu_cpu, and I think it provides
> better cache locality for accessing _cpu and _work on the same cpu.
>
> Thoughts?
Yes, that's better. Thanks, Boqun! Following is the diff which I will fold
into this patch:
---8<-----------------------
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index b3259306b7a5..fac5ae96d8b1 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2717,7 +2717,6 @@ struct kfree_rcu_work {
struct kfree_rcu_cpu *krcp;
};
-static DEFINE_PER_CPU(__typeof__(struct kfree_rcu_work)[KFREE_N_BATCHES], krw);
/*
* Maximum number of kfree(s) to batch, if this limit is hit then the batch of
@@ -2731,7 +2730,7 @@ struct kfree_rcu_cpu {
struct rcu_head *head;
/* Pointer to the per-cpu array of kfree_rcu_work structures */
- struct kfree_rcu_work *krwp;
+ struct kfree_rcu_work krw_arr[KFREE_N_BATCHES];
/* Protect concurrent access to this structure and kfree_rcu_work. */
spinlock_t lock;
@@ -2800,8 +2799,8 @@ static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
lockdep_assert_held(&krcp->lock);
while (i < KFREE_N_BATCHES) {
- if (!krcp->krwp[i].head_free) {
- krwp = &(krcp->krwp[i]);
+ if (!krcp->krw_arr[i].head_free) {
+ krwp = &(krcp->krw_arr[i]);
break;
}
i++;
@@ -3780,13 +3779,11 @@ static void __init kfree_rcu_batch_init(void)
for_each_possible_cpu(cpu) {
struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
- struct kfree_rcu_work *krwp = &(per_cpu(krw, cpu)[0]);
int i = KFREE_N_BATCHES;
spin_lock_init(&krcp->lock);
- krcp->krwp = krwp;
while (i--)
- krwp[i].krcp = krcp;
+ krcp->krw_arr[i].krcp = krcp;
INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
}
}
^ permalink raw reply related
* Re: [PATCH v12 00/11] Appended signatures support for IMA appraisal
From: Mimi Zohar @ 2019-08-28 13:43 UTC (permalink / raw)
To: Jordan Hand, Thiago Jung Bauermann, linux-integrity
Cc: linux-security-module, keyrings, linux-crypto, linuxppc-dev,
linux-doc, linux-kernel, Dmitry Kasatkin, James Morris,
Serge E. Hallyn, David Howells, David Woodhouse, Jessica Yu,
Herbert Xu, David S. Miller, Jonathan Corbet, AKASHI, Takahiro
In-Reply-To: <9682b5d0-1634-2dd0-2cbb-eb1fa8ba7423@linux.microsoft.com>
Hi Jordan,
On Mon, 2019-08-26 at 15:46 -0700, Jordan Hand wrote:
> On 6/27/19 7:19 PM, Thiago Jung Bauermann wrote:
> > On the OpenPOWER platform, secure boot and trusted boot are being
> > implemented using IMA for taking measurements and verifying signatures.
> > Since the kernel image on Power servers is an ELF binary, kernels are
> > signed using the scripts/sign-file tool and thus use the same signature
> > format as signed kernel modules.
> >
> > This patch series adds support in IMA for verifying those signatures.
> > It adds flexibility to OpenPOWER secure boot, because it allows it to boot
> > kernels with the signature appended to them as well as kernels where the
> > signature is stored in the IMA extended attribute.
>
> I know this is pretty late, but I just wanted to let you know that I
> tested this patch set on x86_64 with QEMU.
>
> That is, I enrolled a key to _ima keyring, signed my kernel and modules
> with appended signatures (with scripts/sign-file), set the IMA policy to
> appraise and measure my kernel and modules. Also tested kexec appraisal.
>
> You can add my tested-by if you'd like.
I really appreciate your testing. Based on the recent
Documentation/maintainer/rebasing-and-merging.rst, I'm trying not to
rebase patches already staged in linux-next. Patches are first being
staged in the next-queued-testing branch.
FYI, I just posted a patch that adds IMA appended signature support to
test_kexec_file_load.sh.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v3 01/10] KVM: arm64: Document PV-time interface
From: Christoffer Dall @ 2019-08-28 13:49 UTC (permalink / raw)
To: Steven Price
Cc: kvm, linux-doc, Marc Zyngier, linux-kernel, Russell King,
Catalin Marinas, Paolo Bonzini, Will Deacon, kvmarm,
linux-arm-kernel
In-Reply-To: <20190827085706.GB6541@e113682-lin.lund.arm.com>
On Tue, Aug 27, 2019 at 10:57:06AM +0200, Christoffer Dall wrote:
> On Wed, Aug 21, 2019 at 04:36:47PM +0100, Steven Price wrote:
> > Introduce a paravirtualization interface for KVM/arm64 based on the
> > "Arm Paravirtualized Time for Arm-Base Systems" specification DEN 0057A.
> >
> > This only adds the details about "Stolen Time" as the details of "Live
> > Physical Time" have not been fully agreed.
> >
> > User space can specify a reserved area of memory for the guest and
> > inform KVM to populate the memory with information on time that the host
> > kernel has stolen from the guest.
> >
> > A hypercall interface is provided for the guest to interrogate the
> > hypervisor's support for this interface and the location of the shared
> > memory structures.
> >
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > ---
> > Documentation/virt/kvm/arm/pvtime.txt | 100 ++++++++++++++++++++++++++
> > 1 file changed, 100 insertions(+)
> > create mode 100644 Documentation/virt/kvm/arm/pvtime.txt
> >
> > diff --git a/Documentation/virt/kvm/arm/pvtime.txt b/Documentation/virt/kvm/arm/pvtime.txt
> > new file mode 100644
> > index 000000000000..1ceb118694e7
> > --- /dev/null
> > +++ b/Documentation/virt/kvm/arm/pvtime.txt
> > @@ -0,0 +1,100 @@
> > +Paravirtualized time support for arm64
> > +======================================
> > +
> > +Arm specification DEN0057/A defined a standard for paravirtualised time
> > +support for AArch64 guests:
> > +
> > +https://developer.arm.com/docs/den0057/a
> > +
> > +KVM/arm64 implements the stolen time part of this specification by providing
> > +some hypervisor service calls to support a paravirtualized guest obtaining a
> > +view of the amount of time stolen from its execution.
> > +
> > +Two new SMCCC compatible hypercalls are defined:
> > +
> > +PV_FEATURES 0xC5000020
> > +PV_TIME_ST 0xC5000022
> > +
> > +These are only available in the SMC64/HVC64 calling convention as
> > +paravirtualized time is not available to 32 bit Arm guests. The existence of
> > +the PV_FEATURES hypercall should be probed using the SMCCC 1.1 ARCH_FEATURES
> > +mechanism before calling it.
> > +
> > +PV_FEATURES
> > + Function ID: (uint32) : 0xC5000020
> > + PV_func_id: (uint32) : Either PV_TIME_LPT or PV_TIME_ST
> > + Return value: (int32) : NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant
> > + PV-time feature is supported by the hypervisor.
> > +
> > +PV_TIME_ST
> > + Function ID: (uint32) : 0xC5000022
> > + Return value: (int64) : IPA of the stolen time data structure for this
> > + (V)CPU. On failure:
> > + NOT_SUPPORTED (-1)
> > +
> > +The IPA returned by PV_TIME_ST should be mapped by the guest as normal memory
> > +with inner and outer write back caching attributes, in the inner shareable
> > +domain. A total of 16 bytes from the IPA returned are guaranteed to be
> > +meaningfully filled by the hypervisor (see structure below).
> > +
> > +PV_TIME_ST returns the structure for the calling VCPU.
> > +
> > +Stolen Time
> > +-----------
> > +
> > +The structure pointed to by the PV_TIME_ST hypercall is as follows:
> > +
> > + Field | Byte Length | Byte Offset | Description
> > + ----------- | ----------- | ----------- | --------------------------
> > + Revision | 4 | 0 | Must be 0 for version 0.1
> > + Attributes | 4 | 4 | Must be 0
> > + Stolen time | 8 | 8 | Stolen time in unsigned
> > + | | | nanoseconds indicating how
> > + | | | much time this VCPU thread
> > + | | | was involuntarily not
> > + | | | running on a physical CPU.
> > +
> > +The structure will be updated by the hypervisor prior to scheduling a VCPU. It
> > +will be present within a reserved region of the normal memory given to the
> > +guest. The guest should not attempt to write into this memory. There is a
> > +structure per VCPU of the guest.
> > +
> > +User space interface
> > +====================
> > +
> > +User space can request that KVM provide the paravirtualized time interface to
> > +a guest by creating a KVM_DEV_TYPE_ARM_PV_TIME device, for example:
> > +
> > + struct kvm_create_device pvtime_device = {
> > + .type = KVM_DEV_TYPE_ARM_PV_TIME,
> > + .attr = 0,
> > + .flags = 0,
> > + };
> > +
> > + pvtime_fd = ioctl(vm_fd, KVM_CREATE_DEVICE, &pvtime_device);
> > +
> > +Creation of the device should be done after creating the vCPUs of the virtual
> > +machine.
> > +
> > +The IPA of the structures must be given to KVM. This is the base address
> > +of an array of stolen time structures (one for each VCPU). The base address
> > +must be page aligned. The size must be at least 64 * number of VCPUs and be a
> > +multiple of PAGE_SIZE.
> > +
> > +The memory for these structures should be added to the guest in the usual
> > +manner (e.g. using KVM_SET_USER_MEMORY_REGION).
> > +
> > +For example:
> > +
> > + struct kvm_dev_arm_st_region region = {
> > + .gpa = <IPA of guest base address>,
> > + .size = <size in bytes>
> > + };
>
> This feel fragile; how are you handling userspace creating VCPUs after
> setting this up, the GPA overlapping guest memory, etc. Is the
> philosophy here that the VMM can mess up the VM if it wants, but that
> this should never lead attacks on the host (we better hope not) and so
> we don't care?
>
> It seems to me setting the IPA per vcpu throught the VCPU device would
> avoid a lot of these issues. See
> Documentation/virt/kvm/devices/vcpu.txt.
>
>
I discussed this with Marc the other day, and we realized that if we
make the configuration of the IPA per-PE, then a VMM can construct a VM
where these data structures are distributed within the IPA space of a
VM, which could lead to a lower TLB pressure for some
configurations/workloads.
Thanks,
Christoffer
^ permalink raw reply
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-28 13:37 UTC (permalink / raw)
To: Borislav Petkov
Cc: Pavel Machek, Thomas Gleixner, Greg Kroah-Hartman, linux-kernel,
stable, Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828124621.GI4920@zn.tnic>
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
On Wed 2019-08-28 14:46:21, Borislav Petkov wrote:
> On Wed, Aug 28, 2019 at 02:29:13PM +0200, Pavel Machek wrote:
> > This is not a way to have an inteligent conversation.
>
> No, this *is* the way to keep the conversation sane, without veering
> off into some absurd claims.
>
> So, to cut to the chase: you can simply add "rdrand=force" to your
> cmdline parameters and get back to using RDRAND.
>
> And yet if you still feel this fix does not meet your expectations,
> you were told already to either produce patches or who to contact. I'm
> afraid complaining on this thread won't get you anywhere but that's your
> call.
No, this does not meet my expectations, it violates stable kernel
rules, and will cause regression to some users, while better solution
is known to be available.
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Sergey Senozhatsky @ 2019-08-28 12:59 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Sergey Senozhatsky, Jani Nikula, Petr Mladek,
Uwe Kleine-König, Steven Rostedt, Enrico, Weigelt,
Andrew Morton, metux IT consult, Jonathan Corbet, linux-doc,
linux-kernel
In-Reply-To: <087e8e18-8044-27ef-b0bd-8a1093f53b32@rasmusvillemoes.dk>
On (08/28/19 14:49), Rasmus Villemoes wrote:
> On 28/08/2019 14.02, Sergey Senozhatsky wrote:
> > On (08/28/19 14:54), Jani Nikula wrote:
> > [..]
> >>> I personally think that this feature is not worth the code, data,
> >>> and bikeshedding.
> >>
> >> The obvious alternative, I think already mentioned, is to just add
> >> strerror() or similar as a function. I doubt there'd be much opposition
> >> to that. Folks could use %s and strerr(ret). And a follow-up could add
> >> the special format specifier if needed.
> >
> > Yeah, I'd say that strerror() would be a better alternative
> > to vsprintf() specifier. (if we decide to add such functionality).
>
> Please no. The .text footprint of the changes at the call sites to do
> pr_err("...%s...", errcode(err)) instead of the current
> pr_err("...%d...", err) would very soon dwarf whatever is necessary to
> implement %pE or %dE.
New vsprintf() specifiers have some downsides as well. Should %dE
accidentally (via backport) make it to the -stable kernel, which
does not support %dE, and we are going to lose the actual error
code value as well.
-ss
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Rasmus Villemoes @ 2019-08-28 12:49 UTC (permalink / raw)
To: Sergey Senozhatsky, Jani Nikula
Cc: Petr Mladek, Uwe Kleine-König, Steven Rostedt, Enrico,
Weigelt, Andrew Morton, metux IT consult, Jonathan Corbet,
linux-doc, linux-kernel
In-Reply-To: <20190828120246.GA31416@jagdpanzerIV>
On 28/08/2019 14.02, Sergey Senozhatsky wrote:
> On (08/28/19 14:54), Jani Nikula wrote:
> [..]
>>> I personally think that this feature is not worth the code, data,
>>> and bikeshedding.
>>
>> The obvious alternative, I think already mentioned, is to just add
>> strerror() or similar as a function. I doubt there'd be much opposition
>> to that. Folks could use %s and strerr(ret). And a follow-up could add
>> the special format specifier if needed.
>
> Yeah, I'd say that strerror() would be a better alternative
> to vsprintf() specifier. (if we decide to add such functionality).
Please no. The .text footprint of the changes at the call sites to do
pr_err("...%s...", errcode(err)) instead of the current
pr_err("...%d...", err) would very soon dwarf whatever is necessary to
implement %pE or %dE. Also that would prevent any kind of graceful
fallback in case err doesn't have a known value - errcode() would have
to return some fixed "huh, unknown error number" string, so we'd lose
the actual number.
Rasmus
^ permalink raw reply
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Borislav Petkov @ 2019-08-28 12:46 UTC (permalink / raw)
To: Pavel Machek
Cc: Thomas Gleixner, Greg Kroah-Hartman, linux-kernel, stable,
Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828122913.GE8052@amd>
On Wed, Aug 28, 2019 at 02:29:13PM +0200, Pavel Machek wrote:
> This is not a way to have an inteligent conversation.
No, this *is* the way to keep the conversation sane, without veering
off into some absurd claims.
So, to cut to the chase: you can simply add "rdrand=force" to your
cmdline parameters and get back to using RDRAND.
And yet if you still feel this fix does not meet your expectations,
you were told already to either produce patches or who to contact. I'm
afraid complaining on this thread won't get you anywhere but that's your
call.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-28 12:29 UTC (permalink / raw)
To: Borislav Petkov
Cc: Pavel Machek, Thomas Gleixner, Greg Kroah-Hartman, linux-kernel,
stable, Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828121628.GG4920@zn.tnic>
[-- Attachment #1: Type: text/plain, Size: 652 bytes --]
On Wed 2019-08-28 14:16:28, Borislav Petkov wrote:
> On Wed, Aug 28, 2019 at 02:09:36PM +0200, Pavel Machek wrote:
> > Yes, and now AMD has patch to break it on *all* machines.
>
> It doesn't break all machines - you need to look at that patch again.
This is not a way to have an inteligent conversation.
The patch clearly breaks more machines than it has to. It is known to
cause regression. You snipped the rest of the email with better ways
to solve this.
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Borislav Petkov @ 2019-08-28 12:16 UTC (permalink / raw)
To: Pavel Machek
Cc: Thomas Gleixner, Greg Kroah-Hartman, linux-kernel, stable,
Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828120935.GD8052@amd>
On Wed, Aug 28, 2019 at 02:09:36PM +0200, Pavel Machek wrote:
> Yes, and now AMD has patch to break it on *all* machines.
It doesn't break all machines - you need to look at that patch again.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-28 12:09 UTC (permalink / raw)
To: Borislav Petkov
Cc: Pavel Machek, Thomas Gleixner, Greg Kroah-Hartman, linux-kernel,
stable, Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828120024.GF4920@zn.tnic>
[-- Attachment #1: Type: text/plain, Size: 836 bytes --]
On Wed 2019-08-28 14:00:24, Borislav Petkov wrote:
> On Wed, Aug 28, 2019 at 01:49:47PM +0200, Pavel Machek wrote:
> > AMD screwed this up,
>
> Except that it wasn't AMD who screwed up but BIOS on *some* laptops.
Yes, and now AMD has patch to break it on *all* machines.
Hmm. "Get random data" instruction that fails to return random data,
depending on BIOS... (and even indicates success, IIRC)... Sounds like
CPU vendor screwup to me.
And they can also fix it up. If re-initing random generator is
impossible from kernel, surely CPU microcode can be either updated to
do the init automatically, or at least allow kernel to do the init.
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 01/10] KVM: arm64: Document PV-time interface
From: Steven Price @ 2019-08-28 12:09 UTC (permalink / raw)
To: Christoffer Dall
Cc: Marc Zyngier, Will Deacon, linux-arm-kernel, kvmarm, linux-kernel,
kvm, Catalin Marinas, linux-doc, Russell King, Paolo Bonzini
In-Reply-To: <20190827085706.GB6541@e113682-lin.lund.arm.com>
On 27/08/2019 09:57, Christoffer Dall wrote:
> On Wed, Aug 21, 2019 at 04:36:47PM +0100, Steven Price wrote:
>> Introduce a paravirtualization interface for KVM/arm64 based on the
>> "Arm Paravirtualized Time for Arm-Base Systems" specification DEN 0057A.
>>
>> This only adds the details about "Stolen Time" as the details of "Live
>> Physical Time" have not been fully agreed.
>>
>> User space can specify a reserved area of memory for the guest and
>> inform KVM to populate the memory with information on time that the host
>> kernel has stolen from the guest.
>>
>> A hypercall interface is provided for the guest to interrogate the
>> hypervisor's support for this interface and the location of the shared
>> memory structures.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Documentation/virt/kvm/arm/pvtime.txt | 100 ++++++++++++++++++++++++++
>> 1 file changed, 100 insertions(+)
>> create mode 100644 Documentation/virt/kvm/arm/pvtime.txt
>>
>> diff --git a/Documentation/virt/kvm/arm/pvtime.txt b/Documentation/virt/kvm/arm/pvtime.txt
>> new file mode 100644
>> index 000000000000..1ceb118694e7
>> --- /dev/null
>> +++ b/Documentation/virt/kvm/arm/pvtime.txt
>> @@ -0,0 +1,100 @@
>> +Paravirtualized time support for arm64
>> +======================================
>> +
>> +Arm specification DEN0057/A defined a standard for paravirtualised time
>> +support for AArch64 guests:
>> +
>> +https://developer.arm.com/docs/den0057/a
>> +
>> +KVM/arm64 implements the stolen time part of this specification by providing
>> +some hypervisor service calls to support a paravirtualized guest obtaining a
>> +view of the amount of time stolen from its execution.
>> +
>> +Two new SMCCC compatible hypercalls are defined:
>> +
>> +PV_FEATURES 0xC5000020
>> +PV_TIME_ST 0xC5000022
>> +
>> +These are only available in the SMC64/HVC64 calling convention as
>> +paravirtualized time is not available to 32 bit Arm guests. The existence of
>> +the PV_FEATURES hypercall should be probed using the SMCCC 1.1 ARCH_FEATURES
>> +mechanism before calling it.
>> +
>> +PV_FEATURES
>> + Function ID: (uint32) : 0xC5000020
>> + PV_func_id: (uint32) : Either PV_TIME_LPT or PV_TIME_ST
>> + Return value: (int32) : NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant
>> + PV-time feature is supported by the hypervisor.
>> +
>> +PV_TIME_ST
>> + Function ID: (uint32) : 0xC5000022
>> + Return value: (int64) : IPA of the stolen time data structure for this
>> + (V)CPU. On failure:
>> + NOT_SUPPORTED (-1)
>> +
>> +The IPA returned by PV_TIME_ST should be mapped by the guest as normal memory
>> +with inner and outer write back caching attributes, in the inner shareable
>> +domain. A total of 16 bytes from the IPA returned are guaranteed to be
>> +meaningfully filled by the hypervisor (see structure below).
>> +
>> +PV_TIME_ST returns the structure for the calling VCPU.
>> +
>> +Stolen Time
>> +-----------
>> +
>> +The structure pointed to by the PV_TIME_ST hypercall is as follows:
>> +
>> + Field | Byte Length | Byte Offset | Description
>> + ----------- | ----------- | ----------- | --------------------------
>> + Revision | 4 | 0 | Must be 0 for version 0.1
>> + Attributes | 4 | 4 | Must be 0
>> + Stolen time | 8 | 8 | Stolen time in unsigned
>> + | | | nanoseconds indicating how
>> + | | | much time this VCPU thread
>> + | | | was involuntarily not
>> + | | | running on a physical CPU.
>> +
>> +The structure will be updated by the hypervisor prior to scheduling a VCPU. It
>> +will be present within a reserved region of the normal memory given to the
>> +guest. The guest should not attempt to write into this memory. There is a
>> +structure per VCPU of the guest.
>> +
>> +User space interface
>> +====================
>> +
>> +User space can request that KVM provide the paravirtualized time interface to
>> +a guest by creating a KVM_DEV_TYPE_ARM_PV_TIME device, for example:
>> +
>> + struct kvm_create_device pvtime_device = {
>> + .type = KVM_DEV_TYPE_ARM_PV_TIME,
>> + .attr = 0,
>> + .flags = 0,
>> + };
>> +
>> + pvtime_fd = ioctl(vm_fd, KVM_CREATE_DEVICE, &pvtime_device);
>> +
>> +Creation of the device should be done after creating the vCPUs of the virtual
>> +machine.
>> +
>> +The IPA of the structures must be given to KVM. This is the base address
>> +of an array of stolen time structures (one for each VCPU). The base address
>> +must be page aligned. The size must be at least 64 * number of VCPUs and be a
>> +multiple of PAGE_SIZE.
>> +
>> +The memory for these structures should be added to the guest in the usual
>> +manner (e.g. using KVM_SET_USER_MEMORY_REGION).
>> +
>> +For example:
>> +
>> + struct kvm_dev_arm_st_region region = {
>> + .gpa = <IPA of guest base address>,
>> + .size = <size in bytes>
>> + };
>
> This feel fragile; how are you handling userspace creating VCPUs after
> setting this up,
In this case as long as the structures all fit within the region created
VCPUs can be created/destroyed at will. If the VCPU index is too high
then the kernel will bail out in kvm_update_stolen_time() so the
structure will not be written. I consider this case as user space
messing up, so beyond protecting the host from the mess, user space gets
to keep the pieces.
> the GPA overlapping guest memory, etc.
Again, the (host) kernel is protected against this, but clearly this
will end badly for the guest.
> Is the
> philosophy here that the VMM can mess up the VM if it wants, but that
> this should never lead attacks on the host (we better hope not) and so
> we don't care?
Yes. For things like GPA overlapping guest memory it's not really the
host's position to work out what is "guest memory". It's quite possible
that user space could decide to place the stolen time structures right
in the middle of guest memory - it's just up to user space to inform the
guest what memory is usable. Obviously the expectation is that the
shared structures would be positioned "out of the way" in GPA space in
any normal arrangement.
> It seems to me setting the IPA per vcpu throught the VCPU device would
> avoid a lot of these issues. See
> Documentation/virt/kvm/devices/vcpu.txt.
That is certainly a possibility, I'm not really sure what the benefit is
though? It would still lead to corner cases:
* What if only some VCPUs had stolen time setup on them?
* What if multiple VCPUs pointed to the same location?
* The structures can still overlap with guest memory
It's also more work to setup in user space with the only "benefit" being
that user space could choose to organise the structures however it sees
fit (e.g. no need for them to be contiguous in memory). But I'm not sure
I see a use case for that flexibility.
Perhaps there's some benefit I'm not seeing?
Steve
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Sergey Senozhatsky @ 2019-08-28 12:02 UTC (permalink / raw)
To: Jani Nikula
Cc: Petr Mladek, Uwe Kleine-König, Sergey Senozhatsky,
Steven Rostedt, Enrico, Weigelt, Andrew Morton, metux IT consult,
Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <87o9097bff.fsf@intel.com>
On (08/28/19 14:54), Jani Nikula wrote:
[..]
> > I personally think that this feature is not worth the code, data,
> > and bikeshedding.
>
> The obvious alternative, I think already mentioned, is to just add
> strerror() or similar as a function. I doubt there'd be much opposition
> to that. Folks could use %s and strerr(ret). And a follow-up could add
> the special format specifier if needed.
Yeah, I'd say that strerror() would be a better alternative
to vsprintf() specifier. (if we decide to add such functionality).
-ss
^ permalink raw reply
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Borislav Petkov @ 2019-08-28 12:00 UTC (permalink / raw)
To: Pavel Machek
Cc: Thomas Gleixner, Greg Kroah-Hartman, linux-kernel, stable,
Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828114947.GC8052@amd>
On Wed, Aug 28, 2019 at 01:49:47PM +0200, Pavel Machek wrote:
> AMD screwed this up,
Except that it wasn't AMD who screwed up but BIOS on *some* laptops.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ 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