From: James Clark <james.clark@arm.com> To: Ian Rogers <irogers@google.com>, Namhyung Kim <namhyung@kernel.org> Cc: "Peter Zijlstra" <peterz@infradead.org>, "Ingo Molnar" <mingo@redhat.com>, "Arnaldo Carvalho de Melo" <acme@kernel.org>, "Mark Rutland" <mark.rutland@arm.com>, "Alexander Shishkin" <alexander.shishkin@linux.intel.com>, "Jiri Olsa" <jolsa@kernel.org>, "Adrian Hunter" <adrian.hunter@intel.com>, "Suzuki K Poulose" <suzuki.poulose@arm.com>, "Mike Leach" <mike.leach@linaro.org>, "Leo Yan" <leo.yan@linaro.org>, "John Garry" <john.g.garry@oracle.com>, "Will Deacon" <will@kernel.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Darren Hart" <dvhart@infradead.org>, "Davidlohr Bueso" <dave@stgolabs.net>, "André Almeida" <andrealmeid@igalia.com>, "Kan Liang" <kan.liang@linux.intel.com>, "K Prateek Nayak" <kprateek.nayak@amd.com>, "Sean Christopherson" <seanjc@google.com>, "Paolo Bonzini" <pbonzini@redhat.com>, "Kajol Jain" <kjain@linux.ibm.com>, "Athira Rajeev" <atrajeev@linux.vnet.ibm.com>, "Andrew Jones" <ajones@ventanamicro.com>, "Alexandre Ghiti" <alexghiti@rivosinc.com>, "Atish Patra" <atishp@rivosinc.com>, "Steinar H. Gunderson" <sesse@google.com>, "Yang Jihong" <yangjihong1@huawei.com>, "Yang Li" <yang.lee@linux.alibaba.com>, "Changbin Du" <changbin.du@huawei.com>, "Sandipan Das" <sandipan.das@amd.com>, "Ravi Bangoria" <ravi.bangoria@amd.com>, "Paran Lee" <p4ranlee@gmail.com>, "Nick Desaulniers" <ndesaulniers@google.com>, "Huacai Chen" <chenhuacai@kernel.org>, "Yanteng Si" <siyanteng@loongson.cn>, linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, bpf@vger.kernel.org Subject: Re: [PATCH v3 2/8] libperf cpumap: Ensure empty cpumap is NULL from alloc Date: Mon, 19 Feb 2024 10:38:16 +0000 [thread overview] Message-ID: <b260e3e1-ebe9-4fc2-6b81-bc4735a7bb66@arm.com> (raw) In-Reply-To: <CAP-5=fVNLoes2VaCcqrueiDLBZAZNthSJVD17z77cnyE7wF7ag@mail.gmail.com> On 17/02/2024 00:52, Ian Rogers wrote: > On Fri, Feb 16, 2024 at 4:25 PM Namhyung Kim <namhyung@kernel.org> wrote: >> >> On Fri, Feb 2, 2024 at 3:41 PM Ian Rogers <irogers@google.com> wrote: >>> >>> Potential corner cases could cause a cpumap to be allocated with size >>> 0, but an empty cpumap should be represented as NULL. Add a path in >>> perf_cpu_map__alloc to ensure this. >>> >>> Suggested-by: James Clark <james.clark@arm.com> >>> Closes: https://lore.kernel.org/lkml/2cd09e7c-eb88-6726-6169-647dcd0a8101@arm.com/ >>> Signed-off-by: Ian Rogers <irogers@google.com> >>> --- >>> tools/lib/perf/cpumap.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c >>> index ba49552952c5..cae799ad44e1 100644 >>> --- a/tools/lib/perf/cpumap.c >>> +++ b/tools/lib/perf/cpumap.c >>> @@ -18,9 +18,13 @@ void perf_cpu_map__set_nr(struct perf_cpu_map *map, int nr_cpus) >>> >>> struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus) >>> { >>> - RC_STRUCT(perf_cpu_map) *cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus); >>> + RC_STRUCT(perf_cpu_map) *cpus; >>> struct perf_cpu_map *result; >>> >>> + if (nr_cpus == 0) >>> + return NULL; >> >> But allocation failure also returns NULL. Then callers should check >> what's the expected result.> > Right, we don't have a habit of just aborting on memory allocation I'm not sure why we don't abort on allocation. It would simplify the code a lot and wouldn't change the behavior in any meaningful way. And it would also allow us to print out which line exactly failed which is much more useful than bubbling up the error and hiding it. If we're making the decision that an empty map == NULL rather than non-null but with 0 length then maybe it's time to start thinking about it. > errors. In the case that NULL is returned it is assumed that an empty > CPU map is appropriate. Adding checks throughout the code base that an > empty CPU map is only returned when 0 is given is beyond the scope of > this patch set. > > Thanks, > Ian > >> Thanks, >> Namhyung >> >>> + >>> + cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus); >>> if (ADD_RC_CHK(result, cpus)) { >>> cpus->nr = nr_cpus; >>> refcount_set(&cpus->refcnt, 1); >>> -- >>> 2.43.0.594.gd9cf4e227d-goog >>>
WARNING: multiple messages have this Message-ID (diff)
From: James Clark <james.clark@arm.com> To: Ian Rogers <irogers@google.com>, Namhyung Kim <namhyung@kernel.org> Cc: "Peter Zijlstra" <peterz@infradead.org>, "Ingo Molnar" <mingo@redhat.com>, "Arnaldo Carvalho de Melo" <acme@kernel.org>, "Mark Rutland" <mark.rutland@arm.com>, "Alexander Shishkin" <alexander.shishkin@linux.intel.com>, "Jiri Olsa" <jolsa@kernel.org>, "Adrian Hunter" <adrian.hunter@intel.com>, "Suzuki K Poulose" <suzuki.poulose@arm.com>, "Mike Leach" <mike.leach@linaro.org>, "Leo Yan" <leo.yan@linaro.org>, "John Garry" <john.g.garry@oracle.com>, "Will Deacon" <will@kernel.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Darren Hart" <dvhart@infradead.org>, "Davidlohr Bueso" <dave@stgolabs.net>, "André Almeida" <andrealmeid@igalia.com>, "Kan Liang" <kan.liang@linux.intel.com>, "K Prateek Nayak" <kprateek.nayak@amd.com>, "Sean Christopherson" <seanjc@google.com>, "Paolo Bonzini" <pbonzini@redhat.com>, "Kajol Jain" <kjain@linux.ibm.com>, "Athira Rajeev" <atrajeev@linux.vnet.ibm.com>, "Andrew Jones" <ajones@ventanamicro.com>, "Alexandre Ghiti" <alexghiti@rivosinc.com>, "Atish Patra" <atishp@rivosinc.com>, "Steinar H. Gunderson" <sesse@google.com>, "Yang Jihong" <yangjihong1@huawei.com>, "Yang Li" <yang.lee@linux.alibaba.com>, "Changbin Du" <changbin.du@huawei.com>, "Sandipan Das" <sandipan.das@amd.com>, "Ravi Bangoria" <ravi.bangoria@amd.com>, "Paran Lee" <p4ranlee@gmail.com>, "Nick Desaulniers" <ndesaulniers@google.com>, "Huacai Chen" <chenhuacai@kernel.org>, "Yanteng Si" <siyanteng@loongson.cn>, linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, bpf@vger.kernel.org Subject: Re: [PATCH v3 2/8] libperf cpumap: Ensure empty cpumap is NULL from alloc Date: Mon, 19 Feb 2024 10:38:16 +0000 [thread overview] Message-ID: <b260e3e1-ebe9-4fc2-6b81-bc4735a7bb66@arm.com> (raw) In-Reply-To: <CAP-5=fVNLoes2VaCcqrueiDLBZAZNthSJVD17z77cnyE7wF7ag@mail.gmail.com> On 17/02/2024 00:52, Ian Rogers wrote: > On Fri, Feb 16, 2024 at 4:25 PM Namhyung Kim <namhyung@kernel.org> wrote: >> >> On Fri, Feb 2, 2024 at 3:41 PM Ian Rogers <irogers@google.com> wrote: >>> >>> Potential corner cases could cause a cpumap to be allocated with size >>> 0, but an empty cpumap should be represented as NULL. Add a path in >>> perf_cpu_map__alloc to ensure this. >>> >>> Suggested-by: James Clark <james.clark@arm.com> >>> Closes: https://lore.kernel.org/lkml/2cd09e7c-eb88-6726-6169-647dcd0a8101@arm.com/ >>> Signed-off-by: Ian Rogers <irogers@google.com> >>> --- >>> tools/lib/perf/cpumap.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c >>> index ba49552952c5..cae799ad44e1 100644 >>> --- a/tools/lib/perf/cpumap.c >>> +++ b/tools/lib/perf/cpumap.c >>> @@ -18,9 +18,13 @@ void perf_cpu_map__set_nr(struct perf_cpu_map *map, int nr_cpus) >>> >>> struct perf_cpu_map *perf_cpu_map__alloc(int nr_cpus) >>> { >>> - RC_STRUCT(perf_cpu_map) *cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus); >>> + RC_STRUCT(perf_cpu_map) *cpus; >>> struct perf_cpu_map *result; >>> >>> + if (nr_cpus == 0) >>> + return NULL; >> >> But allocation failure also returns NULL. Then callers should check >> what's the expected result.> > Right, we don't have a habit of just aborting on memory allocation I'm not sure why we don't abort on allocation. It would simplify the code a lot and wouldn't change the behavior in any meaningful way. And it would also allow us to print out which line exactly failed which is much more useful than bubbling up the error and hiding it. If we're making the decision that an empty map == NULL rather than non-null but with 0 length then maybe it's time to start thinking about it. > errors. In the case that NULL is returned it is assumed that an empty > CPU map is appropriate. Adding checks throughout the code base that an > empty CPU map is only returned when 0 is given is beyond the scope of > this patch set. > > Thanks, > Ian > >> Thanks, >> Namhyung >> >>> + >>> + cpus = malloc(sizeof(*cpus) + sizeof(struct perf_cpu) * nr_cpus); >>> if (ADD_RC_CHK(result, cpus)) { >>> cpus->nr = nr_cpus; >>> refcount_set(&cpus->refcnt, 1); >>> -- >>> 2.43.0.594.gd9cf4e227d-goog >>> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-02-19 10:38 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-02-02 23:40 [PATCH v3 0/8] Clean up libperf cpumap's empty function Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-02 23:40 ` [PATCH v3 1/8] libperf cpumap: Add any, empty and min helpers Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-02 23:40 ` [PATCH v3 2/8] libperf cpumap: Ensure empty cpumap is NULL from alloc Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-17 0:25 ` Namhyung Kim 2024-02-17 0:25 ` Namhyung Kim 2024-02-17 0:52 ` Ian Rogers 2024-02-17 0:52 ` Ian Rogers 2024-02-19 10:38 ` James Clark [this message] 2024-02-19 10:38 ` James Clark 2024-02-02 23:40 ` [PATCH v3 3/8] perf arm-spe/cs-etm: Directly iterate CPU maps Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-17 1:01 ` Namhyung Kim 2024-02-17 1:01 ` Namhyung Kim 2024-02-17 1:33 ` Ian Rogers 2024-02-17 1:33 ` Ian Rogers 2024-02-19 10:16 ` James Clark 2024-02-19 10:16 ` James Clark 2024-02-02 23:40 ` [PATCH v3 4/8] perf intel-pt/intel-bts: Switch perf_cpu_map__has_any_cpu_or_is_empty use Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-02 23:40 ` [PATCH v3 5/8] perf cpumap: Clean up use of perf_cpu_map__has_any_cpu_or_is_empty Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-02 23:40 ` [PATCH v3 6/8] perf arm64 header: Remove unnecessary CPU map get and put Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-02 23:40 ` [PATCH v3 7/8] perf stat: Remove duplicate cpus_map_matched function Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-02 23:40 ` [PATCH v3 8/8] perf cpumap: Use perf_cpu_map__for_each_cpu when possible Ian Rogers 2024-02-02 23:40 ` Ian Rogers 2024-02-14 22:02 ` [PATCH v3 0/8] Clean up libperf cpumap's empty function Ian Rogers 2024-02-14 22:02 ` Ian Rogers 2024-02-17 1:04 ` Namhyung Kim 2024-02-17 1:04 ` Namhyung Kim 2024-03-07 23:47 ` Namhyung Kim 2024-03-07 23:47 ` Namhyung Kim 2024-03-18 21:37 ` Arnaldo Carvalho de Melo 2024-03-18 21:37 ` Arnaldo Carvalho de Melo 2024-03-19 4:18 ` Namhyung Kim 2024-03-19 4:18 ` Namhyung Kim
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b260e3e1-ebe9-4fc2-6b81-bc4735a7bb66@arm.com \ --to=james.clark@arm.com \ --cc=acme@kernel.org \ --cc=adrian.hunter@intel.com \ --cc=ajones@ventanamicro.com \ --cc=alexander.shishkin@linux.intel.com \ --cc=alexghiti@rivosinc.com \ --cc=andrealmeid@igalia.com \ --cc=atishp@rivosinc.com \ --cc=atrajeev@linux.vnet.ibm.com \ --cc=bpf@vger.kernel.org \ --cc=changbin.du@huawei.com \ --cc=chenhuacai@kernel.org \ --cc=coresight@lists.linaro.org \ --cc=dave@stgolabs.net \ --cc=dvhart@infradead.org \ --cc=irogers@google.com \ --cc=john.g.garry@oracle.com \ --cc=jolsa@kernel.org \ --cc=kan.liang@linux.intel.com \ --cc=kjain@linux.ibm.com \ --cc=kprateek.nayak@amd.com \ --cc=leo.yan@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-perf-users@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mike.leach@linaro.org \ --cc=mingo@redhat.com \ --cc=namhyung@kernel.org \ --cc=ndesaulniers@google.com \ --cc=p4ranlee@gmail.com \ --cc=pbonzini@redhat.com \ --cc=peterz@infradead.org \ --cc=ravi.bangoria@amd.com \ --cc=sandipan.das@amd.com \ --cc=seanjc@google.com \ --cc=sesse@google.com \ --cc=siyanteng@loongson.cn \ --cc=suzuki.poulose@arm.com \ --cc=tglx@linutronix.de \ --cc=will@kernel.org \ --cc=yang.lee@linux.alibaba.com \ --cc=yangjihong1@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.