* Re: [PATCH 00/15] powerpc/watchpoint: Preparation for more than one watchpoint
From: Ravi Bangoria @ 2020-03-18 12:52 UTC (permalink / raw)
To: Christophe Leroy
Cc: apopple, mikey, Ravi Bangoria, peterz, oleg, npiggin,
linux-kernel, paulus, jolsa, fweisbec, naveen.n.rao, linuxppc-dev,
mingo
In-Reply-To: <b7148b91-e3db-d48a-7294-5c18fc801933@c-s.fr>
On 3/16/20 8:35 PM, Christophe Leroy wrote:
>
>
> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>> So far, powerpc Book3S code has been written with an assumption of only
>> one watchpoint. But future power architecture is introducing second
>> watchpoint register (DAWR). Even though this patchset does not enable
>> 2nd DAWR, it make the infrastructure ready so that enabling 2nd DAWR
>> should just be a matter of changing count.
>
> Some book3s (e300 family for instance, I think G2 as well) already have a DABR2 in addition to DABR.
> Will this series allow to use it as well ?
I wasn't aware of that. I'll take a look at their specs and check if they
can piggyback on this series for 2nd DABR.
Thanks,
Ravi
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: Check for node_online in node_present_pages
From: Michal Hocko @ 2020-03-18 12:52 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Sachin Sant, Nathan Lynch, Srikar Dronamraju, Bharata B Rao,
linux-mm, Kirill Tkhai, Mel Gorman, Joonsoo Kim, Andrew Morton,
linuxppc-dev, Christopher Lameter
In-Reply-To: <2d7c55ed-0f67-bd47-e478-9726734abcc9@suse.cz>
On Wed 18-03-20 12:53:32, Vlastimil Babka wrote:
[...]
> Yes. So here's an alternative proposal for fixing the current situation in SLUB,
> before the long-term solution of having all possible nodes provide valid pgdat
> with zonelists:
>
> - fix SLUB with the hunk at the end of this mail - the point is to use NUMA_NO_NODE
> as fallback instead of node_to_mem_node()
I am not familiar with SLUB to review.
> - this removes all uses of node_to_mem_node (luckily it's just SLUB),
> kill it completely instead of trying to fix it up
Sounds like a good plan to me. The code shouldn't really care.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v5 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: kajoljain @ 2020-03-18 12:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200317150647.GA757893@krava>
On 3/17/20 8:36 PM, Jiri Olsa wrote:
> On Tue, Mar 17, 2020 at 11:53:31AM +0530, Kajol Jain wrote:
>
> SNIP
>
>> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
>> index 0938ad166ece..7786829b048b 100644
>> --- a/tools/perf/util/expr.h
>> +++ b/tools/perf/util/expr.h
>> @@ -17,12 +17,13 @@ struct expr_parse_ctx {
>>
>> struct expr_scanner_ctx {
>> int start_token;
>> + int expr__runtimeparam;
>
> no need for expr__ prefix in here.. jsut runtime_param
Hi Jiri,
Will take care for this part.
>
>> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
>> index 402af3e8d287..85ac6d913782 100644
>> --- a/tools/perf/util/stat-shadow.c
>> +++ b/tools/perf/util/stat-shadow.c
>> @@ -336,7 +336,7 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list)
>> metric_events = counter->metric_events;
>> if (!metric_events) {
>> if (expr__find_other(counter->metric_expr, counter->name,
>> - &metric_names, &num_metric_names) < 0)
>> + &metric_names, &num_metric_names, 1) < 0)
>> continue;
>>
>> metric_events = calloc(sizeof(struct evsel *),
>> @@ -777,7 +777,15 @@ static void generic_metric(struct perf_stat_config *config,
>> }
>>
>> if (!metric_events[i]) {
>> - if (expr__parse(&ratio, &pctx, metric_expr) == 0) {
>> + int param = 1;
>> + if (strstr(metric_expr, "?")) {
>> + char *tmp = strrchr(metric_name, '_');
>> +
>> + tmp++;
>> + param = strtol(tmp, &tmp, 10);
>> + }
>
> so, if metric_expr contains '?' you go and search metric_name for '_'
> and use the number after '_' ... ugh.. what's the logic?
>
> I understand you create as many metrics as the magic runtime param
> tells you.. but what's the connection to this?
>
> could you please outline in the changelog or comment the whole scheme
> of how this all should work? like step by step on some simple example?
Sure, will add that part as comment in my next patch series.
So, you are right, basically right now I am trying to create as many metric events
as define by runtime_param.
To do that, I add one loop in function `metricgroup__add_metric_runtime_param`
where, we are actually passing this parameter value as part of `expr__find_other` function
and changing "?" present in metric expression with this value.
And then adding this metric event to the tail of group_list.
As in our json file, there gonna be single metric event, and out of which we are creating
multiple events, I am also merging this value to the original metric name to specify parameter value.
For example,
command:# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000
# time counts unit events
1.000101867 9,356,933 hv_24x7/pm_pb_cyc,chip=0/ # 2.3 GHz PowerBUS_Frequency_0
1.000101867 9,366,134 hv_24x7/pm_pb_cyc,chip=1/ # 2.3 GHz PowerBUS_Frequency_1
2.000314878 9,365,868 hv_24x7/pm_pb_cyc,chip=0/ # 2.3 GHz PowerBUS_Frequency_0
2.000314878 9,366,092 hv_24x7/pm_pb_cyc,chip=1/ # 2.3 GHz PowerBUS_Frequency_1
So, here _0 and _1 after PowerBUS_Frequency specify parameter value.
So, after adding this to group_list, again we call expr__parse in generic_metric function present in
util/stat-display.c. By this time again we need to pass this parameter value. So, now to get this value actually
I am trying to extract it from metric name itself. Not sure if its good way to do that.
Is it sound fine, or should I add param as part of structure evsel as well. So, another way to do that
is adding this parameter in `evsel` structure and also egroup structure. So, I will update this value
in metricgroup.c and access it in stat-shadow?
Thanks,
Kajol
>
> thanks,
> jirka
>
^ permalink raw reply
* Re: [PATCH v5 2/2] powerpc/64: Prevent stack protection in early boot
From: Daniel Axtens @ 2020-03-18 12:42 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
In-Reply-To: <20200316124421.99211-2-mpe@ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> The previous commit reduced the amount of code that is run before we
> setup a paca. However there are still a few remaining functions that
> run with no paca, or worse, with an arbitrary value in r13 that will
> be used as a paca pointer.
>
> In particular the stack protector canary is stored in the paca, so if
> stack protector is activated for any of these functions we will read
> the stack canary from wherever r13 points. If r13 happens to point
> outside of memory we will get a machine check / checkstop.
>
> For example if we modify initialise_paca() to trigger stack
> protection, and then boot in the mambo simulator with r13 poisoned in
> skiboot before calling the kernel:
>
> DEBUG: 19952232: (19952232): INSTRUCTION: PC=0xC0000000191FC1E8: [0x3C4C006D]: addis r2,r12,0x6D [fetch]
> DEBUG: 19952236: (19952236): INSTRUCTION: PC=0xC00000001807EAD8: [0x7D8802A6]: mflr r12 [fetch]
> FATAL ERROR: 19952276: (19952276): Check Stop for 0:0: Machine Check with ME bit of MSR off
> DEBUG: 19952276: (19952276): INSTRUCTION: PC=0xC0000000191FCA7C: [0xE90D0CF8]: ld r8,0xCF8(r13) [Instruction Failed]
> INFO: 19952276: (19952277): ** Execution stopped: Mambo Error, Machine Check Stop, **
> systemsim % bt
> pc: 0xC0000000191FCA7C initialise_paca+0x54
> lr: 0xC0000000191FC22C early_setup+0x44
> stack:0x00000000198CBED0 0x0 +0x0
> stack:0x00000000198CBF00 0xC0000000191FC22C early_setup+0x44
> stack:0x00000000198CBF90 0x1801C968 +0x1801C968
>
> So annotate the relevant functions to ensure stack protection is never
> enabled for them.
This all makes sense to me, although I don't really understand the stack
protector especially well.
I have checked and I can find no other C functions that are called
before early_setup.
Do we need to do add setup_64.c to the part of the Makefile that
disables tracing of early boot?
ifdef CONFIG_FUNCTION_TRACER
# Do not trace early boot code
CFLAGS_REMOVE_cputable.o = $(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_prom_init.o = $(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_btext.o = $(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_prom.o = $(CC_FLAGS_FTRACE)
-> should we add setup_64.c here?
endif
Regards,
Daniel
>
> Fixes: 06ec27aea9fc ("powerpc/64: add stack protector support")
> Cc: stable@vger.kernel.org # v4.20+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kernel/paca.c | 4 ++--
> arch/powerpc/kernel/setup.h | 2 ++
> arch/powerpc/kernel/setup_64.c | 2 +-
> 3 files changed, 5 insertions(+), 3 deletions(-)
>
> v5: New.
>
> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> index 0ee6308541b1..3f91ccaa9c74 100644
> --- a/arch/powerpc/kernel/paca.c
> +++ b/arch/powerpc/kernel/paca.c
> @@ -176,7 +176,7 @@ static struct slb_shadow * __init new_slb_shadow(int cpu, unsigned long limit)
> struct paca_struct **paca_ptrs __read_mostly;
> EXPORT_SYMBOL(paca_ptrs);
>
> -void __init initialise_paca(struct paca_struct *new_paca, int cpu)
> +void __init __nostackprotector initialise_paca(struct paca_struct *new_paca, int cpu)
> {
> #ifdef CONFIG_PPC_PSERIES
> new_paca->lppaca_ptr = NULL;
> @@ -205,7 +205,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
> }
>
> /* Put the paca pointer into r13 and SPRG_PACA */
> -void setup_paca(struct paca_struct *new_paca)
> +void __nostackprotector setup_paca(struct paca_struct *new_paca)
> {
> /* Setup r13 */
> local_paca = new_paca;
> diff --git a/arch/powerpc/kernel/setup.h b/arch/powerpc/kernel/setup.h
> index 2dd0d9cb5a20..d210671026e9 100644
> --- a/arch/powerpc/kernel/setup.h
> +++ b/arch/powerpc/kernel/setup.h
> @@ -8,6 +8,8 @@
> #ifndef __ARCH_POWERPC_KERNEL_SETUP_H
> #define __ARCH_POWERPC_KERNEL_SETUP_H
>
> +#define __nostackprotector __attribute__((__optimize__("no-stack-protector")))
> +
> void initialize_cache_info(void);
> void irqstack_early_init(void);
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 17886d147dd0..438a9befce41 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -279,7 +279,7 @@ void __init record_spr_defaults(void)
> * device-tree is not accessible via normal means at this point.
> */
>
> -void __init early_setup(unsigned long dt_ptr)
> +void __init __nostackprotector early_setup(unsigned long dt_ptr)
> {
> static __initdata struct paca_struct boot_paca;
>
> --
> 2.24.1
^ permalink raw reply
* Re: [PATCH 01/15] powerpc/watchpoint: Rename current DAWR macros
From: Ravi Bangoria @ 2020-03-18 12:40 UTC (permalink / raw)
To: Christophe Leroy
Cc: apopple, mikey, Ravi Bangoria, peterz, oleg, npiggin,
linux-kernel, paulus, jolsa, fweisbec, naveen.n.rao, linuxppc-dev,
mingo
In-Reply-To: <6ae1865c-fcf6-5bef-2d93-e75b03ed8b44@c-s.fr>
On 3/17/20 3:44 PM, Christophe Leroy wrote:
>
>
> Le 09/03/2020 à 09:57, Ravi Bangoria a écrit :
>> Future Power architecture is introducing second DAWR. Rename current
>> DAWR macros as:
>> s/SPRN_DAWR/SPRN_DAWR0/
>> s/SPRN_DAWRX/SPRN_DAWRX0/
>
> I think you should tell that DAWR0 and DAWRX0 is the real name of the register as documented in (at least) power8 and power9 user manual.
>
> Otherwise, we can't understand why you change the name of the register.
Ok.
Thanks,
Ravi
^ permalink raw reply
* Re: [PATCH 15/15] powerpc/watchpoint/xmon: Support 2nd dawr
From: Ravi Bangoria @ 2020-03-18 12:39 UTC (permalink / raw)
To: Christophe Leroy
Cc: apopple, mikey, Ravi Bangoria, peterz, oleg, npiggin,
linux-kernel, paulus, jolsa, fweisbec, naveen.n.rao, linuxppc-dev,
mingo
In-Reply-To: <dabb2823-783a-6a3f-4f04-f3200a1086fc@c-s.fr>
>> +static int free_data_bpt(void)
>
> This names suggests the function frees a breakpoint.
> I guess it should be find_free_data_bpt()
Sure.
Thanks,
Ravi
^ permalink raw reply
* Re: [PATCH 14/15] powerpc/watchpoint/xmon: Don't allow breakpoint overwriting
From: Ravi Bangoria @ 2020-03-18 12:37 UTC (permalink / raw)
To: Christophe Leroy
Cc: apopple, mikey, Ravi Bangoria, peterz, oleg, npiggin,
linux-kernel, paulus, jolsa, fweisbec, naveen.n.rao, linuxppc-dev,
mingo
In-Reply-To: <b6892b22-c521-207e-e5fd-ca66c774b314@c-s.fr>
On 3/17/20 4:40 PM, Christophe Leroy wrote:
>
>
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> Xmon allows overwriting breakpoints because it's supported by only
>> one dawr. But with multiple dawrs, overwriting becomes ambiguous
>> or unnecessary complicated. So let's not allow it.
>
> Could we drop this completely (I mean the functionnality, not the patch).
Not sure I follow. Isn't the same thing I'm doing?
-Ravi
^ permalink raw reply
* Re: [PATCH 13/15] powerpc/watchpoint: Don't allow concurrent perf and ptrace events
From: Ravi Bangoria @ 2020-03-18 12:35 UTC (permalink / raw)
To: Christophe Leroy
Cc: apopple, mikey, Ravi Bangoria, peterz, oleg, npiggin,
linux-kernel, paulus, jolsa, fweisbec, naveen.n.rao, linuxppc-dev,
mingo
In-Reply-To: <673cf087-1422-84b3-e3bc-13c4dd491414@c-s.fr>
On 3/17/20 4:38 PM, Christophe Leroy wrote:
>
>
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> ptrace and perf watchpoints on powerpc behaves differently. Ptrace
>
> On the 8xx, ptrace generates signal after executing the instruction.
8xx logic is unchanged. I should have mentioned "Book3s DAWR".
>
>> watchpoint works in one-shot mode and generates signal before executing
>> instruction. It's ptrace user's job to single-step the instruction and
>> re-enable the watchpoint. OTOH, in case of perf watchpoint, kernel
>> emulates/single-steps the instruction and then generates event. If perf
>> and ptrace creates two events with same or overlapping address ranges,
>> it's ambiguous to decide who should single-step the instruction. Because
>> of this issue ptrace and perf event can't coexist when the address range
>> overlaps.
>
> Ok, and then ? What's the purpose of this (big) patch ?
Don't allow perf and ptrace watchpoint at the same time if their address
range overlaps.
...
>> +struct breakpoint {
>> + struct list_head list;
>> + struct perf_event *bp;
>> + bool ptrace_bp;
>> +};
>
> Don't we have an equivalent struct already ?
No. Using this we track percpu and perthread watchpoints for both perf and
ptrace. This problems is powerpc(DAWR) specific and thus we need to hook arch
specific logic in watchopint installation/uninstallation path.
...
>> +static bool bp_addr_range_overlap(struct perf_event *bp1, struct perf_event *bp2)
>> +{
>> + __u64 bp1_saddr, bp1_eaddr, bp2_saddr, bp2_eaddr;
>> +
>> + bp1_saddr = bp1->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
>> + bp1_eaddr = (bp1->attr.bp_addr + bp1->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
>> + bp2_saddr = bp2->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
>> + bp2_eaddr = (bp2->attr.bp_addr + bp2->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
>> +
>> + return (bp1_saddr <= bp2_eaddr && bp1_eaddr >= bp2_saddr);
>
> Would be better with something like (HW_BREAKPOINT_SIZE needs to be defined).
>
> bp1_saddr = ALIGN_DOWN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
> bp1_eaddr = ALIGN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
> bp2_saddr = ALIGN_DOWN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);
> bp2_eaddr = ALIGN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);
>
> return (bp1_saddr < bp2_eaddr && bp1_eaddr > bp2_saddr);
Ok.
Thanks,
Ravi
^ permalink raw reply
* Re: [PATCH v5 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: kajoljain @ 2020-03-18 12:18 UTC (permalink / raw)
To: Jiri Olsa
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200317151047.GF757893@krava>
On 3/17/20 8:40 PM, Jiri Olsa wrote:
> On Tue, Mar 17, 2020 at 11:53:31AM +0530, Kajol Jain wrote:
>
> SBIP
>
>> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
>> + struct list_head *group_list, struct pmu_event *pe)
>> +{
>> + int i, count;
>> + int ret = -EINVAL;
>> +
>> + count = arch_get_runtimeparam();
>> +
>> + /* This loop is added to create multiple
>> + * events depend on count value and add
>> + * those events to group_list.
>> + */
>> +
>> + for (i = 0; i < count; i++) {
>> + const char **ids;
>> + int idnum;
>> + struct egroup *eg;
>> + char value[PATH_MAX];
>> +
>> + if (expr__find_other(pe->metric_expr, NULL, &ids, &idnum, i) < 0)
>> + return ret;
>> +
>> + if (events->len > 0)
>> + strbuf_addf(events, ",");
>> +
>> + if (metricgroup__has_constraint(pe))
>> + metricgroup__add_metric_non_group(events, ids, idnum);
>> + else
>> + metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> + eg = malloc(sizeof(*eg));
>> + if (!eg) {
>> + ret = -ENOMEM;
>> + return ret;
>> + }
>> +
>> + sprintf(value, "%s%c%d", pe->metric_name, '_', i);
>> + eg->ids = ids;
>> + eg->idnum = idnum;
>> + eg->metric_name = strdup(value);
>> + if (!eg->metric_name) {
>> + ret = -ENOMEM;
>> + return ret;
>> + }
>> +
>> + eg->metric_expr = pe->metric_expr;
>> + eg->metric_unit = pe->unit;
>> + list_add_tail(&eg->nd, group_list);
>> + ret = 0;
>> +
>> + if (ret != 0)
>> + break;
>
> again, this is part of metricgroup__add_metric_param no? why not use it?
Yes right, thanks for pointing it out. Will update in next version.
Thanks,
Kajol
>
> jirka
>
^ permalink raw reply
* Re: [PATCH v5 08/11] perf/tools: Refactoring metricgroup__add_metric function
From: kajoljain @ 2020-03-18 12:17 UTC (permalink / raw)
To: Jiri Olsa
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200317151030.GE757893@krava>
On 3/17/20 8:40 PM, Jiri Olsa wrote:
> On Tue, Mar 17, 2020 at 11:53:30AM +0530, Kajol Jain wrote:
>> This patch refactor metricgroup__add_metric function where
>> some part of it move to function metricgroup__add_metric_param.
>> No logic change.
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>> tools/perf/util/metricgroup.c | 63 +++++++++++++++++++++--------------
>> 1 file changed, 38 insertions(+), 25 deletions(-)
>>
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index c3a8c701609a..b4919bcfbd8b 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -474,6 +474,41 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
>> return false;
>> }
>>
>> +static int metricgroup__add_metric_param(struct strbuf *events,
>> + struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +
>> + const char **ids;
>> + int idnum;
>> + struct egroup *eg;
>> + int ret = -EINVAL;
>> +
>> + if (expr__find_other(pe->metric_expr, NULL, &ids, &idnum, 1) < 0)
>> + return ret;
>> +
>> + if (events->len > 0)
>> + strbuf_addf(events, ",");
>> +
>> + if (metricgroup__has_constraint(pe))
>> + metricgroup__add_metric_non_group(events, ids, idnum);
>> + else
>> + metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> + eg = malloc(sizeof(*eg));
>> + if (!eg)
>> + ret = -ENOMEM;
>
> ??? you need to return in here, eg is NULL
Yes right, miss this part. Will correct it.
>
>> +
>> + eg->ids = ids;
>> + eg->idnum = idnum;
>> + eg->metric_name = pe->metric_name;
>> + eg->metric_expr = pe->metric_expr;
>> + eg->metric_unit = pe->unit;
>> + list_add_tail(&eg->nd, group_list);
>> + ret = 0;
>> +
>> + return ret;
>> +}
>> +
>> static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>> struct list_head *group_list)
>> {
>> @@ -493,35 +528,13 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>> continue;
>> if (match_metric(pe->metric_group, metric) ||
>> match_metric(pe->metric_name, metric)) {
>> - const char **ids;
>> - int idnum;
>> - struct egroup *eg;
>>
>> pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>>
>> - if (expr__find_other(pe->metric_expr,
>> - NULL, &ids, &idnum) < 0)
>> + ret = metricgroup__add_metric_param(events,
>> + group_list, pe);
>> + if (ret == -EINVAL)
>> continue;
>
> previous code did 'continue' on ret < 0, why just -EINVAL now?
Actually incase we have memory issue then we are not
continuing that's why I added check for -EINVAL explicitly. Because for other cases
I am returning -EINVAL itself. But Yes I miss one part, where
even after matched metric we are continuing incase ret = 0. Will update that part.
Please let me know if it sounds fine.
Thanks,
Kajol
>
> jirka
>
^ permalink raw reply
* Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
From: Ravi Bangoria @ 2020-03-18 12:14 UTC (permalink / raw)
To: Christophe Leroy
Cc: apopple, mikey, Ravi Bangoria, peterz, oleg, npiggin,
linux-kernel, paulus, jolsa, fweisbec, naveen.n.rao, linuxppc-dev,
mingo
In-Reply-To: <3ba94856-0d87-5046-eca9-b5c3d99ec654@c-s.fr>
On 3/17/20 4:29 PM, Christophe Leroy wrote:
>
>
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> Currently we assume that we have only one watchpoint supported by hw.
>> Get rid of that assumption and use dynamic loop instead. This should
>> make supporting more watchpoints very easy.
>
> I think using 'we' is to be avoided in commit message.
>
> Could be something like:
>
> "Currently the handler assumes there is only one watchpoint supported by hw"
>
>>
>> So far, with only one watchpoint, the handler was simple. But with
>> multiple watchpoints, we need a mechanism to detect which watchpoint
>> caused the exception. HW doesn't provide that information and thus
>> we need software logic for this. This makes exception handling bit
>> more complex.
>
> Same here, the 'we' should be avoided.
>
>
> This patch is pretty big. I think you should explain a bit more what is done. Otherwise it is difficult to review.
I understand, and the diff is also messy. But splitting this into granular
patches looks difficult to me.
The patch enables core hw-breakpoint subsystem to install/uninstall more than one
watchpoint, basically converting direct accesses of data structures in a loop.
Similarly changes in thread_chage_pc(), single_step_dabr_instruction() etc.
Now, with more than one watchpoints, exception handler need to know which DAWR
caused the exception, and hw currently does not provide it. So we need sw logic
for the same. To figure out which DAWR caused the exception, we check all
different combinations of user specified range, dawr address range, actual
access range and dawrx constrains. For ex, if user specified range and actual
access range overlaps but dawrx is configured for readonly watchpoint and the
instruction is store, this DAWR must not have caused exception. This logic is
implemented by check_constraints() function.
...
>> -static bool dar_within_range(unsigned long dar, struct arch_hw_breakpoint *info)
>> +static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
>
> Is this name change directly related to the patch ?
Yes. now we have two functions: dar_in_user_range() and dar_in_hw_range().
More detail below...
>
>> {
>> return ((info->address <= dar) && (dar - info->address < info->len));
>> }
>> -static bool
>> -dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
>> +static bool dar_user_range_overlaps(unsigned long dar, int size,
>> + struct arch_hw_breakpoint *info)
>
> Same question.
For single DAWR, hw_breakpoint_handler() logic is:
if (user range overlaps with actual access range)
valid event;
else
valid but extraneous;
With multiple breakpoints, this logic can't work when hw is not telling us which
DAWR caused exception. So the logic is like:
for (i = 0; i < nr_wps; i++) {
if (user range overlaps with actual access range)
valid event;
else if (hw range overlaps with actual access range)
valid but extraneous;
else
exception cause by some other dawr;
}
So we have two functions: dar_user_range_overlaps() and dar_hw_range_overlaps().
If reading instruction fails, we can't get actual access range an in that case
we use: dar_in_user_range() and dar_in_hw_range().
Thanks,
Ravi
^ permalink raw reply
* [PATCH v3] powerpc/perf: Use SIER_USER_MASK while updating SPRN_SIER for EBB events
From: Athira Rajeev @ 2020-03-18 12:06 UTC (permalink / raw)
To: mpe; +Cc: maddy, linuxppc-dev
Sampled Instruction Event Register (SIER), is a PMU register,
captures architecture state for a given sample. And sier_user_mask
defined in commit 330a1eb7775b ("powerpc/perf: Core EBB support for 64-bit
book3s") defines the architected bits that needs to be saved from the SPR.
Currently all of the bits from SIER are saved for EBB events. Patch fixes
this by ANDing the "sier_user_mask" to data from SIER in ebb_switch_out().
This will force save only architected bits from the SIER.
Fixes: 330a1eb7775b ("powerpc/perf: Core EBB support for 64-bit book3s")
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
Changelog:
v2 -> v3:
- Corrected name of SIER register in commit message
as pointed by Segher Boessenkool
v1 -> v2:
- Make the commit message more clearer.
arch/powerpc/perf/core-book3s.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 3086055..48b61cc 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -579,7 +579,7 @@ static void ebb_switch_out(unsigned long mmcr0)
return;
current->thread.siar = mfspr(SPRN_SIAR);
- current->thread.sier = mfspr(SPRN_SIER);
+ current->thread.sier = mfspr(SPRN_SIER) & SIER_USER_MASK;
current->thread.sdar = mfspr(SPRN_SDAR);
current->thread.mmcr0 = mmcr0 & MMCR0_USER_MASK;
current->thread.mmcr2 = mfspr(SPRN_MMCR2) & MMCR2_USER_MASK;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v2] powerpc/perf: Use SIER_USER_MASK while updating SPRN_SIER for EBB events
From: Athira Rajeev @ 2020-03-18 12:00 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: maddy, linuxppc-dev
In-Reply-To: <20200313180642.GV22482@gate.crashing.org>
> On 13-Mar-2020, at 11:36 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
>
> On Fri, Mar 13, 2020 at 01:49:07PM -0400, Athira Rajeev wrote:
>> Sampled instruction address register (SIER), is a PMU register,
>
> SIER stands for "Sampled Instruction Event Register", instead. With that
> change, your patch is totally clear :-)
>
Hi Segher,
Thanks for reviewing. My bad. I am sending a V3 with the register name corrected in the commit message.
Athira
>
> Segher
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: Check for node_online in node_present_pages
From: Vlastimil Babka @ 2020-03-18 11:53 UTC (permalink / raw)
To: Michal Hocko, Srikar Dronamraju
Cc: Sachin Sant, Nathan Lynch, Bharata B Rao, linux-mm, Kirill Tkhai,
Mel Gorman, Joonsoo Kim, Andrew Morton, linuxppc-dev,
Christopher Lameter
In-Reply-To: <20200318100256.GH21362@dhcp22.suse.cz>
On 3/18/20 11:02 AM, Michal Hocko wrote:
> On Wed 18-03-20 12:58:07, Srikar Dronamraju wrote:
>> Calling a kmalloc_node on a possible node which is not yet onlined can
>> lead to panic. Currently node_present_pages() doesn't verify the node is
>> online before accessing the pgdat for the node. However pgdat struct may
>> not be available resulting in a crash.
>>
>> NIP [c0000000003d55f4] ___slab_alloc+0x1f4/0x760
>> LR [c0000000003d5b94] __slab_alloc+0x34/0x60
>> Call Trace:
>> [c0000008b3783960] [c0000000003d5734] ___slab_alloc+0x334/0x760 (unreliable)
>> [c0000008b3783a40] [c0000000003d5b94] __slab_alloc+0x34/0x60
>> [c0000008b3783a70] [c0000000003d6fa0] __kmalloc_node+0x110/0x490
>> [c0000008b3783af0] [c0000000003443d8] kvmalloc_node+0x58/0x110
>> [c0000008b3783b30] [c0000000003fee38] mem_cgroup_css_online+0x108/0x270
>> [c0000008b3783b90] [c000000000235aa8] online_css+0x48/0xd0
>> [c0000008b3783bc0] [c00000000023eaec] cgroup_apply_control_enable+0x2ec/0x4d0
>> [c0000008b3783ca0] [c000000000242318] cgroup_mkdir+0x228/0x5f0
>> [c0000008b3783d10] [c00000000051e170] kernfs_iop_mkdir+0x90/0xf0
>> [c0000008b3783d50] [c00000000043dc00] vfs_mkdir+0x110/0x230
>> [c0000008b3783da0] [c000000000441c90] do_mkdirat+0xb0/0x1a0
>> [c0000008b3783e20] [c00000000000b278] system_call+0x5c/0x68
>>
>> Fix this by verifying the node is online before accessing the pgdat
>> structure. Fix the same for node_spanned_pages() too.
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-mm@kvack.org
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Sachin Sant <sachinp@linux.vnet.ibm.com>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Christopher Lameter <cl@linux.com>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Cc: Bharata B Rao <bharata@linux.ibm.com>
>> Cc: Nathan Lynch <nathanl@linux.ibm.com>
>>
>> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
>> Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
>> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> ---
>> include/linux/mmzone.h | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>> index f3f264826423..88078a3b95e5 100644
>> --- a/include/linux/mmzone.h
>> +++ b/include/linux/mmzone.h
>> @@ -756,8 +756,10 @@ typedef struct pglist_data {
>> atomic_long_t vm_stat[NR_VM_NODE_STAT_ITEMS];
>> } pg_data_t;
>>
>> -#define node_present_pages(nid) (NODE_DATA(nid)->node_present_pages)
>> -#define node_spanned_pages(nid) (NODE_DATA(nid)->node_spanned_pages)
>> +#define node_present_pages(nid) \
>> + (node_online(nid) ? NODE_DATA(nid)->node_present_pages : 0)
>> +#define node_spanned_pages(nid) \
>> + (node_online(nid) ? NODE_DATA(nid)->node_spanned_pages : 0)
>
> I believe this is a wrong approach. We really do not want to special
> case all the places which require NODE_DATA. Can we please go and
> allocate pgdat for all possible nodes?
>
> The current state of memory less hacks subtle bugs poping up here and
> there just prove that we should have done that from the very begining
> IMHO.
Yes. So here's an alternative proposal for fixing the current situation in SLUB,
before the long-term solution of having all possible nodes provide valid pgdat
with zonelists:
- fix SLUB with the hunk at the end of this mail - the point is to use NUMA_NO_NODE
as fallback instead of node_to_mem_node()
- this removes all uses of node_to_mem_node (luckily it's just SLUB),
kill it completely instead of trying to fix it up
- patch 1/4 is not needed with the fix
- perhaps many of your other patches are alss not needed
- once we get the long-term solution, some of the !node_online() checks can be removed
----8<----
diff --git a/mm/slub.c b/mm/slub.c
index 17dc00e33115..1d4f2d7a0080 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1511,7 +1511,7 @@ static inline struct page *alloc_slab_page(struct kmem_cache *s,
struct page *page;
unsigned int order = oo_order(oo);
- if (node == NUMA_NO_NODE)
+ if (node == NUMA_NO_NODE || !node_online(node))
page = alloc_pages(flags, order);
else
page = __alloc_pages_node(node, flags, order);
@@ -1973,8 +1973,6 @@ static void *get_partial(struct kmem_cache *s, gfp_t flags, int node,
if (node == NUMA_NO_NODE)
searchnode = numa_mem_id();
- else if (!node_present_pages(node))
- searchnode = node_to_mem_node(node);
object = get_partial_node(s, get_node(s, searchnode), c, flags);
if (object || node != NUMA_NO_NODE)
@@ -2568,12 +2566,15 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
redo:
if (unlikely(!node_match(page, node))) {
- int searchnode = node;
-
- if (node != NUMA_NO_NODE && !node_present_pages(node))
- searchnode = node_to_mem_node(node);
-
- if (unlikely(!node_match(page, searchnode))) {
+ /*
+ * node_match() false implies node != NUMA_NO_NODE
+ * but if the node is not online and has no pages, just
+ * ignore the constraint
+ */
+ if ((!node_online(node) || !node_present_pages(node))) {
+ node = NUMA_NO_NODE;
+ goto redo;
+ } else {
stat(s, ALLOC_NODE_MISMATCH);
deactivate_slab(s, page, c->freelist, c);
goto new_slab;
^ permalink raw reply related
* Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
From: Christophe Leroy @ 2020-03-18 11:44 UTC (permalink / raw)
To: Michael Ellerman, Ravi Bangoria, mikey
Cc: apopple, peterz, fweisbec, oleg, npiggin, linux-kernel, paulus,
jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <87zhcdevz7.fsf@mpe.ellerman.id.au>
Le 18/03/2020 à 12:35, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>>> Currently we assume that we have only one watchpoint supported by hw.
>>> Get rid of that assumption and use dynamic loop instead. This should
>>> make supporting more watchpoints very easy.
>>
>> I think using 'we' is to be avoided in commit message.
>
> Hmm, is it?
>
> I use 'we' all the time. Which doesn't mean it's correct, but I think it
> reads OK.
>
> cheers
>
From
https://www.kernel.org/doc/html/latest/process/submitting-patches.html :
Describe your changes in imperative mood, e.g. “make xyzzy do frotz”
instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to
do frotz”, as if you are giving orders to the codebase to change its
behaviour.
Christophe
^ permalink raw reply
* Re: [PATCH v5 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: kajoljain @ 2020-03-18 11:44 UTC (permalink / raw)
To: Jiri Olsa
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200317150703.GB757893@krava>
On 3/17/20 8:37 PM, Jiri Olsa wrote:
> On Tue, Mar 17, 2020 at 11:53:31AM +0530, Kajol Jain wrote:
>
> SNIP
>
>> diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
>> index 3b4cdfc5efd6..dcc3c6ab2e67 100644
>> --- a/tools/perf/arch/powerpc/util/header.c
>> +++ b/tools/perf/arch/powerpc/util/header.c
>> @@ -7,6 +7,8 @@
>> #include <string.h>
>> #include <linux/stringify.h>
>> #include "header.h"
>> +#include "metricgroup.h"
>> +#include <api/fs/fs.h>
>>
>> #define mfspr(rn) ({unsigned long rval; \
>> asm volatile("mfspr %0," __stringify(rn) \
>> @@ -16,6 +18,8 @@
>> #define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */
>> #define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* Revison field */
>>
>> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/sockets"
>> +
>> int
>> get_cpuid(char *buffer, size_t sz)
>> {
>> @@ -44,3 +48,9 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>>
>> return bufp;
>> }
>> +
>> +int arch_get_runtimeparam(void)
>> +{
>> + int count;
>> + return sysfs__read_int(SOCKETS_INFO_FILE_PATH, &count) < 0 ? 1 : count;
>
> is that SOCKETS_INFO_FILE_PATH define used later? if not please
> put the path directly as an argument to sysfs__read_int
>
Ok will do that.
Thanks,
Kajol
> jirka
>
^ permalink raw reply
* Re: [PATCH v5 08/11] perf/tools: Refactoring metricgroup__add_metric function
From: kajoljain @ 2020-03-18 11:41 UTC (permalink / raw)
To: Jiri Olsa
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200317150945.GC757893@krava>
On 3/17/20 8:39 PM, Jiri Olsa wrote:
> On Tue, Mar 17, 2020 at 11:53:30AM +0530, Kajol Jain wrote:
>> This patch refactor metricgroup__add_metric function where
>> some part of it move to function metricgroup__add_metric_param.
>> No logic change.
>
> can't compile this change:
>
> [jolsa@krava perf]$ make JOBS=1
> BUILD: Doing 'make -j1' parallel build
> CC util/metricgroup.o
> util/metricgroup.c: In function ‘metricgroup__add_metric_param’:
> util/metricgroup.c:486:6: error: too many arguments to function ‘expr__find_other’
> 486 | if (expr__find_other(pe->metric_expr, NULL, &ids, &idnum, 1) < 0)
> | ^~~~~~~~~~~~~~~~
> In file included from util/metricgroup.c:14:
> util/expr.h:25:5: note: declared here
> 25 | int expr__find_other(const char *expr, const char *one, const char ***other,
> | ^~~~~~~~~~~~~~~~
> mv: cannot stat 'util/.metricgroup.o.tmp': No such file or directory
> make[4]: *** [/home/jolsa/kernel/linux-perf/tools/build/Makefile.build:97: util/metricgroup.o] Error 1
> make[3]: *** [/home/jolsa/kernel/linux-perf/tools/build/Makefile.build:139: util] Error 2
> make[2]: *** [Makefile.perf:616: perf-in.o] Error 2
> make[1]: *** [Makefile.perf:225: sub-make] Error 2
> make: *** [Makefile:70: all] Error 2
Hi Jiri,
I made a mistake while arranging the patches in the series. I'll re-arrange it
and send it again.
Thanks,
Kajol
>
> jirka
>
>>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>> tools/perf/util/metricgroup.c | 63 +++++++++++++++++++++--------------
>> 1 file changed, 38 insertions(+), 25 deletions(-)
>>
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index c3a8c701609a..b4919bcfbd8b 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -474,6 +474,41 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
>> return false;
>> }
>>
>> +static int metricgroup__add_metric_param(struct strbuf *events,
>> + struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +
>> + const char **ids;
>> + int idnum;
>> + struct egroup *eg;
>> + int ret = -EINVAL;
>> +
>> + if (expr__find_other(pe->metric_expr, NULL, &ids, &idnum, 1) < 0)
>> + return ret;
>> +
>> + if (events->len > 0)
>> + strbuf_addf(events, ",");
>> +
>> + if (metricgroup__has_constraint(pe))
>> + metricgroup__add_metric_non_group(events, ids, idnum);
>> + else
>> + metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> + eg = malloc(sizeof(*eg));
>> + if (!eg)
>> + ret = -ENOMEM;
>> +
>> + eg->ids = ids;
>> + eg->idnum = idnum;
>> + eg->metric_name = pe->metric_name;
>> + eg->metric_expr = pe->metric_expr;
>> + eg->metric_unit = pe->unit;
>> + list_add_tail(&eg->nd, group_list);
>> + ret = 0;
>> +
>> + return ret;
>> +}
>> +
>> static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>> struct list_head *group_list)
>> {
>> @@ -493,35 +528,13 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>> continue;
>> if (match_metric(pe->metric_group, metric) ||
>> match_metric(pe->metric_name, metric)) {
>> - const char **ids;
>> - int idnum;
>> - struct egroup *eg;
>>
>> pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>>
>> - if (expr__find_other(pe->metric_expr,
>> - NULL, &ids, &idnum) < 0)
>> + ret = metricgroup__add_metric_param(events,
>> + group_list, pe);
>> + if (ret == -EINVAL)
>> continue;
>> - if (events->len > 0)
>> - strbuf_addf(events, ",");
>> -
>> - if (metricgroup__has_constraint(pe))
>> - metricgroup__add_metric_non_group(events, ids, idnum);
>> - else
>> - metricgroup__add_metric_weak_group(events, ids, idnum);
>> -
>> - eg = malloc(sizeof(struct egroup));
>> - if (!eg) {
>> - ret = -ENOMEM;
>> - break;
>> - }
>> - eg->ids = ids;
>> - eg->idnum = idnum;
>> - eg->metric_name = pe->metric_name;
>> - eg->metric_expr = pe->metric_expr;
>> - eg->metric_unit = pe->unit;
>> - list_add_tail(&eg->nd, group_list);
>> - ret = 0;
>> }
>> }
>> return ret;
>> --
>> 2.18.1
>>
>
^ permalink raw reply
* Re: [PATCH 12/15] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
From: Michael Ellerman @ 2020-03-18 11:35 UTC (permalink / raw)
To: Christophe Leroy, Ravi Bangoria, mikey
Cc: apopple, peterz, fweisbec, oleg, npiggin, linux-kernel, paulus,
jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <3ba94856-0d87-5046-eca9-b5c3d99ec654@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 09/03/2020 à 09:58, Ravi Bangoria a écrit :
>> Currently we assume that we have only one watchpoint supported by hw.
>> Get rid of that assumption and use dynamic loop instead. This should
>> make supporting more watchpoints very easy.
>
> I think using 'we' is to be avoided in commit message.
Hmm, is it?
I use 'we' all the time. Which doesn't mean it's correct, but I think it
reads OK.
cheers
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: Check for node_online in node_present_pages
From: Michal Hocko @ 2020-03-18 11:14 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Sachin Sant, Nathan Lynch, Bharata B Rao, linux-mm, Kirill Tkhai,
Mel Gorman, Joonsoo Kim, Andrew Morton, linuxppc-dev,
Christopher Lameter, Vlastimil Babka
In-Reply-To: <20200318110215.GC27520@linux.vnet.ibm.com>
On Wed 18-03-20 16:32:15, Srikar Dronamraju wrote:
> * Michal Hocko <mhocko@suse.com> [2020-03-18 11:02:56]:
>
> > On Wed 18-03-20 12:58:07, Srikar Dronamraju wrote:
[...]
> > > -#define node_present_pages(nid) (NODE_DATA(nid)->node_present_pages)
> > > -#define node_spanned_pages(nid) (NODE_DATA(nid)->node_spanned_pages)
> > > +#define node_present_pages(nid) \
> > > + (node_online(nid) ? NODE_DATA(nid)->node_present_pages : 0)
> > > +#define node_spanned_pages(nid) \
> > > + (node_online(nid) ? NODE_DATA(nid)->node_spanned_pages : 0)
> >
> > I believe this is a wrong approach. We really do not want to special
> > case all the places which require NODE_DATA. Can we please go and
> > allocate pgdat for all possible nodes?
> >
>
> I can do that but the question I had was should we make this change just for
> Powerpc or should the change be for other archs.
No, we shouldn't, really. If NODE_DATA is non-null for all possible
nodes then this shouldn't be really necessary and arch specific.
> NODE_DATA initialization always seems to be in arch specific code.
>
> The other archs that are affected seem to be mips, sh and sparc
> These archs seem to have making an assumption that NODE_DATA has to be local
> only,
Which is all good and fine for nodes that hold some memory. If those
architectures support memory less nodes at all then I do not see any
problem to have remote pgdata.
> For example on sparc / arch/sparc/mm/init_64.c in allocate_node_data function.
>
> NODE_DATA(nid) = memblock_alloc_node(sizeof(struct pglist_data),
> SMP_CACHE_BYTES, nid);
> if (!NODE_DATA(nid)) {
> prom_printf("Cannot allocate pglist_data for nid[%d]\n", nid);
> prom_halt();
> }
>
> NODE_DATA(nid)->node_id = nid;
This code is not about memroy less nodes, is it? It looks more like a
allocation failure panic-like handling because there is not enough
memory to hold pgdat. This also strongly suggests that this platform
doesn't really expect memory less nodes in the early init path.
> So even if I make changes to allocate NODE_DATA from fallback node, I may not
> be able to test them.
Please try to focus on the architecture you can test for. From the
existing reports I have seen this looks mostly to be a problem for x86
and ppc
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: Check for node_online in node_present_pages
From: Srikar Dronamraju @ 2020-03-18 11:02 UTC (permalink / raw)
To: Michal Hocko
Cc: Sachin Sant, Nathan Lynch, Bharata B Rao, linux-mm, Kirill Tkhai,
Mel Gorman, Joonsoo Kim, Andrew Morton, linuxppc-dev,
Christopher Lameter, Vlastimil Babka
In-Reply-To: <20200318100256.GH21362@dhcp22.suse.cz>
* Michal Hocko <mhocko@suse.com> [2020-03-18 11:02:56]:
> On Wed 18-03-20 12:58:07, Srikar Dronamraju wrote:
> > Calling a kmalloc_node on a possible node which is not yet onlined can
> > lead to panic. Currently node_present_pages() doesn't verify the node is
> > online before accessing the pgdat for the node. However pgdat struct may
> > not be available resulting in a crash.
> >
> > NIP [c0000000003d55f4] ___slab_alloc+0x1f4/0x760
> > LR [c0000000003d5b94] __slab_alloc+0x34/0x60
> > Call Trace:
> > [c0000008b3783960] [c0000000003d5734] ___slab_alloc+0x334/0x760 (unreliable)
> > [c0000008b3783a40] [c0000000003d5b94] __slab_alloc+0x34/0x60
> > [c0000008b3783a70] [c0000000003d6fa0] __kmalloc_node+0x110/0x490
> > [c0000008b3783af0] [c0000000003443d8] kvmalloc_node+0x58/0x110
> > [c0000008b3783b30] [c0000000003fee38] mem_cgroup_css_online+0x108/0x270
> > [c0000008b3783b90] [c000000000235aa8] online_css+0x48/0xd0
> > [c0000008b3783bc0] [c00000000023eaec] cgroup_apply_control_enable+0x2ec/0x4d0
> > [c0000008b3783ca0] [c000000000242318] cgroup_mkdir+0x228/0x5f0
> > [c0000008b3783d10] [c00000000051e170] kernfs_iop_mkdir+0x90/0xf0
> > [c0000008b3783d50] [c00000000043dc00] vfs_mkdir+0x110/0x230
> > [c0000008b3783da0] [c000000000441c90] do_mkdirat+0xb0/0x1a0
> > [c0000008b3783e20] [c00000000000b278] system_call+0x5c/0x68
> >
> > Fix this by verifying the node is online before accessing the pgdat
> > structure. Fix the same for node_spanned_pages() too.
> >
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: linux-mm@kvack.org
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Sachin Sant <sachinp@linux.vnet.ibm.com>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Cc: Christopher Lameter <cl@linux.com>
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > Cc: Bharata B Rao <bharata@linux.ibm.com>
> > Cc: Nathan Lynch <nathanl@linux.ibm.com>
> >
> > Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> > Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> > include/linux/mmzone.h | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index f3f264826423..88078a3b95e5 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -756,8 +756,10 @@ typedef struct pglist_data {
> > atomic_long_t vm_stat[NR_VM_NODE_STAT_ITEMS];
> > } pg_data_t;
> >
> > -#define node_present_pages(nid) (NODE_DATA(nid)->node_present_pages)
> > -#define node_spanned_pages(nid) (NODE_DATA(nid)->node_spanned_pages)
> > +#define node_present_pages(nid) \
> > + (node_online(nid) ? NODE_DATA(nid)->node_present_pages : 0)
> > +#define node_spanned_pages(nid) \
> > + (node_online(nid) ? NODE_DATA(nid)->node_spanned_pages : 0)
>
> I believe this is a wrong approach. We really do not want to special
> case all the places which require NODE_DATA. Can we please go and
> allocate pgdat for all possible nodes?
>
I can do that but the question I had was should we make this change just for
Powerpc or should the change be for other archs.
NODE_DATA initialization always seems to be in arch specific code.
The other archs that are affected seem to be mips, sh and sparc
These archs seem to have making an assumption that NODE_DATA has to be local
only,
For example on sparc / arch/sparc/mm/init_64.c in allocate_node_data function.
NODE_DATA(nid) = memblock_alloc_node(sizeof(struct pglist_data),
SMP_CACHE_BYTES, nid);
if (!NODE_DATA(nid)) {
prom_printf("Cannot allocate pglist_data for nid[%d]\n", nid);
prom_halt();
}
NODE_DATA(nid)->node_id = nid;
So even if I make changes to allocate NODE_DATA from fallback node, I may not
be able to test them.
So please let me know your thoughts around the same.
> The current state of memory less hacks subtle bugs poping up here and
> there just prove that we should have done that from the very begining
> IMHO.
>
> > #ifdef CONFIG_FLAT_NODE_MEM_MAP
> > #define pgdat_page_nr(pgdat, pagenr) ((pgdat)->node_mem_map + (pagenr))
> > #else
> > --
> > 2.18.1
>
> --
> Michal Hocko
> SUSE Labs
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: Slub: Increased mem consumption on cpu,mem-less node powerpc guest
From: Vlastimil Babka @ 2020-03-18 10:18 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Sachin Sant, Andrew Morton, linuxppc-dev, aneesh.kumar, bharata,
Pekka Enberg, linux-mm, David Rientjes, Christoph Lameter,
Michal Hocko, Joonsoo Kim
In-Reply-To: <20200318032044.GC4879@linux.vnet.ibm.com>
On 3/18/20 4:20 AM, Srikar Dronamraju wrote:
> * Vlastimil Babka <vbabka@suse.cz> [2020-03-17 17:45:15]:
>>
>> Yes, that Kirill's patch was about the memcg shrinker map allocation. But the
>> patch hunk that Bharata posted as a "hack" that fixes the problem, it follows
>> that there has to be something else that calls kmalloc_node(node) where node is
>> one that doesn't have present pages.
>>
>> He mentions alloc_fair_sched_group() which has:
>>
>> for_each_possible_cpu(i) {
>> cfs_rq = kzalloc_node(sizeof(struct cfs_rq),
>> GFP_KERNEL, cpu_to_node(i));
>> ...
>> se = kzalloc_node(sizeof(struct sched_entity),
>> GFP_KERNEL, cpu_to_node(i));
>>
>
>
> Sachin's experiment.
> Upstream-next/ memcg /
> possible nodes were 0-31
> online nodes were 0-1
> kmalloc_node called for_each_node / for_each_possible_node.
> This would crash while allocating slab from !N_ONLINE nodes.
So you're saying the crash was actually for allocation on e.g. node 2, not node 0?
But I believe it was on node 0, because init_kmem_cache_nodes() will only
allocate kmem_cache_node on nodes with N_NORMAL_MEMORY (which doesn't include
0), and slab_mem_going_online_callback() was probably not called for node 0 (it
was not dynamically onlined).
Also if node 0 was fine, node_to_mem_node(2-31) (not initialized explicitly)
would have returned 0 and thus not crash as well.
> Bharata's experiment.
> Upstream
> possible nodes were 0-1
> online nodes were 0-1
> kmalloc_node called for_each_online_node/ for_each_possible_cpu
> i.e kmalloc is called for N_ONLINE nodes.
> So wouldn't crash
>
> Even if his possible nodes were 0-256. I don't think we have kmalloc_node
> being called in !N_ONLINE nodes. Hence its not crashing.
> If we see the above code that you quote, kzalloc_node is using cpu_to_node
> which in Bharata's case will always return 1.
Are you sure that for_each_possible_cpu(), cpu_to_node() will be 1? Are all of
them properly initialized or is there a similar issue as with
node_to_mem_node(), that some were not initialized and thus cpu_to_node() will
return 0?
Because AFAICS, if kzalloc_node() was always called 1, then
node_present_pages(1) is true, and the "hack" that Bharata reports to work in
his original mail would make no functional difference.
>
>> I assume one of these structs is 1k and other 512 bytes (rounded) and that for
>> some possible cpu's cpu_to_node(i) will be 0, which has no present pages. And as
>> Bharata pasted, node_to_mem_node(0) = 0
>> So this looks like the same scenario, but it doesn't crash? Is the node 0
>> actually online here, and/or does it have N_NORMAL_MEMORY state?
>
> I still dont have any clue on the leak though.
Let's assume that kzalloc_node() was called with 0 for some of the possible
CPU's. I still wonder why it won't crash, but let's assume kmem_cache_node does
exist for node 0 here.
So the execution AFAICS goes like this:
slab_alloc_node(0)
c = raw_cpu_ptr(s->cpu_slab);
object = c->freelist;
page = c->page;
if (unlikely(!object || !node_match(page, node))) {
// whatever we have in the per-cpu cache must be from node 1
// because node 0 has no memory, so there's no node_match and thus
__slab_alloc(node == 0)
___slab_alloc(node == 0)
page = c->page;
redo:
if (unlikely(!node_match(page, node))) { // still no match
int searchnode = node;
if (node != NUMA_NO_NODE && !node_present_pages(node))
// true && true for node 0
searchnode = node_to_mem_node(node);
// searchnode is 0, not 1
if (unlikely(!node_match(page, searchnode))) {
// page still from node 1, searchnode is 0, no match
stat(s, ALLOC_NODE_MISMATCH);
deactivate_slab(s, page, c->freelist, c);
// we removed the slab from cpu's cache
goto new_slab;
}
new_slab:
if (slub_percpu_partial(c)) {
page = c->page = slub_percpu_partial(c);
slub_set_percpu_partial(c, page);
stat(s, CPU_PARTIAL_ALLOC);
goto redo;
// huh, so with CONFIG_SLUB_CPU_PARTIAL
// this can become an infinite loop actually?
}
// Bharata's slub stats don't include cpu_partial_alloc so I assume
// CONFIG_SLUB_CPU_PARTIAL is not enabled and we don't loop
freelist = new_slab_objects(s, gfpflags, node, &c);
freelist = new_slab_objects(s, gfpflags, node, &c);
if (node == NUMA_NO_NODE) // false, it's 0
else if (!node_present_pages(node)) // true for 0
searchnode = node_to_mem_node(node); // still 0
object = get_partial_node(s, get_node(s, searchnode),...);
// object is NULL as node 0 has nothing
// but we have node == 0 so we return the NULL
if (object || node != NUMA_NO_NODE)
return object;
// and we don't fallback to get_any_partial which would
// have found e.g. the slab we deactivated earlier
return get_any_partial(s, flags, c);
page = new_slab(s, flags, node);
// we attempt to allocate new slab on node 0, but it will come
// from node 1
So that explains the leak I think. We keep throwing away slabs from node 1 only
to allocate new ones on node 1. Effectively each cfs_rq object and each
sched_entity object will get a new (high-order?) page
for a possible cpu where cpu_to_node() is 0.
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: Check for node_online in node_present_pages
From: Michal Hocko @ 2020-03-18 10:02 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Sachin Sant, Nathan Lynch, Bharata B Rao, linux-mm, Kirill Tkhai,
Mel Gorman, Joonsoo Kim, Andrew Morton, linuxppc-dev,
Christopher Lameter, Vlastimil Babka
In-Reply-To: <20200318072810.9735-2-srikar@linux.vnet.ibm.com>
On Wed 18-03-20 12:58:07, Srikar Dronamraju wrote:
> Calling a kmalloc_node on a possible node which is not yet onlined can
> lead to panic. Currently node_present_pages() doesn't verify the node is
> online before accessing the pgdat for the node. However pgdat struct may
> not be available resulting in a crash.
>
> NIP [c0000000003d55f4] ___slab_alloc+0x1f4/0x760
> LR [c0000000003d5b94] __slab_alloc+0x34/0x60
> Call Trace:
> [c0000008b3783960] [c0000000003d5734] ___slab_alloc+0x334/0x760 (unreliable)
> [c0000008b3783a40] [c0000000003d5b94] __slab_alloc+0x34/0x60
> [c0000008b3783a70] [c0000000003d6fa0] __kmalloc_node+0x110/0x490
> [c0000008b3783af0] [c0000000003443d8] kvmalloc_node+0x58/0x110
> [c0000008b3783b30] [c0000000003fee38] mem_cgroup_css_online+0x108/0x270
> [c0000008b3783b90] [c000000000235aa8] online_css+0x48/0xd0
> [c0000008b3783bc0] [c00000000023eaec] cgroup_apply_control_enable+0x2ec/0x4d0
> [c0000008b3783ca0] [c000000000242318] cgroup_mkdir+0x228/0x5f0
> [c0000008b3783d10] [c00000000051e170] kernfs_iop_mkdir+0x90/0xf0
> [c0000008b3783d50] [c00000000043dc00] vfs_mkdir+0x110/0x230
> [c0000008b3783da0] [c000000000441c90] do_mkdirat+0xb0/0x1a0
> [c0000008b3783e20] [c00000000000b278] system_call+0x5c/0x68
>
> Fix this by verifying the node is online before accessing the pgdat
> structure. Fix the same for node_spanned_pages() too.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Christopher Lameter <cl@linux.com>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Bharata B Rao <bharata@linux.ibm.com>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
>
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> include/linux/mmzone.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index f3f264826423..88078a3b95e5 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -756,8 +756,10 @@ typedef struct pglist_data {
> atomic_long_t vm_stat[NR_VM_NODE_STAT_ITEMS];
> } pg_data_t;
>
> -#define node_present_pages(nid) (NODE_DATA(nid)->node_present_pages)
> -#define node_spanned_pages(nid) (NODE_DATA(nid)->node_spanned_pages)
> +#define node_present_pages(nid) \
> + (node_online(nid) ? NODE_DATA(nid)->node_present_pages : 0)
> +#define node_spanned_pages(nid) \
> + (node_online(nid) ? NODE_DATA(nid)->node_spanned_pages : 0)
I believe this is a wrong approach. We really do not want to special
case all the places which require NODE_DATA. Can we please go and
allocate pgdat for all possible nodes?
The current state of memory less hacks subtle bugs poping up here and
there just prove that we should have done that from the very begining
IMHO.
> #ifdef CONFIG_FLAT_NODE_MEM_MAP
> #define pgdat_page_nr(pgdat, pagenr) ((pgdat)->node_mem_map + (pagenr))
> #else
> --
> 2.18.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Fix MCE handling on pseries
From: Ganesh @ 2020-03-17 14:35 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev, mpe; +Cc: mahesh
In-Reply-To: <1584437866.2pbq6ca4ia.astroid@bobo.none>
[-- Attachment #1: Type: text/plain, Size: 3197 bytes --]
On 3/17/20 3:31 PM, Nicholas Piggin wrote:
> Ganesh's on March 16, 2020 9:47 pm:
>>
>> On 3/14/20 9:18 AM, Nicholas Piggin wrote:
>>> Ganesh Goudar's on March 14, 2020 12:04 am:
>>>> MCE handling on pSeries platform fails as recent rework to use common
>>>> code for pSeries and PowerNV in machine check error handling tries to
>>>> access per-cpu variables in realmode. The per-cpu variables may be
>>>> outside the RMO region on pSeries platform and needs translation to be
>>>> enabled for access. Just moving these per-cpu variable into RMO region
>>>> did'nt help because we queue some work to workqueues in real mode, which
>>>> again tries to touch per-cpu variables.
>>> Which queues are these? We should not be using Linux workqueues, but the
>>> powerpc mce code which uses irq_work.
>> Yes, irq work queues accesses memory outside RMO.
>> irq_work_queue()->__irq_work_queue_local()->[this_cpu_ptr(&lazy_list) | this_cpu_ptr(&raised_list)]
> Hmm, okay.
>
>>>> Also fwnmi_release_errinfo()
>>>> cannot be called when translation is not enabled.
>>> Why not?
>> It crashes when we try to get RTAS token for "ibm, nmi-interlock" device
>> tree node. But yes we can avoid it by storing it rtas_token somewhere but haven't
>> tried it, here is the backtrace I got when fwnmi_release_errinfo() called from
>> realmode handler.
> Okay, I actually had problems with that messing up soft-irq state too
> and so I sent a patch to get rid of it, but that's the least of your
> problems really.
>
>>>> This patch fixes this by enabling translation in the exception handler
>>>> when all required real mode handling is done. This change only affects
>>>> the pSeries platform.
>>> Not supposed to do this, because we might not be in a state
>>> where the MMU is ready to be turned on at this point.
>>>
>>> I'd like to understand better which accesses are a problem, and whether
>>> we can fix them all to be in the RMO.
>> I faced three such access problems,
>> * accessing per-cpu data (like mce_event,mce_event_queue and mce_event_queue),
>> we can move this inside RMO.
>> * calling fwnmi_release_errinfo().
>> * And queuing work to irq_work_queue, not sure how to fix this.
> Yeah. The irq_work_queue one is the biggest problem.
>
> This code "worked" prior to the series unifying pseries and powernv
> machine check handlers, 9ca766f9891d ("powerpc/64s/pseries: machine
> check convert to use common event code") and friends. But it does in
> basically the same way as your fix (i.e., it runs this early handler
> in virtual mode), but that's not really the right fix.
>
> Consider: you get a SLB multi hit on a kernel address due to hardware or
> software error. That access causes a MCE, but before the error can be
> decode to save and flush the SLB, you turn on relocation and that
> causes another SLB multi hit...
We turn on relocation only after all the realmode handling/recovery is done
like SLB flush and reload, All we do after we turn relocation on is saving
mce event to array and queuing the work to irq_workqueue.
So we are good to turn it on here.
> I think the irq_work subsystem will have to be changed to use an array
> unfortunately.
>
> Thanks,
> Nick
>
[-- Attachment #2: Type: text/html, Size: 4803 bytes --]
^ permalink raw reply
* [PATCH 00/12] Fix broken references for Documentation/*
From: Mauro Carvalho Chehab @ 2020-03-17 13:10 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Rafael J. Wysocki, Linus Walleij, dri-devel, Paul Mackerras,
Miquel Raynal, linux-arch, Tyler Hicks, Christoph Hellwig,
Doug Ledford, Alan Stern, Uwe Kleine-König, Julien Thierry,
Michal Vokáč, linux-pm, Boqun Feng, Jyri Sarha,
Alexander Viro, Thomas Gleixner, Sean Paul, Anton Altaparmakov,
Greg Kroah-Hartman, linux-kernel, linux-crypto, linux-fsdevel,
Andrew Morton, freedreno, Wanpeng Li, Benjamin Gaignard,
David Airlie, Harry Wei, Corentin Labbe, Ben Peled,
Daniel Lezcano, kvmarm, Bartosz Golaszewski, Wolfgang Grandegger,
Andrea Parri, Suzuki K Poulose, linux-arm-msm, kvm-ppc, linux-can,
linux-gpio, Maxime Ripard, OGAWA Hirofumi, Mathieu Poirier,
Nicolas Pitre, linux-ntfs-dev, Miklos Szeredi,
Sean Christopherson, James Morse, Alexander Shishkin,
Akira Yokosawa, Alex Shi, Will Deacon, linux-afs, Dan Murphy,
Rob Herring, Jonathan Corbet, linux-rdma, Kishon Vijay Abraham I,
Chen-Yu Tsai, Tomi Valkeinen, linux-arm-kernel, Federico Vaga,
Jade Alglave, Alexey Dobriyan, linux-pwm, Gregory CLEMENT,
Borislav Petkov, Maxime Ripard, Luc Maranget, Yuti Amonkar,
Joseph Qi, Dmitry Torokhov, Thierry Reding, David S. Miller,
ocfs2-devel, Jan Kara, kvm, Peter Zijlstra, Amir Goldstein,
David Howells, linux-mm, Mikulas Patocka, H. Peter Anvin,
Amit Kucheria, Herbert Xu, Mauro Carvalho Chehab, Marc Zyngier,
Mark Fasheh, x86, Jason Gunthorpe, Ingo Molnar, Zhang Rui,
Mike Leach, devicetree, Thomas Zimmermann, Paul E. McKenney,
Daniel Lustig, ecryptfs, Maarten Lankhorst, Rob Herring,
Marc Kleine-Budde, Nicholas Piggin, Tigran A. Aivazian,
David Sterba, Jim Mattson, netdev, linux-unionfs, Rob Clark,
Daniel Vetter, Paolo Bonzini, Vitaly Kuznetsov, linuxppc-dev,
Joerg Roedel, Joel Becker
This patch series is against next-20200317. It fixes all references to files
under Documentation/* that were moved, renamed or removed.
After this patch series, this script:
./scripts/documentation-file-ref-check
Doesn't complain about any broken reference.
Mauro Carvalho Chehab (12):
MAINTAINERS: dt: update display/allwinner file entry
MAINTAINERS: dt: update etnaviv file reference
MAINTAINERS: drop an old reference to stm32 pwm timers doc
docs: dt: fix references to m_can.txt file
docs: dt: fix references to ap806-system-controller.txt
docs: dt: fix a broken reference to input.yaml
docs: dt: fix broken reference to phy-cadence-torrent.yaml
docs: fix broken references to text files
docs: fix broken references for ReST files that moved around
docs: dt: display/ti: fix typos at the devicetree/ directory name
docs: filesystems: fix renamed references
docs: kernel-parameters.txt: remove reference for removed Intel MPX
Documentation/ABI/stable/sysfs-devices-node | 2 +-
Documentation/ABI/testing/procfs-smaps_rollup | 2 +-
Documentation/admin-guide/cpu-load.rst | 2 +-
.../admin-guide/kernel-parameters.txt | 11 ++--
Documentation/admin-guide/nfs/nfsroot.rst | 2 +-
.../bindings/arm/freescale/fsl,scu.txt | 2 +-
.../bindings/display/ti/ti,am65x-dss.yaml | 2 +-
.../bindings/display/ti/ti,j721e-dss.yaml | 2 +-
.../bindings/display/ti/ti,k2g-dss.yaml | 2 +-
.../devicetree/bindings/gpio/gpio-mvebu.txt | 2 +-
.../devicetree/bindings/net/can/tcan4x5x.txt | 2 +-
.../bindings/phy/ti,phy-j721e-wiz.yaml | 2 +-
.../bindings/thermal/armada-thermal.txt | 2 +-
.../doc-guide/maintainer-profile.rst | 2 +-
.../driver-api/driver-model/device.rst | 4 +-
.../driver-api/driver-model/overview.rst | 2 +-
Documentation/filesystems/dax.txt | 2 +-
Documentation/filesystems/dnotify.txt | 2 +-
.../filesystems/ramfs-rootfs-initramfs.rst | 2 +-
Documentation/filesystems/sysfs.rst | 2 +-
Documentation/memory-barriers.txt | 2 +-
.../powerpc/firmware-assisted-dump.rst | 2 +-
Documentation/process/adding-syscalls.rst | 2 +-
Documentation/process/submit-checklist.rst | 2 +-
.../it_IT/process/adding-syscalls.rst | 2 +-
.../it_IT/process/submit-checklist.rst | 2 +-
.../translations/ko_KR/memory-barriers.txt | 2 +-
.../translations/zh_CN/filesystems/sysfs.txt | 8 +--
.../zh_CN/process/submit-checklist.rst | 2 +-
Documentation/virt/kvm/arm/pvtime.rst | 2 +-
Documentation/virt/kvm/devices/vcpu.rst | 2 +-
Documentation/virt/kvm/hypercalls.rst | 4 +-
Documentation/virt/kvm/mmu.rst | 2 +-
Documentation/virt/kvm/review-checklist.rst | 2 +-
MAINTAINERS | 61 +++++++++----------
Next/merge.log | 12 ++--
arch/powerpc/include/uapi/asm/kvm_para.h | 2 +-
arch/x86/kvm/mmu/mmu.c | 2 +-
drivers/base/core.c | 2 +-
.../allwinner/sun8i-ce/sun8i-ce-cipher.c | 2 +-
.../crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 2 +-
.../allwinner/sun8i-ss/sun8i-ss-cipher.c | 2 +-
.../crypto/allwinner/sun8i-ss/sun8i-ss-core.c | 2 +-
drivers/gpu/drm/Kconfig | 2 +-
drivers/gpu/drm/drm_ioctl.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 +-
drivers/hwtracing/coresight/Kconfig | 2 +-
drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
fs/Kconfig | 2 +-
fs/Kconfig.binfmt | 2 +-
fs/adfs/Kconfig | 2 +-
fs/affs/Kconfig | 2 +-
fs/afs/Kconfig | 6 +-
fs/bfs/Kconfig | 2 +-
fs/cramfs/Kconfig | 2 +-
fs/ecryptfs/Kconfig | 2 +-
fs/fat/Kconfig | 8 +--
fs/fuse/Kconfig | 2 +-
fs/fuse/dev.c | 2 +-
fs/hfs/Kconfig | 2 +-
fs/hpfs/Kconfig | 2 +-
fs/isofs/Kconfig | 2 +-
fs/namespace.c | 2 +-
fs/notify/inotify/Kconfig | 2 +-
fs/ntfs/Kconfig | 2 +-
fs/ocfs2/Kconfig | 2 +-
fs/overlayfs/Kconfig | 6 +-
fs/proc/Kconfig | 4 +-
fs/romfs/Kconfig | 2 +-
fs/sysfs/dir.c | 2 +-
fs/sysfs/file.c | 2 +-
fs/sysfs/mount.c | 2 +-
fs/sysfs/symlink.c | 2 +-
fs/sysv/Kconfig | 2 +-
fs/udf/Kconfig | 2 +-
include/linux/kobject.h | 2 +-
include/linux/kobject_ns.h | 2 +-
include/linux/mm.h | 4 +-
include/linux/relay.h | 2 +-
include/linux/sysfs.h | 2 +-
include/uapi/linux/ethtool_netlink.h | 2 +-
include/uapi/linux/kvm.h | 4 +-
include/uapi/rdma/rdma_user_ioctl_cmds.h | 2 +-
kernel/relay.c | 2 +-
lib/kobject.c | 4 +-
mm/gup.c | 12 ++--
tools/include/uapi/linux/kvm.h | 4 +-
virt/kvm/arm/vgic/vgic-mmio-v3.c | 2 +-
virt/kvm/arm/vgic/vgic.h | 4 +-
89 files changed, 151 insertions(+), 151 deletions(-)
--
2.24.1
^ permalink raw reply
* [PATCH v3] ocxl: control via sysfs whether the FPGA is reloaded on a link reset
From: Frederic Barrat @ 2020-03-18 10:02 UTC (permalink / raw)
To: linuxppc-dev, fbarrat, clombard, felix, ajd, alastair
From: Philippe Bergheaud <felix@linux.ibm.com>
Some opencapi FPGA images allow to control if the FPGA should be reloaded
on the next adapter reset. If it is supported, the image specifies it
through a Vendor Specific DVSEC in the config space of function 0.
Signed-off-by: Philippe Bergheaud <felix@linux.ibm.com>
---
Changelog:
v2:
- refine ResetReload debug message
- do not call get_function_0() if pci_dev is for function 0
v3:
- avoid get_function_0() in ocxl_config_set_reset_reload also
Documentation/ABI/testing/sysfs-class-ocxl | 10 ++++
drivers/misc/ocxl/config.c | 68 +++++++++++++++++++++-
drivers/misc/ocxl/ocxl_internal.h | 6 ++
drivers/misc/ocxl/sysfs.c | 35 +++++++++++
include/misc/ocxl-config.h | 1 +
5 files changed, 119 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-class-ocxl b/Documentation/ABI/testing/sysfs-class-ocxl
index b5b1fa197592..b9ea671d5805 100644
--- a/Documentation/ABI/testing/sysfs-class-ocxl
+++ b/Documentation/ABI/testing/sysfs-class-ocxl
@@ -33,3 +33,13 @@ Date: January 2018
Contact: linuxppc-dev@lists.ozlabs.org
Description: read/write
Give access the global mmio area for the AFU
+
+What: /sys/class/ocxl/<afu name>/reload_on_reset
+Date: February 2020
+Contact: linuxppc-dev@lists.ozlabs.org
+Description: read/write
+ Control whether the FPGA is reloaded on a link reset
+ 0 Do not reload FPGA image from flash
+ 1 Reload FPGA image from flash
+ unavailable
+ The device does not support this capability
diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
index c8e19bfb5ef9..b364b6ceb996 100644
--- a/drivers/misc/ocxl/config.c
+++ b/drivers/misc/ocxl/config.c
@@ -71,6 +71,20 @@ static int find_dvsec_afu_ctrl(struct pci_dev *dev, u8 afu_idx)
return 0;
}
+/**
+ * get_function_0() - Find a related PCI device (function 0)
+ * @device: PCI device to match
+ *
+ * Returns a pointer to the related device, or null if not found
+ */
+static struct pci_dev *get_function_0(struct pci_dev *dev)
+{
+ unsigned int devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
+
+ return pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
+ dev->bus->number, devfn);
+}
+
static void read_pasid(struct pci_dev *dev, struct ocxl_fn_config *fn)
{
u16 val;
@@ -159,7 +173,7 @@ static int read_dvsec_afu_info(struct pci_dev *dev, struct ocxl_fn_config *fn)
static int read_dvsec_vendor(struct pci_dev *dev)
{
int pos;
- u32 cfg, tlx, dlx;
+ u32 cfg, tlx, dlx, reset_reload;
/*
* vendor specific DVSEC is optional
@@ -183,6 +197,58 @@ static int read_dvsec_vendor(struct pci_dev *dev)
dev_dbg(&dev->dev, " CFG version = 0x%x\n", cfg);
dev_dbg(&dev->dev, " TLX version = 0x%x\n", tlx);
dev_dbg(&dev->dev, " DLX version = 0x%x\n", dlx);
+
+ if (ocxl_config_get_reset_reload(dev, &reset_reload) != 0)
+ dev_dbg(&dev->dev, " ResetReload is not available\n");
+ else
+ dev_dbg(&dev->dev, " ResetReload = 0x%x\n", reset_reload);
+ return 0;
+}
+
+int ocxl_config_get_reset_reload(struct pci_dev *dev, int *val)
+{
+ int reset_reload = -1;
+ int pos = 0;
+ struct pci_dev *dev0 = dev;
+
+ if (PCI_FUNC(dev->devfn) != 0)
+ dev0 = get_function_0(dev);
+
+ if (dev0)
+ pos = find_dvsec(dev0, OCXL_DVSEC_VENDOR_ID);
+
+ if (pos)
+ pci_read_config_dword(dev0,
+ pos + OCXL_DVSEC_VENDOR_RESET_RELOAD,
+ &reset_reload);
+ if (reset_reload == -1)
+ return reset_reload;
+
+ *val = reset_reload & BIT(0);
+ return 0;
+}
+
+int ocxl_config_set_reset_reload(struct pci_dev *dev, int val)
+{
+ int reset_reload = -1;
+ int pos = 0;
+ struct pci_dev *dev0 = dev;
+
+ if (PCI_FUNC(dev->devfn) != 0)
+ dev0 = get_function_0(dev);
+
+ if (dev0)
+ pos = find_dvsec(dev0, OCXL_DVSEC_VENDOR_ID);
+
+ if (pos)
+ pci_read_config_dword(dev0,
+ pos + OCXL_DVSEC_VENDOR_RESET_RELOAD,
+ &reset_reload);
+ if (reset_reload == -1)
+ return reset_reload;
+
+ val &= BIT(0);
+ pci_write_config_dword(dev0, pos + OCXL_DVSEC_VENDOR_RESET_RELOAD, val);
return 0;
}
diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
index 345bf843a38e..af9a84aeee6f 100644
--- a/drivers/misc/ocxl/ocxl_internal.h
+++ b/drivers/misc/ocxl/ocxl_internal.h
@@ -112,6 +112,12 @@ void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
*/
int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
+/*
+ * Control whether the FPGA is reloaded on a link reset
+ */
+int ocxl_config_get_reset_reload(struct pci_dev *dev, int *val);
+int ocxl_config_set_reset_reload(struct pci_dev *dev, int val);
+
/*
* Check if an AFU index is valid for the given function.
*
diff --git a/drivers/misc/ocxl/sysfs.c b/drivers/misc/ocxl/sysfs.c
index 58f1ba264206..8f69f7311343 100644
--- a/drivers/misc/ocxl/sysfs.c
+++ b/drivers/misc/ocxl/sysfs.c
@@ -51,11 +51,46 @@ static ssize_t contexts_show(struct device *device,
afu->pasid_count, afu->pasid_max);
}
+static ssize_t reload_on_reset_show(struct device *device,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ocxl_afu *afu = to_afu(device);
+ struct ocxl_fn *fn = afu->fn;
+ struct pci_dev *pci_dev = to_pci_dev(fn->dev.parent);
+ int val;
+
+ if (ocxl_config_get_reset_reload(pci_dev, &val))
+ return scnprintf(buf, PAGE_SIZE, "unavailable\n");
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t reload_on_reset_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ocxl_afu *afu = to_afu(device);
+ struct ocxl_fn *fn = afu->fn;
+ struct pci_dev *pci_dev = to_pci_dev(fn->dev.parent);
+ int rc, val;
+
+ rc = sscanf(buf, "%i", &val);
+ if ((rc != 1) || !(val == 1 || val == 0))
+ return -EINVAL;
+
+ if (ocxl_config_set_reset_reload(pci_dev, val))
+ return -ENODEV;
+
+ return count;
+}
+
static struct device_attribute afu_attrs[] = {
__ATTR_RO(global_mmio_size),
__ATTR_RO(pp_mmio_size),
__ATTR_RO(afu_version),
__ATTR_RO(contexts),
+ __ATTR_RW(reload_on_reset),
};
static ssize_t global_mmio_read(struct file *filp, struct kobject *kobj,
diff --git a/include/misc/ocxl-config.h b/include/misc/ocxl-config.h
index 3526fa996a22..ccfd3b463517 100644
--- a/include/misc/ocxl-config.h
+++ b/include/misc/ocxl-config.h
@@ -41,5 +41,6 @@
#define OCXL_DVSEC_VENDOR_CFG_VERS 0x0C
#define OCXL_DVSEC_VENDOR_TLX_VERS 0x10
#define OCXL_DVSEC_VENDOR_DLX_VERS 0x20
+#define OCXL_DVSEC_VENDOR_RESET_RELOAD 0x38
#endif /* _OCXL_CONFIG_H_ */
--
2.25.1
^ permalink raw reply related
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