* Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
From: Michal Hocko @ 2019-07-12 7:02 UTC (permalink / raw)
To: Hoan Tran OS
Cc: Catalin Marinas, Heiko Carstens, open list:MEMORY MANAGEMENT,
Paul Mackerras, H . Peter Anvin, sparclinux@vger.kernel.org,
Alexander Duyck, linux-s390@vger.kernel.org, x86@kernel.org,
Mike Rapoport, Christian Borntraeger, Ingo Molnar,
Vlastimil Babka, Open Source Submission, Pavel Tatashin,
Vasily Gorbik, Will Deacon, Borislav Petkov, Thomas Gleixner,
linux-arm-kernel@lists.infradead.org, Oscar Salvador,
linux-kernel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org, David S . Miller
In-Reply-To: <1562887528-5896-1-git-send-email-Hoan@os.amperecomputing.com>
On Thu 11-07-19 23:25:44, Hoan Tran OS wrote:
> In NUMA layout which nodes have memory ranges that span across other nodes,
> the mm driver can detect the memory node id incorrectly.
>
> For example, with layout below
> Node 0 address: 0000 xxxx 0000 xxxx
> Node 1 address: xxxx 1111 xxxx 1111
>
> Note:
> - Memory from low to high
> - 0/1: Node id
> - x: Invalid memory of a node
>
> When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
> config, mm only checks the memory validity but not the node id.
> Because of that, Node 1 also detects the memory from node 0 as below
> when it scans from the start address to the end address of node 1.
>
> Node 0 address: 0000 xxxx xxxx xxxx
> Node 1 address: xxxx 1111 1111 1111
>
> This layout could occur on any architecture. This patch enables
> CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA to fix this issue.
Yes it can occur on any arch but most sane platforms simply do not
overlap physical ranges. So I do not really see any reason to
unconditionally enable the config for everybody. What is an advantage?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [PATCH v4 0/3] Forced-wakeup for stop states on Powernv
From: Abhishek Goel @ 2019-07-12 7:04 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm
Cc: ego, daniel.lezcano, rjw, npiggin, Abhishek Goel, dja
Currently, the cpuidle governors determine what idle state a idling CPU
should enter into based on heuristics that depend on the idle history on
that CPU. Given that no predictive heuristic is perfect, there are cases
where the governor predicts a shallow idle state, hoping that the CPU will
be busy soon. However, if no new workload is scheduled on that CPU in the
near future, the CPU will end up in the shallow state.
Motivation
----------
In case of POWER, this is problematic, when the predicted state in the
aforementioned scenario is a shallow stop state on a tickless system. As
we might get stuck into shallow states even for hours, in absence of ticks
or interrupts.
To address this, We forcefully wakeup the cpu by setting the decrementer.
The decrementer is set to a value that corresponds with the residency of
the next available state. Thus firing up a timer that will forcefully
wakeup the cpu. Few such iterations will essentially train the governor to
select a deeper state for that cpu, as the timer here corresponds to the
next available cpuidle state residency. Thus, cpu will eventually end up
in the deepest possible state and we won't get stuck in a shallow state
for long duration.
Experiment
----------
For earlier versions when this feature was meat to be only for shallow lite
states, I performed experiments for three scenarios to collect some data.
case 1 :
Without this patch and without tick retained, i.e. in a upstream kernel,
It would spend more than even a second to get out of stop0_lite.
case 2 : With tick retained in a upstream kernel -
Generally, we have a sched tick at 4ms(CONF_HZ = 250). Ideally I expected
it to take 8 sched tick to get out of stop0_lite. Experimentally,
observation was
=========================================================
sample min max 99percentile
20 4ms 12ms 4ms
=========================================================
It would take atleast one sched tick to get out of stop0_lite.
case 2 : With this patch (not stopping tick, but explicitly queuing a
timer)
============================================================
sample min max 99percentile
============================================================
20 144us 192us 144us
============================================================
Description of current implementation
-------------------------------------
We calculate timeout for the current idle state as the residency value
of the next available idle state. If the decrementer is set to be
greater than this timeout, we update the decrementer value with the
residency of next available idle state. Thus, essentially training the
governor to select the next available deeper state until we reach the
deepest state. Hence, we won't get stuck unnecessarily in shallow states
for longer duration.
--------------------------------
v1 of auto-promotion : https://lkml.org/lkml/2019/3/22/58 This patch was
implemented only for shallow lite state in generic cpuidle driver.
v2 : Removed timeout_needed and rebased to current
upstream kernel
Then,
v1 of forced-wakeup : Moved the code to cpuidle powernv driver and started
as forced wakeup instead of auto-promotion
v2 : Extended the forced wakeup logic for all states.
Setting the decrementer instead of queuing up a hrtimer to implement the
logic.
v3 : 1) Cleanly handle setting the decrementer after exiting out of stop
states.
2) Added a disable_callback feature to compute timeout whenever a
state is enbaled or disabled instead of computing everytime in fast
idle path.
3) Use disable callback to recompute timeout whenever state usage
is changed for a state. Also, cleaned up the get_snooze_timeout
function.
v4 : Changed the type and name of set/reset decrementer function.
Handled irq work pending in try_set_dec_before_idle.
No change in patch 2 and 3.
Abhishek Goel (3):
cpuidle-powernv : forced wakeup for stop states
cpuidle : Add callback whenever a state usage is enabled/disabled
cpuidle-powernv : Recompute the idle-state timeouts when state usage
is enabled/disabled
arch/powerpc/include/asm/time.h | 2 ++
arch/powerpc/kernel/time.c | 43 ++++++++++++++++++++++++
drivers/cpuidle/cpuidle-powernv.c | 55 +++++++++++++++++++++++--------
drivers/cpuidle/sysfs.c | 15 ++++++++-
include/linux/cpuidle.h | 5 +++
5 files changed, 106 insertions(+), 14 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH v4 1/3] cpuidle-powernv : forced wakeup for stop states
From: Abhishek Goel @ 2019-07-12 7:04 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm
Cc: ego, daniel.lezcano, rjw, npiggin, Abhishek Goel, dja
In-Reply-To: <20190712070457.55242-1-huntbag@linux.vnet.ibm.com>
Currently, the cpuidle governors determine what idle state a idling CPU
should enter into based on heuristics that depend on the idle history on
that CPU. Given that no predictive heuristic is perfect, there are cases
where the governor predicts a shallow idle state, hoping that the CPU will
be busy soon. However, if no new workload is scheduled on that CPU in the
near future, the CPU may end up in the shallow state.
This is problematic, when the predicted state in the aforementioned
scenario is a shallow stop state on a tickless system. As we might get
stuck into shallow states for hours, in absence of ticks or interrupts.
To address this, We forcefully wakeup the cpu by setting the
decrementer. The decrementer is set to a value that corresponds with the
residency of the next available state. Thus firing up a timer that will
forcefully wakeup the cpu. Few such iterations will essentially train the
governor to select a deeper state for that cpu, as the timer here
corresponds to the next available cpuidle state residency. Thus, cpu will
eventually end up in the deepest possible state.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
Auto-promotion
v1 : started as auto promotion logic for cpuidle states in generic
driver
v2 : Removed timeout_needed and rebased the code to upstream kernel
Forced-wakeup
v1 : New patch with name of forced wakeup started
v2 : Extending the forced wakeup logic for all states. Setting the
decrementer instead of queuing up a hrtimer to implement the logic.
v3 : Cleanly handle setting/resetting of decrementer so as to not break
irq work
v4 : Changed type and name of set/reset decrementer fucntion
Handled irq_work_pending in try_set_dec_before_idle
arch/powerpc/include/asm/time.h | 2 ++
arch/powerpc/kernel/time.c | 43 +++++++++++++++++++++++++++++++
drivers/cpuidle/cpuidle-powernv.c | 40 ++++++++++++++++++++++++++++
3 files changed, 85 insertions(+)
diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index 54f4ec1f9fab..294a472ce161 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -188,6 +188,8 @@ static inline unsigned long tb_ticks_since(unsigned long tstamp)
extern u64 mulhdu(u64, u64);
#endif
+extern bool try_set_dec_before_idle(u64 timeout);
+extern void try_reset_dec_after_idle(void);
extern void div128_by_32(u64 dividend_high, u64 dividend_low,
unsigned divisor, struct div_result *dr);
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 694522308cd5..d004c0d8e099 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -576,6 +576,49 @@ void arch_irq_work_raise(void)
#endif /* CONFIG_IRQ_WORK */
+/*
+ * This function tries setting decrementer before entering into idle.
+ * Returns true if we have reprogrammed the decrementer for idle.
+ * Returns false if the decrementer is unchanged.
+ */
+bool try_set_dec_before_idle(u64 timeout)
+{
+ u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
+ u64 now = get_tb_or_rtc();
+
+ if (now + timeout > *next_tb)
+ return false;
+
+ set_dec(timeout);
+ if (test_irq_work_pending())
+ set_dec(1);
+
+ return true;
+}
+
+/*
+ * This function gets called if we have set decrementer before
+ * entering into idle. It tries to reset/restore the decrementer
+ * to its original value.
+ */
+void try_reset_dec_after_idle(void)
+{
+ u64 now;
+ u64 *next_tb;
+
+ if (test_irq_work_pending())
+ return;
+
+ now = get_tb_or_rtc();
+ next_tb = this_cpu_ptr(&decrementers_next_tb);
+ if (now >= *next_tb)
+ return;
+
+ set_dec(*next_tb - now);
+ if (test_irq_work_pending())
+ set_dec(1);
+}
+
/*
* timer_interrupt - gets called when the decrementer overflows,
* with interrupts disabled.
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 84b1ebe212b3..17e20e408ffe 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -21,6 +21,7 @@
#include <asm/opal.h>
#include <asm/runlatch.h>
#include <asm/cpuidle.h>
+#include <asm/time.h>
/*
* Expose only those Hardware idle states via the cpuidle framework
@@ -46,6 +47,26 @@ static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly
static u64 default_snooze_timeout __read_mostly;
static bool snooze_timeout_en __read_mostly;
+static u64 forced_wakeup_timeout(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ int i;
+
+ for (i = index + 1; i < drv->state_count; i++) {
+ struct cpuidle_state *s = &drv->states[i];
+ struct cpuidle_state_usage *su = &dev->states_usage[i];
+
+ if (s->disabled || su->disable)
+ continue;
+
+ return (s->target_residency + 2 * s->exit_latency) *
+ tb_ticks_per_usec;
+ }
+
+ return 0;
+}
+
static u64 get_snooze_timeout(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
@@ -144,8 +165,27 @@ static int stop_loop(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
+ u64 timeout_tb;
+ bool forced_wakeup = false;
+
+ timeout_tb = forced_wakeup_timeout(dev, drv, index);
+
+ /* Ensure that the timeout is at least one microsecond
+ * greater than current decrement value. Else, we will
+ * unnecesarily wakeup again within a microsecond if we
+ * set decrementer with such timeout value.
+ */
+ timeout_tb += 512;
+
+ if (timeout_tb)
+ forced_wakeup = try_set_dec_before_idle(timeout_tb);
+
power9_idle_type(stop_psscr_table[index].val,
stop_psscr_table[index].mask);
+
+ if (forced_wakeup)
+ try_reset_dec_after_idle();
+
return index;
}
--
2.17.1
^ permalink raw reply related
* [RFC v4 2/3] cpuidle : Add callback whenever a state usage is enabled/disabled
From: Abhishek Goel @ 2019-07-12 7:04 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm
Cc: ego, daniel.lezcano, rjw, npiggin, Abhishek Goel, dja
In-Reply-To: <20190712070457.55242-1-huntbag@linux.vnet.ibm.com>
To force wakeup a cpu, we need to compute the timeout in the fast idle
path as a state may be enabled or disabled but there did not exist a
feedback to driver when a state is enabled or disabled.
This patch adds a callback whenever a state_usage records a store for
disable attribute.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
drivers/cpuidle/sysfs.c | 15 ++++++++++++++-
include/linux/cpuidle.h | 4 ++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index eb20adb5de23..141671a53967 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -415,8 +415,21 @@ static ssize_t cpuidle_state_store(struct kobject *kobj, struct attribute *attr,
struct cpuidle_state_usage *state_usage = kobj_to_state_usage(kobj);
struct cpuidle_state_attr *cattr = attr_to_stateattr(attr);
- if (cattr->store)
+ if (cattr->store) {
ret = cattr->store(state, state_usage, buf, size);
+ if (ret == size &&
+ strncmp(cattr->attr.name, "disable",
+ strlen("disable"))) {
+ struct kobject *cpuidle_kobj = kobj->parent;
+ struct cpuidle_device *dev =
+ to_cpuidle_device(cpuidle_kobj);
+ struct cpuidle_driver *drv =
+ cpuidle_get_cpu_driver(dev);
+
+ if (drv->disable_callback)
+ drv->disable_callback(dev, drv);
+ }
+ }
return ret;
}
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index bb9a0db89f1a..8a0e54bd0d5d 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -119,6 +119,10 @@ struct cpuidle_driver {
/* the driver handles the cpus in cpumask */
struct cpumask *cpumask;
+
+ void (*disable_callback)(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv);
+
};
#ifdef CONFIG_CPU_IDLE
--
2.17.1
^ permalink raw reply related
* [RFC v4 3/3] cpuidle-powernv : Recompute the idle-state timeouts when state usage is enabled/disabled
From: Abhishek Goel @ 2019-07-12 7:04 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm
Cc: ego, daniel.lezcano, rjw, npiggin, Abhishek Goel, dja
In-Reply-To: <20190712070457.55242-1-huntbag@linux.vnet.ibm.com>
The disable callback can be used to compute timeout for other states
whenever a state is enabled or disabled. We store the computed timeout
in "timeout" defined in cpuidle state strucure. So, we compute timeout
only when some state is enabled or disabled and not every time in the
fast idle path.
We also use the computed timeout to get timeout for snooze, thus getting
rid of get_snooze_timeout for snooze loop.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
drivers/cpuidle/cpuidle-powernv.c | 35 +++++++++++--------------------
include/linux/cpuidle.h | 1 +
2 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 17e20e408ffe..29add322d0c4 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -45,7 +45,6 @@ struct stop_psscr_table {
static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly;
static u64 default_snooze_timeout __read_mostly;
-static bool snooze_timeout_en __read_mostly;
static u64 forced_wakeup_timeout(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
@@ -67,26 +66,13 @@ static u64 forced_wakeup_timeout(struct cpuidle_device *dev,
return 0;
}
-static u64 get_snooze_timeout(struct cpuidle_device *dev,
- struct cpuidle_driver *drv,
- int index)
+static void pnv_disable_callback(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv)
{
int i;
- if (unlikely(!snooze_timeout_en))
- return default_snooze_timeout;
-
- for (i = index + 1; i < drv->state_count; i++) {
- struct cpuidle_state *s = &drv->states[i];
- struct cpuidle_state_usage *su = &dev->states_usage[i];
-
- if (s->disabled || su->disable)
- continue;
-
- return s->target_residency * tb_ticks_per_usec;
- }
-
- return default_snooze_timeout;
+ for (i = 0; i < drv->state_count; i++)
+ drv->states[i].timeout = forced_wakeup_timeout(dev, drv, i);
}
static int snooze_loop(struct cpuidle_device *dev,
@@ -94,16 +80,20 @@ static int snooze_loop(struct cpuidle_device *dev,
int index)
{
u64 snooze_exit_time;
+ u64 snooze_timeout = drv->states[index].timeout;
+
+ if (!snooze_timeout)
+ snooze_timeout = default_snooze_timeout;
set_thread_flag(TIF_POLLING_NRFLAG);
local_irq_enable();
- snooze_exit_time = get_tb() + get_snooze_timeout(dev, drv, index);
+ snooze_exit_time = get_tb() + snooze_timeout;
ppc64_runlatch_off();
HMT_very_low();
while (!need_resched()) {
- if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time) {
+ if (get_tb() > snooze_exit_time) {
/*
* Task has not woken up but we are exiting the polling
* loop anyway. Require a barrier after polling is
@@ -168,7 +158,7 @@ static int stop_loop(struct cpuidle_device *dev,
u64 timeout_tb;
bool forced_wakeup = false;
- timeout_tb = forced_wakeup_timeout(dev, drv, index);
+ timeout_tb = drv->states[index].timeout;
/* Ensure that the timeout is at least one microsecond
* greater than current decrement value. Else, we will
@@ -263,6 +253,7 @@ static int powernv_cpuidle_driver_init(void)
*/
drv->cpumask = (struct cpumask *)cpu_present_mask;
+ drv->disable_callback = pnv_disable_callback;
return 0;
}
@@ -422,8 +413,6 @@ static int powernv_idle_probe(void)
/* Device tree can indicate more idle states */
max_idle_state = powernv_add_idle_states();
default_snooze_timeout = TICK_USEC * tb_ticks_per_usec;
- if (max_idle_state > 1)
- snooze_timeout_en = true;
} else
return -ENODEV;
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 8a0e54bd0d5d..31662b657b9c 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -50,6 +50,7 @@ struct cpuidle_state {
int power_usage; /* in mW */
unsigned int target_residency; /* in US */
bool disabled; /* disabled on all CPUs */
+ unsigned long long timeout; /* timeout for exiting out of a state */
int (*enter) (struct cpuidle_device *dev,
struct cpuidle_driver *drv,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V2] mm/ioremap: Probe platform for p4d huge map support
From: Michael Ellerman @ 2019-07-12 7:07 UTC (permalink / raw)
To: Anshuman Khandual, Andrew Morton
Cc: Stephen Rothwell, x86, Peter Zijlstra, Catalin Marinas,
Dave Hansen, Will Deacon, Michal Hocko, linux-mm, Ingo Molnar,
linux-arm-kernel, Andy Lutomirski, Thomas Gleixner, linuxppc-dev,
Kirill A . Shutemov
In-Reply-To: <fbc147c7-bec2-daed-b828-c4ae170010a9@arm.com>
Anshuman Khandual <anshuman.khandual@arm.com> writes:
> On 07/03/2019 04:36 AM, Andrew Morton wrote:
>> On Fri, 28 Jun 2019 10:50:31 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>
>>> Finishing up what the commit c2febafc67734a ("mm: convert generic code to
>>> 5-level paging") started out while levelling up P4D huge mapping support
>>> at par with PUD and PMD. A new arch call back arch_ioremap_p4d_supported()
>>> is being added which just maintains status quo (P4D huge map not supported)
>>> on x86, arm64 and powerpc.
>>
>> Does this have any runtime effects? If so, what are they and why? If
>> not, what's the actual point?
>
> It just finishes up what the previous commit c2febafc67734a ("mm: convert
> generic code to 5-level paging") left off with respect p4d based huge page
> enablement for ioremap. When HAVE_ARCH_HUGE_VMAP is enabled its just a simple
> check from the arch about the support, hence runtime effects are minimal.
The return value of arch_ioremap_p4d_supported() is stored in the
variable ioremap_p4d_capable which is then returned by
ioremap_p4d_enabled().
That is used by ioremap_try_huge_p4d() called from ioremap_p4d_range()
from ioremap_page_range().
The runtime effect is that it prevents ioremap_page_range() from trying
to create huge mappings at the p4d level on arches that don't support
it.
cheers
^ permalink raw reply
* Re: [PATCH 2/3] DMA mapping: Move SME handling to x86-specific files
From: Christoph Hellwig @ 2019-07-12 7:13 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: linux-s390, Mike Anderson, Konrad Rzeszutek Wilk, Robin Murphy,
x86, Ram Pai, linux-kernel, Alexey Dobriyan, Halil Pasic, iommu,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-fsdevel,
Thomas Gleixner, linuxppc-dev, Christoph Hellwig,
Marek Szyprowski
In-Reply-To: <20190712053631.9814-3-bauerman@linux.ibm.com>
Honestly I think this code should go away without any replacement.
There is no reason why we should have a special debug printk just
for one specific reason why there is a requirement for a large DMA
mask.
^ permalink raw reply
* [RFC PATCH kernel] powerpc/xive: Drop deregistered irqs
From: Alexey Kardashevskiy @ 2019-07-12 8:20 UTC (permalink / raw)
To: linuxppc-dev
Cc: Alexey Kardashevskiy, Alistair Popple, Greg Kurz, Nicholas Piggin,
Paul Mackerras, Cédric Le Goater, David Gibson
There is a race between releasing an irq on one cpu and fetching it
from XIVE on another cpu as there does not seem to be any locking between
these, probably because xive_irq_chip::irq_shutdown() is supposed to
remove the irq from all queues in the system which it does not do.
As a result, when such released irq appears in a queue, we take it
from the queue but we do not change the current priority on that cpu and
since there is no handler for the irq, EOI is never called and the cpu
current priority remains elevated (7 vs. 0xff==unmasked). If another irq
is assigned to the same cpu, then that device stops working until irq
is moved to another cpu or the device is reset.
This checks if irq is still registered, if not, it assumes no valid irq
was fetched from the loop and if there is none left, it continues to
the irq==0 case (not visible in this patch) and sets priority to 0xff
which is basically unmasking. This calls irq_to_desc() on a hot path now
which is a radix tree lookup; hopefully this won't be noticeable as
that tree is quite small.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Found it on P9 system with:
- a host with 8 cpus online
- a boot disk on ahci with its msix on cpu#0
- a guest with 2xGPUs + 6xNVLink + 4 cpus
- GPU#0 from the guest is bound to the same cpu#0.
Killing a guest killed ahci and therefore the host because of the race.
Note that VFIO masks interrupts first and only then resets the device.
Alternatives:
1. Fix xive_irq_chip::irq_shutdown() to walk through all cpu queues and
drop deregistered irqs.
2. Exploit chip->irq_get_irqchip_state function from
62e0468650c30f0298 "genirq: Add optional hardware synchronization for shutdown".
Both require deep XIVE knowledge which I do not have.
---
arch/powerpc/sysdev/xive/common.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index 082c7e1c20f0..65742e280337 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -148,8 +148,12 @@ static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
irq = xive_read_eq(&xc->queue[prio], just_peek);
/* Found something ? That's it */
- if (irq)
- break;
+ if (irq) {
+ /* Another CPU may have shut this irq down, check it */
+ if (irq_to_desc(irq))
+ break;
+ irq = 0;
+ }
/* Clear pending bits */
xc->pending_prio &= ~(1 << prio);
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH kernel] powerpc/xive: Drop deregistered irqs
From: Benjamin Herrenschmidt @ 2019-07-12 8:29 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Greg Kurz, Nicholas Piggin, Paul Mackerras, Cédric Le Goater,
Alistair Popple, David Gibson
In-Reply-To: <20190712082036.40440-1-aik@ozlabs.ru>
On Fri, 2019-07-12 at 18:20 +1000, Alexey Kardashevskiy wrote:
>
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index 082c7e1c20f0..65742e280337 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -148,8 +148,12 @@ static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
> irq = xive_read_eq(&xc->queue[prio], just_peek);
>
> /* Found something ? That's it */
> - if (irq)
> - break;
> + if (irq) {
> + /* Another CPU may have shut this irq down, check it */
> + if (irq_to_desc(irq))
What if it gets deregistered here .... ?
> + break;
> + irq = 0;
> + }
>
> /* Clear pending bits */
> xc->pending_prio &= ~(1 << prio);
Wouldn't it be better to check the return value from generic_handle_irq
instead ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH V2] mm/ioremap: Probe platform for p4d huge map support
From: Stephen Rothwell @ 2019-07-12 8:40 UTC (permalink / raw)
To: Michael Ellerman
Cc: x86, Anshuman Khandual, Peter Zijlstra, Catalin Marinas,
Dave Hansen, Will Deacon, Michal Hocko, linux-mm, Ingo Molnar,
linux-arm-kernel, Andy Lutomirski, Andrew Morton, linuxppc-dev,
Thomas Gleixner, Kirill A . Shutemov
In-Reply-To: <87tvbrennf.fsf@concordia.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 913 bytes --]
Hi all,
On Fri, 12 Jul 2019 17:07:48 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> The return value of arch_ioremap_p4d_supported() is stored in the
> variable ioremap_p4d_capable which is then returned by
> ioremap_p4d_enabled().
>
> That is used by ioremap_try_huge_p4d() called from ioremap_p4d_range()
> from ioremap_page_range().
When I first saw this, I wondered if we expect
arch_ioremap_p4d_supported() to ever return something that is not
computable at compile time. If not, why do we have this level of
redirection? Why not just make it a static inline functions defined in
an arch specific include file (or even just a CONFIG_ option)?
In particular, ioremap_p4d_enabled() either returns ioremap_p4d_capable
or 0 and is static to one file and has one call site ... The same is
true of ioremap_pud_enabled() and ioremap_pmd_enabled().
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [RFC PATCH kernel] powerpc/xive: Drop deregistered irqs
From: Alexey Kardashevskiy @ 2019-07-12 9:37 UTC (permalink / raw)
To: Benjamin Herrenschmidt, linuxppc-dev
Cc: Greg Kurz, Nicholas Piggin, Paul Mackerras, Cédric Le Goater,
Alistair Popple, David Gibson
In-Reply-To: <abbee9db2fa0abdaa2e01bb92bab920773a2ad8d.camel@kernel.crashing.org>
On 12/07/2019 18:29, Benjamin Herrenschmidt wrote:
> On Fri, 2019-07-12 at 18:20 +1000, Alexey Kardashevskiy wrote:
>>
>> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
>> index 082c7e1c20f0..65742e280337 100644
>> --- a/arch/powerpc/sysdev/xive/common.c
>> +++ b/arch/powerpc/sysdev/xive/common.c
>> @@ -148,8 +148,12 @@ static u32 xive_scan_interrupts(struct xive_cpu *xc, bool just_peek)
>> irq = xive_read_eq(&xc->queue[prio], just_peek);
>>
>> /* Found something ? That's it */
>> - if (irq)
>> - break;
>> + if (irq) {
>> + /* Another CPU may have shut this irq down, check it */
>> + if (irq_to_desc(irq))
>
> What if it gets deregistered here .... ?
Yeah that is the problem.
>
>> + break;
>> + irq = 0;
>> + }
>>
>> /* Clear pending bits */
>> xc->pending_prio &= ~(1 << prio);
>
> Wouldn't it be better to check the return value from generic_handle_irq
> instead ?
Where exactly, here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/irq.c#n614
If so, then in order to do EOI, I'll need the desc which is gone, or I
am missing the point?
--
Alexey
^ permalink raw reply
* [PATCH kernel v4 0/4 repost] powerpc/ioda2: Yet another attempt to allow DMA masks between 32 and 59
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
Alexey Kardashevskiy, David Gibson
This is an attempt to allow DMA masks between 32..59 which are not large
enough to use either a PHB3 bypass mode or a sketchy bypass. Depending
on the max order, up to 40 is usually available.
Changelogs are in the patches.
This is based on sha1
a2b6f26c264e Christophe Leroy "powerpc/module64: Use symbolic instructions names.".
Please comment. Thanks.
Alexey Kardashevskiy (4):
powerpc/powernv/ioda: Fix race in TCE level allocation
powerpc/iommu: Allow bypass-only for DMA
powerpc/powernv/ioda2: Allocate TCE table levels on demand for default
DMA window
powerpc/powernv/ioda2: Create bigger default window with 64k IOMMU
pages
arch/powerpc/include/asm/iommu.h | 8 +-
arch/powerpc/platforms/powernv/pci.h | 2 +-
arch/powerpc/kernel/dma-iommu.c | 11 ++-
arch/powerpc/kernel/iommu.c | 74 +++++++++++++------
arch/powerpc/platforms/powernv/pci-ioda-tce.c | 38 ++++++----
arch/powerpc/platforms/powernv/pci-ioda.c | 40 ++++++++--
6 files changed, 121 insertions(+), 52 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH kernel v4 1/4] powerpc/powernv/ioda: Fix race in TCE level allocation
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, stable, Alexey Kardashevskiy,
Oliver O'Halloran, David Gibson
In-Reply-To: <20190712094509.56695-1-aik@ozlabs.ru>
pnv_tce() returns a pointer to a TCE entry and originally a TCE table
would be pre-allocated. For the default case of 2GB window the table
needs only a single level and that is fine. However if more levels are
requested, it is possible to get a race when 2 threads want a pointer
to a TCE entry from the same page of TCEs.
This adds cmpxchg to handle the race. Note that once TCE is non-zero,
it cannot become zero again.
CC: stable@vger.kernel.org # v4.19+
Fixes: a68bd1267b72 ("powerpc/powernv/ioda: Allocate indirect TCE levels on demand")
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
The race occurs about 30 times in the first 3 minutes of copying files
via rsync and that's about it.
This fixes EEH's from
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=110810
---
Changes:
v2:
* replaced spin_lock with cmpxchg+readonce
---
arch/powerpc/platforms/powernv/pci-ioda-tce.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda-tce.c b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
index e28f03e1eb5e..8d6569590161 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda-tce.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
@@ -48,6 +48,9 @@ static __be64 *pnv_alloc_tce_level(int nid, unsigned int shift)
return addr;
}
+static void pnv_pci_ioda2_table_do_free_pages(__be64 *addr,
+ unsigned long size, unsigned int levels);
+
static __be64 *pnv_tce(struct iommu_table *tbl, bool user, long idx, bool alloc)
{
__be64 *tmp = user ? tbl->it_userspace : (__be64 *) tbl->it_base;
@@ -57,9 +60,9 @@ static __be64 *pnv_tce(struct iommu_table *tbl, bool user, long idx, bool alloc)
while (level) {
int n = (idx & mask) >> (level * shift);
- unsigned long tce;
+ unsigned long oldtce, tce = be64_to_cpu(READ_ONCE(tmp[n]));
- if (tmp[n] == 0) {
+ if (!tce) {
__be64 *tmp2;
if (!alloc)
@@ -70,10 +73,15 @@ static __be64 *pnv_tce(struct iommu_table *tbl, bool user, long idx, bool alloc)
if (!tmp2)
return NULL;
- tmp[n] = cpu_to_be64(__pa(tmp2) |
- TCE_PCI_READ | TCE_PCI_WRITE);
+ tce = __pa(tmp2) | TCE_PCI_READ | TCE_PCI_WRITE;
+ oldtce = be64_to_cpu(cmpxchg(&tmp[n], 0,
+ cpu_to_be64(tce)));
+ if (oldtce) {
+ pnv_pci_ioda2_table_do_free_pages(tmp2,
+ ilog2(tbl->it_level_size) + 3, 1);
+ tce = oldtce;
+ }
}
- tce = be64_to_cpu(tmp[n]);
tmp = __va(tce & ~(TCE_PCI_READ | TCE_PCI_WRITE));
idx &= ~mask;
--
2.17.1
^ permalink raw reply related
* [PATCH kernel v4 4/4] powerpc/powernv/ioda2: Create bigger default window with 64k IOMMU pages
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
Alexey Kardashevskiy, David Gibson
In-Reply-To: <20190712094509.56695-1-aik@ozlabs.ru>
At the moment we create a small window only for 32bit devices, the window
maps 0..2GB of the PCI space only. For other devices we either use
a sketchy bypass or hardware bypass but the former can only work if
the amount of RAM is no bigger than the device's DMA mask and the latter
requires devices to support at least 59bit DMA.
This extends the default DMA window to the maximum size possible to allow
a wider DMA mask than just 32bit. The default window size is now limited
by the the iommu_table::it_map allocation bitmap which is a contiguous
array, 1 bit per an IOMMU page.
This increases the default IOMMU page size from hard coded 4K to
the system page size to allow wider DMA masks.
This increases the level number to not exceed the max order allocation
limit per TCE level. By the same time, this keeps minimal levels number
as 2 in order to save memory.
As the extended window now overlaps the 32bit MMIO region, this adds
an area reservation to iommu_init_table().
After this change the default window size is 0x80000000000==1<<43 so
devices limited to DMA mask smaller than the amount of system RAM can
still use more than just 2GB of memory for DMA.
With the on-demand allocation of indirect TCE table levels enabled and
2 levels, the first TCE level size is just
1<<ceil((log2(0x7ffffffffff+1)-16)/2)=16384 TCEs or 2 system pages.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v4:
* fixed take/release ownership handlers
* fixed reserved region for tables with it_offset!=0 (this is not going
to be exploited here but still this is a correct behavior)
v3:
* fixed tce levels calculation
v2:
* adjusted level number to the max order
---
arch/powerpc/include/asm/iommu.h | 8 ++-
arch/powerpc/kernel/iommu.c | 74 ++++++++++++++++-------
arch/powerpc/platforms/powernv/pci-ioda.c | 40 ++++++++++--
3 files changed, 92 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index 18d342b815e4..a527a5fe1b01 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -111,6 +111,8 @@ struct iommu_table {
struct iommu_table_ops *it_ops;
struct kref it_kref;
int it_nid;
+ unsigned long it_reserved_start; /* Start of not-DMA-able (MMIO) area */
+ unsigned long it_reserved_end;
};
#define IOMMU_TABLE_USERSPACE_ENTRY_RO(tbl, entry) \
@@ -149,8 +151,10 @@ extern int iommu_tce_table_put(struct iommu_table *tbl);
/* Initializes an iommu_table based in values set in the passed-in
* structure
*/
-extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
- int nid);
+extern struct iommu_table *iommu_init_table_res(struct iommu_table *tbl,
+ int nid, unsigned long res_start, unsigned long res_end);
+#define iommu_init_table(tbl, nid) iommu_init_table_res((tbl), (nid), 0, 0)
+
#define IOMMU_TABLE_GROUP_MAX_TABLES 2
struct iommu_table_group;
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 0a67ce9f827e..2b501dc15352 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -633,11 +633,54 @@ static void iommu_table_clear(struct iommu_table *tbl)
#endif
}
+static void iommu_table_reserve_pages(struct iommu_table *tbl,
+ unsigned long res_start, unsigned long res_end)
+{
+ int i;
+
+ WARN_ON_ONCE(res_end >= res_start);
+ /*
+ * Reserve page 0 so it will not be used for any mappings.
+ * This avoids buggy drivers that consider page 0 to be invalid
+ * to crash the machine or even lose data.
+ */
+ if (tbl->it_offset == 0)
+ set_bit(0, tbl->it_map);
+
+ tbl->it_reserved_start = res_start;
+ tbl->it_reserved_end = res_end;
+
+ /* Check if res_start..res_end isn't empty and overlaps the table */
+ if (res_start && res_end &&
+ (tbl->it_offset + tbl->it_size < res_start ||
+ res_end < tbl->it_offset))
+ return;
+
+ for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i)
+ set_bit(i - tbl->it_offset, tbl->it_map);
+}
+
+static void iommu_table_release_pages(struct iommu_table *tbl)
+{
+ int i;
+
+ /*
+ * In case we have reserved the first bit, we should not emit
+ * the warning below.
+ */
+ if (tbl->it_offset == 0)
+ clear_bit(0, tbl->it_map);
+
+ for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i)
+ clear_bit(i - tbl->it_offset, tbl->it_map);
+}
+
/*
* Build a iommu_table structure. This contains a bit map which
* is used to manage allocation of the tce space.
*/
-struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
+struct iommu_table *iommu_init_table_res(struct iommu_table *tbl, int nid,
+ unsigned long res_start, unsigned long res_end)
{
unsigned long sz;
static int welcomed = 0;
@@ -656,13 +699,7 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
tbl->it_map = page_address(page);
memset(tbl->it_map, 0, sz);
- /*
- * Reserve page 0 so it will not be used for any mappings.
- * This avoids buggy drivers that consider page 0 to be invalid
- * to crash the machine or even lose data.
- */
- if (tbl->it_offset == 0)
- set_bit(0, tbl->it_map);
+ iommu_table_reserve_pages(tbl, res_start, res_end);
/* We only split the IOMMU table if we have 1GB or more of space */
if ((tbl->it_size << tbl->it_page_shift) >= (1UL * 1024 * 1024 * 1024))
@@ -714,12 +751,7 @@ static void iommu_table_free(struct kref *kref)
return;
}
- /*
- * In case we have reserved the first bit, we should not emit
- * the warning below.
- */
- if (tbl->it_offset == 0)
- clear_bit(0, tbl->it_map);
+ iommu_table_release_pages(tbl);
/* verify that table contains no entries */
if (!bitmap_empty(tbl->it_map, tbl->it_size))
@@ -1024,15 +1056,14 @@ int iommu_take_ownership(struct iommu_table *tbl)
for (i = 0; i < tbl->nr_pools; i++)
spin_lock(&tbl->pools[i].lock);
- if (tbl->it_offset == 0)
- clear_bit(0, tbl->it_map);
+ iommu_table_release_pages(tbl);
if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
pr_err("iommu_tce: it_map is not empty");
ret = -EBUSY;
- /* Restore bit#0 set by iommu_init_table() */
- if (tbl->it_offset == 0)
- set_bit(0, tbl->it_map);
+ /* Undo iommu_table_release_pages, i.e. restore bit#0, etc */
+ iommu_table_reserve_pages(tbl, tbl->it_reserved_start,
+ tbl->it_reserved_end);
} else {
memset(tbl->it_map, 0xff, sz);
}
@@ -1055,9 +1086,8 @@ void iommu_release_ownership(struct iommu_table *tbl)
memset(tbl->it_map, 0, sz);
- /* Restore bit#0 set by iommu_init_table() */
- if (tbl->it_offset == 0)
- set_bit(0, tbl->it_map);
+ iommu_table_reserve_pages(tbl, tbl->it_reserved_start,
+ tbl->it_reserved_end);
for (i = 0; i < tbl->nr_pools; i++)
spin_unlock(&tbl->pools[i].lock);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index d8080558d020..874909c6f314 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2420,6 +2420,7 @@ static long pnv_pci_ioda2_setup_default_config(struct pnv_ioda_pe *pe)
{
struct iommu_table *tbl = NULL;
long rc;
+ unsigned long res_start, res_end;
/*
* crashkernel= specifies the kdump kernel's maximum memory at
@@ -2433,19 +2434,46 @@ static long pnv_pci_ioda2_setup_default_config(struct pnv_ioda_pe *pe)
* DMA window can be larger than available memory, which will
* cause errors later.
*/
- const u64 window_size = min((u64)pe->table_group.tce32_size, max_memory);
+ const u64 maxblock = 1UL << (PAGE_SHIFT + MAX_ORDER - 1);
- rc = pnv_pci_ioda2_create_table(&pe->table_group, 0,
- IOMMU_PAGE_SHIFT_4K,
- window_size,
- POWERNV_IOMMU_DEFAULT_LEVELS, false, &tbl);
+ /*
+ * We create the default window as big as we can. The constraint is
+ * the max order of allocation possible. The TCE tableis likely to
+ * end up being multilevel and with on-demand allocation in place,
+ * the initial use is not going to be huge as the default window aims
+ * to support cripplied devices (i.e. not fully 64bit DMAble) only.
+ */
+ /* iommu_table::it_map uses 1 bit per IOMMU page, hence 8 */
+ const u64 window_size = min((maxblock * 8) << PAGE_SHIFT, max_memory);
+ /* Each TCE level cannot exceed maxblock so go multilevel if needed */
+ unsigned long tces_order = ilog2(window_size >> PAGE_SHIFT);
+ unsigned long tcelevel_order = ilog2(maxblock >> 3);
+ unsigned int levels = tces_order / tcelevel_order;
+
+ if (tces_order % tcelevel_order)
+ levels += 1;
+ /*
+ * We try to stick to default levels (which is >1 at the moment) in
+ * order to save memory by relying on on-demain TCE level allocation.
+ */
+ levels = max_t(unsigned int, levels, POWERNV_IOMMU_DEFAULT_LEVELS);
+
+ rc = pnv_pci_ioda2_create_table(&pe->table_group, 0, PAGE_SHIFT,
+ window_size, levels, false, &tbl);
if (rc) {
pe_err(pe, "Failed to create 32-bit TCE table, err %ld",
rc);
return rc;
}
- iommu_init_table(tbl, pe->phb->hose->node);
+ /* We use top part of 32bit space for MMIO so exclude it from DMA */
+ res_start = 0;
+ res_end = 0;
+ if (window_size > pe->phb->ioda.m32_pci_base) {
+ res_start = pe->phb->ioda.m32_pci_base >> tbl->it_page_shift;
+ res_end = min(window_size, SZ_4G) >> tbl->it_page_shift;
+ }
+ iommu_init_table_res(tbl, pe->phb->hose->node, res_start, res_end);
rc = pnv_pci_ioda2_set_window(&pe->table_group, 0, tbl);
if (rc) {
--
2.17.1
^ permalink raw reply related
* [PATCH kernel v4 2/4] powerpc/iommu: Allow bypass-only for DMA
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
Alexey Kardashevskiy, David Gibson
In-Reply-To: <20190712094509.56695-1-aik@ozlabs.ru>
POWER8 and newer support a bypass mode which maps all host memory to
PCI buses so an IOMMU table is not always required. However if we fail to
create such a table, the DMA setup fails and the kernel does not boot.
This skips the 32bit DMA setup check if the bypass is can be selected.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
This minor thing helped me debugging next 2 patches so it can help
somebody else too.
---
arch/powerpc/kernel/dma-iommu.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/dma-iommu.c b/arch/powerpc/kernel/dma-iommu.c
index a0879674a9c8..c963d704fa31 100644
--- a/arch/powerpc/kernel/dma-iommu.c
+++ b/arch/powerpc/kernel/dma-iommu.c
@@ -122,18 +122,17 @@ int dma_iommu_dma_supported(struct device *dev, u64 mask)
{
struct iommu_table *tbl = get_iommu_table_base(dev);
- if (!tbl) {
- dev_info(dev, "Warning: IOMMU dma not supported: mask 0x%08llx"
- ", table unavailable\n", mask);
- return 0;
- }
-
if (dev_is_pci(dev) && dma_iommu_bypass_supported(dev, mask)) {
dev->archdata.iommu_bypass = true;
dev_dbg(dev, "iommu: 64-bit OK, using fixed ops\n");
return 1;
}
+ if (!tbl) {
+ dev_err(dev, "Warning: IOMMU dma not supported: mask 0x%08llx, table unavailable\n", mask);
+ return 0;
+ }
+
if (tbl->it_offset > (mask >> tbl->it_page_shift)) {
dev_info(dev, "Warning: IOMMU offset too big for device mask\n");
dev_info(dev, "mask: 0x%08llx, table offset: 0x%08lx\n",
--
2.17.1
^ permalink raw reply related
* [PATCH kernel v4 3/4] powerpc/powernv/ioda2: Allocate TCE table levels on demand for default DMA window
From: Alexey Kardashevskiy @ 2019-07-12 9:45 UTC (permalink / raw)
To: linuxppc-dev
Cc: Sam Bobroff, Alistair Popple, Oliver O'Halloran,
Alexey Kardashevskiy, David Gibson
In-Reply-To: <20190712094509.56695-1-aik@ozlabs.ru>
We allocate only the first level of multilevel TCE tables for KVM
already (alloc_userspace_copy==true), and the rest is allocated on demand.
This is not enabled though for bare metal.
This removes the KVM limitation (implicit, via the alloc_userspace_copy
parameter) and always allocates just the first level. The on-demand
allocation of missing levels is already implemented.
As from now on DMA map might happen with disabled interrupts, this
allocates TCEs with GFP_ATOMIC; otherwise lockdep reports errors 1].
In practice just a single page is allocated there so chances for failure
are quite low.
To save time when creating a new clean table, this skips non-allocated
indirect TCE entries in pnv_tce_free just like we already do in
the VFIO IOMMU TCE driver.
This changes the default level number from 1 to 2 to reduce the amount
of memory required for the default 32bit DMA window at the boot time.
The default window size is up to 2GB which requires 4MB of TCEs which is
unlikely to be used entirely or at all as most devices these days are
64bit capable so by switching to 2 levels by default we save 4032KB of
RAM per a device.
While at this, add __GFP_NOWARN to alloc_pages_node() as the userspace
can trigger this path via VFIO, see the failure and try creating a table
again with different parameters which might succeed.
[1]:
===
BUG: sleeping function called from invalid context at mm/page_alloc.c:4596
in_atomic(): 1, irqs_disabled(): 1, pid: 1038, name: scsi_eh_1
2 locks held by scsi_eh_1/1038:
#0: 000000005efd659a (&host->eh_mutex){+.+.}, at: ata_eh_acquire+0x34/0x80
#1: 0000000006cf56a6 (&(&host->lock)->rlock){....}, at: ata_exec_internal_sg+0xb0/0x5c0
irq event stamp: 500
hardirqs last enabled at (499): [<c000000000cb8a74>] _raw_spin_unlock_irqrestore+0x94/0xd0
hardirqs last disabled at (500): [<c000000000cb85c4>] _raw_spin_lock_irqsave+0x44/0x120
softirqs last enabled at (0): [<c000000000101120>] copy_process.isra.4.part.5+0x640/0x1a80
softirqs last disabled at (0): [<0000000000000000>] 0x0
CPU: 73 PID: 1038 Comm: scsi_eh_1 Not tainted 5.2.0-rc6-le_nv2_aikATfstn1-p1 #634
Call Trace:
[c000003d064cef50] [c000000000c8e6c4] dump_stack+0xe8/0x164 (unreliable)
[c000003d064cefa0] [c00000000014ed78] ___might_sleep+0x2f8/0x310
[c000003d064cf020] [c0000000003ca084] __alloc_pages_nodemask+0x2a4/0x1560
[c000003d064cf220] [c0000000000c2530] pnv_alloc_tce_level.isra.0+0x90/0x130
[c000003d064cf290] [c0000000000c2888] pnv_tce+0x128/0x3b0
[c000003d064cf360] [c0000000000c2c00] pnv_tce_build+0xb0/0xf0
[c000003d064cf3c0] [c0000000000bbd9c] pnv_ioda2_tce_build+0x3c/0xb0
[c000003d064cf400] [c00000000004cfe0] ppc_iommu_map_sg+0x210/0x550
[c000003d064cf510] [c00000000004b7a4] dma_iommu_map_sg+0x74/0xb0
[c000003d064cf530] [c000000000863944] ata_qc_issue+0x134/0x470
[c000003d064cf5b0] [c000000000863ec4] ata_exec_internal_sg+0x244/0x5c0
[c000003d064cf700] [c0000000008642d0] ata_exec_internal+0x90/0xe0
[c000003d064cf780] [c0000000008650ac] ata_dev_read_id+0x2ec/0x640
[c000003d064cf8d0] [c000000000878e28] ata_eh_recover+0x948/0x16d0
[c000003d064cfa10] [c00000000087d760] sata_pmp_error_handler+0x480/0xbf0
[c000003d064cfbc0] [c000000000884624] ahci_error_handler+0x74/0xe0
[c000003d064cfbf0] [c000000000879fa8] ata_scsi_port_error_handler+0x2d8/0x7c0
[c000003d064cfca0] [c00000000087a544] ata_scsi_error+0xb4/0x100
[c000003d064cfd00] [c000000000802450] scsi_error_handler+0x120/0x510
[c000003d064cfdb0] [c000000000140c48] kthread+0x1b8/0x1c0
[c000003d064cfe20] [c00000000000bd8c] ret_from_kernel_thread+0x5c/0x70
ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
irq event stamp: 2305
========================================================
hardirqs last enabled at (2305): [<c00000000000e4c8>] fast_exc_return_irq+0x28/0x34
hardirqs last disabled at (2303): [<c000000000cb9fd0>] __do_softirq+0x4a0/0x654
WARNING: possible irq lock inversion dependency detected
5.2.0-rc6-le_nv2_aikATfstn1-p1 #634 Tainted: G W
softirqs last enabled at (2304): [<c000000000cba054>] __do_softirq+0x524/0x654
softirqs last disabled at (2297): [<c00000000010f278>] irq_exit+0x128/0x180
--------------------------------------------------------
swapper/0/0 just changed the state of lock:
0000000006cf56a6 (&(&host->lock)->rlock){-...}, at: ahci_single_level_irq_intr+0xac/0x120
but this lock took another, HARDIRQ-unsafe lock in the past:
(fs_reclaim){+.+.}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(fs_reclaim);
local_irq_disable();
lock(&(&host->lock)->rlock);
lock(fs_reclaim);
<Interrupt>
lock(&(&host->lock)->rlock);
*** DEADLOCK ***
no locks held by swapper/0/0.
the shortest dependencies between 2nd lock and 1st lock:
-> (fs_reclaim){+.+.} ops: 167579 {
HARDIRQ-ON-W at:
lock_acquire+0xf8/0x2a0
fs_reclaim_acquire.part.23+0x44/0x60
kmem_cache_alloc_node_trace+0x80/0x590
alloc_desc+0x64/0x270
__irq_alloc_descs+0x2e4/0x3a0
irq_domain_alloc_descs+0xb0/0x150
irq_create_mapping+0x168/0x2c0
xics_smp_probe+0x2c/0x98
pnv_smp_probe+0x40/0x9c
smp_prepare_cpus+0x524/0x6c4
kernel_init_freeable+0x1b4/0x650
kernel_init+0x2c/0x148
ret_from_kernel_thread+0x5c/0x70
SOFTIRQ-ON-W at:
lock_acquire+0xf8/0x2a0
fs_reclaim_acquire.part.23+0x44/0x60
kmem_cache_alloc_node_trace+0x80/0x590
alloc_desc+0x64/0x270
__irq_alloc_descs+0x2e4/0x3a0
irq_domain_alloc_descs+0xb0/0x150
irq_create_mapping+0x168/0x2c0
xics_smp_probe+0x2c/0x98
pnv_smp_probe+0x40/0x9c
smp_prepare_cpus+0x524/0x6c4
kernel_init_freeable+0x1b4/0x650
kernel_init+0x2c/0x148
ret_from_kernel_thread+0x5c/0x70
INITIAL USE at:
lock_acquire+0xf8/0x2a0
fs_reclaim_acquire.part.23+0x44/0x60
kmem_cache_alloc_node_trace+0x80/0x590
alloc_desc+0x64/0x270
__irq_alloc_descs+0x2e4/0x3a0
irq_domain_alloc_descs+0xb0/0x150
irq_create_mapping+0x168/0x2c0
xics_smp_probe+0x2c/0x98
pnv_smp_probe+0x40/0x9c
smp_prepare_cpus+0x524/0x6c4
kernel_init_freeable+0x1b4/0x650
kernel_init+0x2c/0x148
ret_from_kernel_thread+0x5c/0x70
}
===
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Alistair Popple <alistair@popple.id.au>
---
Changes:
v4:
* added lockdep output to the commit log to justify ATOMIC in alloc_pages_node
v2:
* added __GFP_NOWARN to alloc_pages_node
---
arch/powerpc/platforms/powernv/pci.h | 2 +-
arch/powerpc/platforms/powernv/pci-ioda-tce.c | 20 +++++++++----------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 469c24463247..f914f0b14e4e 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -219,7 +219,7 @@ extern struct iommu_table_group *pnv_npu_compound_attach(
struct pnv_ioda_pe *pe);
/* pci-ioda-tce.c */
-#define POWERNV_IOMMU_DEFAULT_LEVELS 1
+#define POWERNV_IOMMU_DEFAULT_LEVELS 2
#define POWERNV_IOMMU_MAX_LEVELS 5
extern int pnv_tce_build(struct iommu_table *tbl, long index, long npages,
diff --git a/arch/powerpc/platforms/powernv/pci-ioda-tce.c b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
index 8d6569590161..a0b9c0c23ed2 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda-tce.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda-tce.c
@@ -36,7 +36,8 @@ static __be64 *pnv_alloc_tce_level(int nid, unsigned int shift)
struct page *tce_mem = NULL;
__be64 *addr;
- tce_mem = alloc_pages_node(nid, GFP_KERNEL, shift - PAGE_SHIFT);
+ tce_mem = alloc_pages_node(nid, GFP_ATOMIC | __GFP_NOWARN,
+ shift - PAGE_SHIFT);
if (!tce_mem) {
pr_err("Failed to allocate a TCE memory, level shift=%d\n",
shift);
@@ -169,6 +170,9 @@ void pnv_tce_free(struct iommu_table *tbl, long index, long npages)
if (ptce)
*ptce = cpu_to_be64(0);
+ else
+ /* Skip the rest of the level */
+ i |= tbl->it_level_size - 1;
}
}
@@ -268,7 +272,6 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
unsigned int table_shift = max_t(unsigned int, entries_shift + 3,
PAGE_SHIFT);
const unsigned long tce_table_size = 1UL << table_shift;
- unsigned int tmplevels = levels;
if (!levels || (levels > POWERNV_IOMMU_MAX_LEVELS))
return -EINVAL;
@@ -276,9 +279,6 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
if (!is_power_of_2(window_size))
return -EINVAL;
- if (alloc_userspace_copy && (window_size > (1ULL << 32)))
- tmplevels = 1;
-
/* Adjust direct table size from window_size and levels */
entries_shift = (entries_shift + levels - 1) / levels;
level_shift = entries_shift + 3;
@@ -289,7 +289,7 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
/* Allocate TCE table */
addr = pnv_pci_ioda2_table_do_alloc_pages(nid, level_shift,
- tmplevels, tce_table_size, &offset, &total_allocated);
+ 1, tce_table_size, &offset, &total_allocated);
/* addr==NULL means that the first level allocation failed */
if (!addr)
@@ -300,18 +300,18 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
* we did not allocate as much as we wanted,
* release partially allocated table.
*/
- if (tmplevels == levels && offset < tce_table_size)
+ if (levels == 1 && offset < tce_table_size)
goto free_tces_exit;
/* Allocate userspace view of the TCE table */
if (alloc_userspace_copy) {
offset = 0;
uas = pnv_pci_ioda2_table_do_alloc_pages(nid, level_shift,
- tmplevels, tce_table_size, &offset,
+ 1, tce_table_size, &offset,
&total_allocated_uas);
if (!uas)
goto free_tces_exit;
- if (tmplevels == levels && (offset < tce_table_size ||
+ if (levels == 1 && (offset < tce_table_size ||
total_allocated_uas != total_allocated))
goto free_uas_exit;
}
@@ -326,7 +326,7 @@ long pnv_pci_ioda2_table_alloc_pages(int nid, __u64 bus_offset,
pr_debug("Created TCE table: ws=%08llx ts=%lx @%08llx base=%lx uas=%p levels=%d/%d\n",
window_size, tce_table_size, bus_offset, tbl->it_base,
- tbl->it_userspace, tmplevels, levels);
+ tbl->it_userspace, 1, levels);
return 0;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V2] mm/ioremap: Probe platform for p4d huge map support
From: Anshuman Khandual @ 2019-07-12 9:58 UTC (permalink / raw)
To: Michael Ellerman, Andrew Morton
Cc: Stephen Rothwell, x86, Peter Zijlstra, Catalin Marinas,
Dave Hansen, Will Deacon, Michal Hocko, linux-mm, Ingo Molnar,
linux-arm-kernel, Andy Lutomirski, Thomas Gleixner, linuxppc-dev,
Kirill A . Shutemov
In-Reply-To: <87tvbrennf.fsf@concordia.ellerman.id.au>
On 07/12/2019 12:37 PM, Michael Ellerman wrote:
> Anshuman Khandual <anshuman.khandual@arm.com> writes:
>> On 07/03/2019 04:36 AM, Andrew Morton wrote:
>>> On Fri, 28 Jun 2019 10:50:31 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>>
>>>> Finishing up what the commit c2febafc67734a ("mm: convert generic code to
>>>> 5-level paging") started out while levelling up P4D huge mapping support
>>>> at par with PUD and PMD. A new arch call back arch_ioremap_p4d_supported()
>>>> is being added which just maintains status quo (P4D huge map not supported)
>>>> on x86, arm64 and powerpc.
>>>
>>> Does this have any runtime effects? If so, what are they and why? If
>>> not, what's the actual point?
>>
>> It just finishes up what the previous commit c2febafc67734a ("mm: convert
>> generic code to 5-level paging") left off with respect p4d based huge page
>> enablement for ioremap. When HAVE_ARCH_HUGE_VMAP is enabled its just a simple
>> check from the arch about the support, hence runtime effects are minimal.
>
> The return value of arch_ioremap_p4d_supported() is stored in the
> variable ioremap_p4d_capable which is then returned by
> ioremap_p4d_enabled().
>
> That is used by ioremap_try_huge_p4d() called from ioremap_p4d_range()
> from ioremap_page_range().
That is right.
>
> The runtime effect is that it prevents ioremap_page_range() from trying
> to create huge mappings at the p4d level on arches that don't support
> it.
But now after first checking with an arch callback. Previously p4d huge
mappings were disabled on all platforms as ioremap_p4d_capable remained
clear through out being a static.
^ permalink raw reply
* Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
From: Hoan Tran OS @ 2019-07-12 10:56 UTC (permalink / raw)
To: Michal Hocko
Cc: Catalin Marinas, Heiko Carstens, open list:MEMORY MANAGEMENT,
Paul Mackerras, H . Peter Anvin, sparclinux@vger.kernel.org,
Alexander Duyck, linux-s390@vger.kernel.org, x86@kernel.org,
Mike Rapoport, Christian Borntraeger, Ingo Molnar,
Vlastimil Babka, Open Source Submission, Pavel Tatashin,
Vasily Gorbik, Will Deacon, Borislav Petkov, Thomas Gleixner,
linux-arm-kernel@lists.infradead.org, Oscar Salvador,
linux-kernel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org, David S . Miller
In-Reply-To: <20190712070247.GM29483@dhcp22.suse.cz>
Hi,
On 7/12/19 2:02 PM, Michal Hocko wrote:
> On Thu 11-07-19 23:25:44, Hoan Tran OS wrote:
>> In NUMA layout which nodes have memory ranges that span across other nodes,
>> the mm driver can detect the memory node id incorrectly.
>>
>> For example, with layout below
>> Node 0 address: 0000 xxxx 0000 xxxx
>> Node 1 address: xxxx 1111 xxxx 1111
>>
>> Note:
>> - Memory from low to high
>> - 0/1: Node id
>> - x: Invalid memory of a node
>>
>> When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
>> config, mm only checks the memory validity but not the node id.
>> Because of that, Node 1 also detects the memory from node 0 as below
>> when it scans from the start address to the end address of node 1.
>>
>> Node 0 address: 0000 xxxx xxxx xxxx
>> Node 1 address: xxxx 1111 1111 1111
>>
>> This layout could occur on any architecture. This patch enables
>> CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA to fix this issue.
>
> Yes it can occur on any arch but most sane platforms simply do not
> overlap physical ranges. So I do not really see any reason to
> unconditionally enable the config for everybody. What is an advantage?
>
As I observed from arch folder, there are 9 arch support NUMA config.
./arch/ia64/Kconfig:387:config NUMA
./arch/powerpc/Kconfig:582:config NUMA
./arch/sparc/Kconfig:281:config NUMA
./arch/alpha/Kconfig:557:config NUMA
./arch/sh/mm/Kconfig:112:config NUMA
./arch/arm64/Kconfig:841:config NUMA
./arch/x86/Kconfig:1531:config NUMA
./arch/mips/Kconfig:2646:config NUMA
./arch/s390/Kconfig:441:config NUMA
And there are 5 arch enables CONFIG_NODES_SPAN_OTHER_NODES with NUMA
arch/powerpc/Kconfig:637:config NODES_SPAN_OTHER_NODES
arch/sparc/Kconfig:299:config NODES_SPAN_OTHER_NODES
arch/x86/Kconfig:1575:config NODES_SPAN_OTHER_NODES
arch/s390/Kconfig:446:config NODES_SPAN_OTHER_NODES
arch/arm64 (which I intended to enable in the original patch)
It would be good if we can enable it by-default. Otherwise, let arch
enables it by them-self. Do you have any suggestions?
Thanks
Hoan
^ permalink raw reply
* Re: [PATCH v9 05/10] namei: O_BENEATH-style path resolution flags
From: Aleksa Sarai @ 2019-07-12 10:57 UTC (permalink / raw)
To: Al Viro
Cc: linux-ia64, linux-sh, Alexei Starovoitov, linux-kernel,
David Howells, linux-kselftest, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai,
linux-arm-kernel, linux-mips, linux-xtensa, Kees Cook,
Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k,
Andy Lutomirski, Shuah Khan, David Drysdale, Christian Brauner,
J. Bruce Fields, linux-parisc, linux-api, Chanho Min, Jeff Layton,
Oleg Nesterov, Eric Biederman, linux-alpha, linux-fsdevel,
Andrew Morton, Linus Torvalds, containers
In-Reply-To: <20190712043341.GI17978@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]
On 2019-07-12, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Jul 07, 2019 at 12:57:32AM +1000, Aleksa Sarai wrote:
> > @@ -1442,8 +1464,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > struct inode *inode = nd->inode;
> >
> > while (1) {
> > - if (path_equal(&nd->path, &nd->root))
> > + if (path_equal(&nd->path, &nd->root)) {
> > + if (unlikely(nd->flags & LOOKUP_BENEATH))
> > + return -EXDEV;
>
> > @@ -1468,6 +1493,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > return -ECHILD;
> > if (&mparent->mnt == nd->path.mnt)
> > break;
> > + if (unlikely(nd->flags & LOOKUP_XDEV))
> > + return -EXDEV;
> > /* we know that mountpoint was pinned */
> > nd->path.dentry = mountpoint;
> > nd->path.mnt = &mparent->mnt;
> > @@ -1482,6 +1509,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
> > return -ECHILD;
> > if (!mounted)
> > break;
> > + if (unlikely(nd->flags & LOOKUP_XDEV))
> > + return -EXDEV;
>
> Are you sure these failure exits in follow_dotdot_rcu() won't give
> suprious hard errors?
I could switch to -ECHILD for the *_rcu() checks if you'd prefer that.
Though, I'd have (probably naively) thought that you'd have already
gotten -ECHILD from the seqlock checks if there was a race during ".."
handling.
> > + if (unlikely(nd->flags & LOOKUP_BENEATH)) {
> > + error = dirfd_path_init(nd);
> > + if (unlikely(error))
> > + return ERR_PTR(error);
> > + nd->root = nd->path;
> > + if (!(nd->flags & LOOKUP_RCU))
> > + path_get(&nd->root);
> > + }
> > if (*s == '/') {
> > if (likely(!nd->root.mnt))
> > set_root(nd);
> > @@ -2350,9 +2400,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
> > s = ERR_PTR(error);
> > return s;
> > }
> > - error = dirfd_path_init(nd);
> > - if (unlikely(error))
> > - return ERR_PTR(error);
> > + if (likely(!nd->path.mnt)) {
>
> Is that a weird way of saying "if we hadn't already called dirfd_path_init()"?
Yes. I did it to be more consistent with the other "have we got the
root" checks elsewhere. Is there another way you'd prefer I do it?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v9 04/10] namei: split out nd->dfd handling to dirfd_path_init
From: Aleksa Sarai @ 2019-07-12 12:07 UTC (permalink / raw)
To: Al Viro
Cc: linux-ia64, linux-sh, Alexei Starovoitov, linux-kernel,
David Howells, linux-kselftest, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai,
linux-arm-kernel, linux-mips, linux-xtensa, Kees Cook,
Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k,
Andy Lutomirski, Shuah Khan, David Drysdale, Christian Brauner,
J. Bruce Fields, linux-parisc, linux-api, Chanho Min, Jeff Layton,
Oleg Nesterov, Eric Biederman, linux-alpha, linux-fsdevel,
Andrew Morton, Linus Torvalds, containers
In-Reply-To: <20190712042050.GH17978@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 960 bytes --]
On 2019-07-12, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Jul 07, 2019 at 12:57:31AM +1000, Aleksa Sarai wrote:
> > Previously, path_init's handling of *at(dfd, ...) was only done once,
> > but with LOOKUP_BENEATH (and LOOKUP_IN_ROOT) we have to parse the
> > initial nd->path at different times (before or after absolute path
> > handling) depending on whether we have been asked to scope resolution
> > within a root.
>
> > if (*s == '/') {
> > - set_root(nd);
> > - if (likely(!nd_jump_root(nd)))
> > - return s;
> > - return ERR_PTR(-ECHILD);
>
> > + if (likely(!nd->root.mnt))
> > + set_root(nd);
>
> How can we get there with non-NULL nd->root.mnt, when LOOKUP_ROOT case
> has been already handled by that point?
Yup, you're completely right. I will remove the
if (!nd->root.mnt)
in the next version.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
From: Michal Hocko @ 2019-07-12 12:12 UTC (permalink / raw)
To: Hoan Tran OS
Cc: Catalin Marinas, Heiko Carstens, open list:MEMORY MANAGEMENT,
Paul Mackerras, H . Peter Anvin, sparclinux@vger.kernel.org,
Alexander Duyck, linux-s390@vger.kernel.org, x86@kernel.org,
Mike Rapoport, Christian Borntraeger, Ingo Molnar,
Vlastimil Babka, Open Source Submission, Pavel Tatashin,
Vasily Gorbik, Will Deacon, Borislav Petkov, Thomas Gleixner,
linux-arm-kernel@lists.infradead.org, Oscar Salvador,
linux-kernel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org, David S . Miller
In-Reply-To: <586ae736-a429-cf94-1520-1a94ffadad88@os.amperecomputing.com>
On Fri 12-07-19 10:56:47, Hoan Tran OS wrote:
[...]
> It would be good if we can enable it by-default. Otherwise, let arch
> enables it by them-self. Do you have any suggestions?
I can hardly make any suggestions when it is not really clear _why_ you
want to remove this config option in the first place. Please explain
what motivated you to make this change.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v9 04/10] namei: split out nd->dfd handling to dirfd_path_init
From: Aleksa Sarai @ 2019-07-12 12:12 UTC (permalink / raw)
To: Al Viro
Cc: linux-ia64, linux-sh, Alexei Starovoitov, linux-kernel,
David Howells, linux-kselftest, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai,
linux-arm-kernel, linux-mips, linux-xtensa, Kees Cook,
Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k,
Andy Lutomirski, Shuah Khan, David Drysdale, Christian Brauner,
J. Bruce Fields, linux-parisc, linux-api, Chanho Min, Jeff Layton,
Oleg Nesterov, Eric Biederman, linux-alpha, linux-fsdevel,
Andrew Morton, Linus Torvalds, containers
In-Reply-To: <20190712120743.mka3vl5t4zndc5wj@yavin>
[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]
On 2019-07-12, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2019-07-12, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Sun, Jul 07, 2019 at 12:57:31AM +1000, Aleksa Sarai wrote:
> > > Previously, path_init's handling of *at(dfd, ...) was only done once,
> > > but with LOOKUP_BENEATH (and LOOKUP_IN_ROOT) we have to parse the
> > > initial nd->path at different times (before or after absolute path
> > > handling) depending on whether we have been asked to scope resolution
> > > within a root.
> >
> > > if (*s == '/') {
> > > - set_root(nd);
> > > - if (likely(!nd_jump_root(nd)))
> > > - return s;
> > > - return ERR_PTR(-ECHILD);
> >
> > > + if (likely(!nd->root.mnt))
> > > + set_root(nd);
> >
> > How can we get there with non-NULL nd->root.mnt, when LOOKUP_ROOT case
> > has been already handled by that point?
>
> Yup, you're completely right. I will remove the
> if (!nd->root.mnt)
> in the next version.
Ah sorry, there is a reason for it -- later in the series the
LOOKUP_BENEATH case means that you might end up with a non-NULL
nd->root.mnt. If you want, I can move the addition of the conditional to
later in the series (it was easier to split the patch by-hunk back when
you originally asked me to split out dirfd_path_init()).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v9 01/10] namei: obey trailing magic-link DAC permissions
From: Aleksa Sarai @ 2019-07-12 12:20 UTC (permalink / raw)
To: Al Viro
Cc: linux-ia64, linux-sh, Alexei Starovoitov, linux-kernel,
David Howells, linux-kselftest, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai,
linux-arm-kernel, linux-mips, linux-xtensa, Kees Cook,
Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k,
Andy Lutomirski, Shuah Khan, David Drysdale, Christian Brauner,
J. Bruce Fields, linux-parisc, linux-api, Chanho Min, Jeff Layton,
Oleg Nesterov, Eric Biederman, linux-alpha, linux-fsdevel,
Andrew Morton, Linus Torvalds, containers
In-Reply-To: <20190712041454.GG17978@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]
On 2019-07-12, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Jul 07, 2019 at 12:57:28AM +1000, Aleksa Sarai wrote:
> > @@ -514,7 +516,14 @@ static void set_nameidata(struct nameidata *p, int dfd, struct filename *name)
> > p->stack = p->internal;
> > p->dfd = dfd;
> > p->name = name;
> > - p->total_link_count = old ? old->total_link_count : 0;
> > + p->total_link_count = 0;
> > + p->acc_mode = 0;
> > + p->opath_mask = FMODE_PATH_READ | FMODE_PATH_WRITE;
> > + if (old) {
> > + p->total_link_count = old->total_link_count;
> > + p->acc_mode = old->acc_mode;
> > + p->opath_mask = old->opath_mask;
> > + }
>
> Huh? Could somebody explain why traversals of NFS4 referrals should inherit
> ->acc_mode and ->opath_mask?
I'll be honest -- I don't understand what set_nameidata() did so I just
did what I thought would be an obvious change (to just copy the
contents). I thought it was related to some aspect of the symlink stack
handling.
In that case, should they both be set to 0 on set_nameidata()? This will
mean that fd re-opening (or magic-link opening) through a
set_nameidata() would always fail.
> > static __always_inline
> > -const char *get_link(struct nameidata *nd)
> > +const char *get_link(struct nameidata *nd, bool trailing)
> > {
> > struct saved *last = nd->stack + nd->depth - 1;
> > struct dentry *dentry = last->link.dentry;
> > @@ -1081,6 +1134,44 @@ const char *get_link(struct nameidata *nd)
> > } else {
> > res = get(dentry, inode, &last->done);
> > }
> > + /* If we just jumped it was because of a magic-link. */
> > + if (unlikely(nd->flags & LOOKUP_JUMPED)) {
> [...]
> In any case, this "bool trailing" is completely wrong; whether that
> check belongs in trailing_symlink() or (some of) its callers, putting
> it into get_link() is a mistake, forced by kludgy check for procfs-style
> symlinks.
The error path for LOOKUP_JUMPED comes from the old O_BENEATH patchset,
but all of the "bool trailing" logic is definitely my gaff (I was
quietly hoping you'd have a much better solution than the whole
get_link() thing -- it definitely felt very kludgey to write).
I will work on the suggestion in your follow-up email. Thanks!
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use
From: Michael Ellerman @ 2019-07-12 12:22 UTC (permalink / raw)
To: Christophe Leroy, Aneesh Kumar K.V, npiggin, paulus; +Cc: linuxppc-dev
In-Reply-To: <a9efab7b-0c61-5224-f936-82db1039d159@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 12/07/2019 à 08:25, Michael Ellerman a écrit :
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
...
>>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>>> index 11caa0291254..b181d6860f28 100644
>>> --- a/arch/powerpc/kernel/traps.c
>>> +++ b/arch/powerpc/kernel/traps.c
>>> @@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
>>> }
>>> NOKPROBE_SYMBOL(oops_end);
>>>
>>> +static char *get_mmu_str(void)
>>> +{
>>> + if (early_radix_enabled())
>>> + return " MMU=Radix";
>>> + if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
>>> + return " MMU=Hash";
>>> + return "";
>>> +}
>>
>> We don't change MMU once we're up, so just do this logic once and stash
>> it into a static string, rather than rechecking on every oops.
>
> Do we really have oops so often that we have to worry about that ?
Sometimes :)
But no I don't mean it's a performance issue, it just seems simpler to
compute the value once and store it. In fact for most platforms it can
just be a static string at compile time, it's only 64-bit Book3S that
needs to do anything at runtime.
cheers
^ permalink raw reply
* Re: [PATCH v2] powerpc/book3s/mm: Update Oops message to print the correct translation in use
From: Christophe Leroy @ 2019-07-12 12:25 UTC (permalink / raw)
To: Michael Ellerman, Aneesh Kumar K.V, npiggin, paulus; +Cc: linuxppc-dev
In-Reply-To: <87r26ve93k.fsf@concordia.ellerman.id.au>
Le 12/07/2019 à 14:22, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Le 12/07/2019 à 08:25, Michael Ellerman a écrit :
>>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> ...
>>>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>>>> index 11caa0291254..b181d6860f28 100644
>>>> --- a/arch/powerpc/kernel/traps.c
>>>> +++ b/arch/powerpc/kernel/traps.c
>>>> @@ -250,15 +250,22 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
>>>> }
>>>> NOKPROBE_SYMBOL(oops_end);
>>>>
>>>> +static char *get_mmu_str(void)
>>>> +{
>>>> + if (early_radix_enabled())
>>>> + return " MMU=Radix";
>>>> + if (early_mmu_has_feature(MMU_FTR_HPTE_TABLE))
>>>> + return " MMU=Hash";
>>>> + return "";
>>>> +}
>>>
>>> We don't change MMU once we're up, so just do this logic once and stash
>>> it into a static string, rather than rechecking on every oops.
>>
>> Do we really have oops so often that we have to worry about that ?
>
> Sometimes :)
>
> But no I don't mean it's a performance issue, it just seems simpler to
> compute the value once and store it. In fact for most platforms it can
> just be a static string at compile time, it's only 64-bit Book3S that
> needs to do anything at runtime.
Right, but I'm sure GCC will take care of that since the function is
static and called only once.
Christophe
>
> cheers
>
^ 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