| Message ID | 20230208093016.20670-2-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Fixes to error handling logic | expand |
Hi Ilpo, I do not see a why two patch series are needed for the resctrl fixes. It may make it easier for everybody if it is handled as one patch series (with fixes first)? On 2/8/2023 1:30 AM, Ilpo Järvinen wrote: > From: Fenghua Yu <fenghua.yu@intel.com> > > malloc_and_init_memory() in fill_buf isn't checking if memalign() > successfully allocated memory or not before accessing the memory. > > Check the return value of memalign() and return NULL if allocating > aligned memory fails. > > Fixes: a2561b12fe39 ("selftests/resctrl: Add built in benchmark") > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> Missing your Signed-off-by? > --- > tools/testing/selftests/resctrl/fill_buf.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > index 56ccbeae0638..f4880c962ec4 100644 > --- a/tools/testing/selftests/resctrl/fill_buf.c > +++ b/tools/testing/selftests/resctrl/fill_buf.c > @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s) > size_t s64; > > void *p = memalign(PAGE_SIZE, s); This may also be a good time to stop using an obsolete call? > + if (!p) > + return p; Could you please return NULL explicitly? > > p64 = (uint64_t *)p; > s64 = s / sizeof(uint64_t); Reinette
On Mon, 13 Feb 2023, Reinette Chatre wrote: > I do not see a why two patch series are needed for > the resctrl fixes. It may make it easier for everybody if > it is handled as one patch series (with fixes first)? Ok, I can put the fixes and cleanups into one series. > On 2/8/2023 1:30 AM, Ilpo Järvinen wrote: > > From: Fenghua Yu <fenghua.yu@intel.com> > > > > malloc_and_init_memory() in fill_buf isn't checking if memalign() > > successfully allocated memory or not before accessing the memory. > > > > Check the return value of memalign() and return NULL if allocating > > aligned memory fails. > > > > Fixes: a2561b12fe39 ("selftests/resctrl: Add built in benchmark") > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > > Missing your Signed-off-by? These were intentionally. When I didn't modify the original patch at all during forward porting it, I just kept the original From and SoB as is. But from the doc you pointed me to, I see now x86 wants also handlers sobs. > > --- > > tools/testing/selftests/resctrl/fill_buf.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > > index 56ccbeae0638..f4880c962ec4 100644 > > --- a/tools/testing/selftests/resctrl/fill_buf.c > > +++ b/tools/testing/selftests/resctrl/fill_buf.c > > @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s) > > size_t s64; > > > > void *p = memalign(PAGE_SIZE, s); > > This may also be a good time to stop using an obsolete call? Sure, I can add another patch to change that to posix_memalign(). > > + if (!p) > > + return p; > > Could you please return NULL explicitly? I'll change it. Thanks for you comments.
Hi Ilpo, On 2/14/2023 1:32 AM, Ilpo Järvinen wrote: > On Mon, 13 Feb 2023, Reinette Chatre wrote: >> Missing your Signed-off-by? > > These were intentionally. When I didn't modify the original patch at > all during forward porting it, I just kept the original From and SoB as > is. But from the doc you pointed me to, I see now x86 wants also handlers > sobs. I do not think this is x86 specific. Documentation/process/submitting-patches.rst states: "Any further SoBs (Signed-off-by:'s) following the author's SoB are from people handling and transporting the patch, but were not involved in its development. SoB chains should reflect the **real** route a patch took as it was propagated to the maintainers and ultimately to Linus, with the first SoB entry signalling primary authorship of a single author." > >>> --- >>> tools/testing/selftests/resctrl/fill_buf.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c >>> index 56ccbeae0638..f4880c962ec4 100644 >>> --- a/tools/testing/selftests/resctrl/fill_buf.c >>> +++ b/tools/testing/selftests/resctrl/fill_buf.c >>> @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s) >>> size_t s64; >>> >>> void *p = memalign(PAGE_SIZE, s); >> >> This may also be a good time to stop using an obsolete call? > > Sure, I can add another patch to change that to posix_memalign(). You can also consider aligned_alloc(). Reinette
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 56ccbeae0638..f4880c962ec4 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s) size_t s64; void *p = memalign(PAGE_SIZE, s); + if (!p) + return p; p64 = (uint64_t *)p; s64 = s / sizeof(uint64_t);