| Message ID | 20221026054508.19634-1-w@1wt.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4a95be7ed7669311350d041ca6cd37bf96f92d8c |
| Headers | show |
| Series | selftests/nolibc: always rebuild the sysroot when running a test | expand |
On Wed, Oct 26, 2022 at 07:45:08AM +0200, Willy Tarreau wrote: > Paul and myself got trapped a few times by not seeing the effects of > applying a patch to the nolibc source code until a "make clean" was > issued in the nolibc directory. It's particularly annoying when trying > to confirm that a proposed patch really solves a problem (or that > reverting it reintroduces the problem). > > The reason for the sysroot not being rebuilt was that it can be quite > slow. But in fact it's only slow after a "make clean" issued at the > kernel's topdir, because it's the main "make headers" that can take a > tens of seconds; as long as "usr/include" still contains headers, the > "headers_install" phase is only a quick "rsync", and rebuilding the > whole nolibc sysroot takes a bit less than one second, which is perfectly > acceptable for a test, even more once the time lost caused by misleading > results if factored in. > > This patch marks the sysroot target as phony and starts by clearing > the previous sysroot for the current architecture before reinstalling > it. Thanks to this, applying a patch to nolibc makes the effect > immediately visible to "make nolibc-test": > > $ time make -j -C tools/testing/selftests/nolibc nolibc-test > make: Entering directory '/k/tools/testing/selftests/nolibc' > MKDIR sysroot/x86/include > make[1]: Entering directory '/k/tools/include/nolibc' > make[2]: Entering directory '/k' > make[2]: Leaving directory '/k' > make[2]: Entering directory '/k' > INSTALL /k/tools/testing/selftests/nolibc/sysroot/sysroot/include > make[2]: Leaving directory '/k' > make[1]: Leaving directory '/k/tools/include/nolibc' > CC nolibc-test > make: Leaving directory '/k/tools/testing/selftests/nolibc' > > real 0m0.869s > user 0m0.716s > sys 0m0.149s > > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Link: https://lore.kernel.org/all/20221021155645.GK5600@paulmck-ThinkPad-P17-Gen-1/ > Signed-off-by: Willy Tarreau <w@1wt.eu> Works like a champ with reverting and unreverting c9388e0c1c6c ("tools/nolibc/string: Fix memcmp() implementation"), thank you!!! I have queued this. I expect to push this into the next merge window, thus avoiding the need to document the need for "make clean" in my pull request. ;-) Thanx, Paul > --- > tools/testing/selftests/nolibc/Makefile | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile > index 69ea659caca9..22f1e1d73fa8 100644 > --- a/tools/testing/selftests/nolibc/Makefile > +++ b/tools/testing/selftests/nolibc/Makefile > @@ -95,6 +95,7 @@ all: run > sysroot: sysroot/$(ARCH)/include > > sysroot/$(ARCH)/include: > + $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot > $(QUIET_MKDIR)mkdir -p sysroot > $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone > $(Q)mv sysroot/sysroot sysroot/$(ARCH) > @@ -133,3 +134,5 @@ clean: > $(Q)rm -rf initramfs > $(call QUIET_CLEAN, run.out) > $(Q)rm -rf run.out > + > +.PHONY: sysroot/$(ARCH)/include > -- > 2.35.3 >
On Wed, Oct 26, 2022 at 09:48:25AM -0700, Paul E. McKenney wrote: > On Wed, Oct 26, 2022 at 07:45:08AM +0200, Willy Tarreau wrote: > Works like a champ with reverting and unreverting c9388e0c1c6c > ("tools/nolibc/string: Fix memcmp() implementation"), thank you!!! Thanks for the feedback, and glad it suits your needs as well. I hope that it will progressively encourage a few of us to enhance it with more tests. > I have queued this. I expect to push this into the next merge window, > thus avoiding the need to document the need for "make clean" in my > pull request. ;-) Stupid question, is it really worth postponing it, considering that we've just introduced this series right now ? I mean, if the current usage is confusing, it's probably better to propose the fix before 6.1-final ? It's not a new feature here but rather a fix for a recently introduced one, thus I think it could be part of the next fix series. Rest assured I don't want to put a mess into your patch workflow, just asking :-) Thanks! Willy
On Wed, Oct 26, 2022 at 09:59:02PM +0200, Willy Tarreau wrote: > On Wed, Oct 26, 2022 at 09:48:25AM -0700, Paul E. McKenney wrote: > > On Wed, Oct 26, 2022 at 07:45:08AM +0200, Willy Tarreau wrote: > > Works like a champ with reverting and unreverting c9388e0c1c6c > > ("tools/nolibc/string: Fix memcmp() implementation"), thank you!!! > > Thanks for the feedback, and glad it suits your needs as well. I > hope that it will progressively encourage a few of us to enhance > it with more tests. Here is hoping! ;-) > > I have queued this. I expect to push this into the next merge window, > > thus avoiding the need to document the need for "make clean" in my > > pull request. ;-) > > Stupid question, is it really worth postponing it, considering that > we've just introduced this series right now ? I mean, if the current > usage is confusing, it's probably better to propose the fix before > 6.1-final ? It's not a new feature here but rather a fix for a recently > introduced one, thus I think it could be part of the next fix series. > Rest assured I don't want to put a mess into your patch workflow, just > asking :-) You lost me here. My intent is to push these nolicb patches into the upcoming v6.2 merge window: 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp() 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test I didn't see the problem until I queued the third patch (e1bbfe393c900), and it is still in -rcu, not in v6.1. What am I missing here? Thanx, Paul
On Wed, Oct 26, 2022 at 01:41:38PM -0700, Paul E. McKenney wrote: > > > I have queued this. I expect to push this into the next merge window, > > > thus avoiding the need to document the need for "make clean" in my > > > pull request. ;-) > > > > Stupid question, is it really worth postponing it, considering that > > we've just introduced this series right now ? I mean, if the current > > usage is confusing, it's probably better to propose the fix before > > 6.1-final ? It's not a new feature here but rather a fix for a recently > > introduced one, thus I think it could be part of the next fix series. > > Rest assured I don't want to put a mess into your patch workflow, just > > asking :-) > > You lost me here. Ah sorry! > My intent is to push these nolicb patches into the upcoming v6.2 > merge window: > > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation > e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp() > 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test > > I didn't see the problem until I queued the third patch (e1bbfe393c900), > and it is still in -rcu, not in v6.1. > > What am I missing here? I thought that since some of them are fixes, they would be pushed during 6.1-rc so that we don't release 6.1 with known defects. For example Rasmus' fix for memcmp() or the strlen() fix would IMHO make sense for this release since we're aware of the bugs and we have the fixes. The 3rd one is indeed an addition and in no way a fix and it can easily wait for 6.2. The 4th one is more of a usability fix but I agree that for this last one it's debatable, I was mostly seeing this as a possiility to avoid causing needless confusion. Hoping this clarifies my initial question. Thanks, Willy
On Thu, Oct 27, 2022 at 04:34:56AM +0200, Willy Tarreau wrote: > On Wed, Oct 26, 2022 at 01:41:38PM -0700, Paul E. McKenney wrote: > > > > I have queued this. I expect to push this into the next merge window, > > > > thus avoiding the need to document the need for "make clean" in my > > > > pull request. ;-) > > > > > > Stupid question, is it really worth postponing it, considering that > > > we've just introduced this series right now ? I mean, if the current > > > usage is confusing, it's probably better to propose the fix before > > > 6.1-final ? It's not a new feature here but rather a fix for a recently > > > introduced one, thus I think it could be part of the next fix series. > > > Rest assured I don't want to put a mess into your patch workflow, just > > > asking :-) > > > > You lost me here. > > Ah sorry! > > > My intent is to push these nolicb patches into the upcoming v6.2 > > merge window: > > > > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 > > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation > > e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp() > > 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test > > > > I didn't see the problem until I queued the third patch (e1bbfe393c900), > > and it is still in -rcu, not in v6.1. > > > > What am I missing here? > > I thought that since some of them are fixes, they would be pushed during > 6.1-rc so that we don't release 6.1 with known defects. For example Rasmus' > fix for memcmp() or the strlen() fix would IMHO make sense for this > release since we're aware of the bugs and we have the fixes. The 3rd one > is indeed an addition and in no way a fix and it can easily wait for 6.2. > The 4th one is more of a usability fix but I agree that for this last one > it's debatable, I was mostly seeing this as a possiility to avoid causing > needless confusion. > > Hoping this clarifies my initial question. Very much so, thank you! I was not considering the bug fixed by the first two patches to be serious, my mistake, apologies for my misclassification. Given that background, I would rebase these two, test them, and send off a pull request, probably early next week. 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation I would push the other two commits into the upcoming merge window. Or might the discussion between you and Rasmus result in changes to either of those first two commits? If so, I should of course wait for that discussion to resolve. Thanx, Paul
On Thu, Oct 27, 2022 at 10:04:53AM -0700, Paul E. McKenney wrote: > > > My intent is to push these nolicb patches into the upcoming v6.2 > > > merge window: > > > > > > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 > > > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation > > > e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp() > > > 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test > > > > > > I didn't see the problem until I queued the third patch (e1bbfe393c900), > > > and it is still in -rcu, not in v6.1. > > > > > > What am I missing here? > > > > I thought that since some of them are fixes, they would be pushed during > > 6.1-rc so that we don't release 6.1 with known defects. For example Rasmus' > > fix for memcmp() or the strlen() fix would IMHO make sense for this > > release since we're aware of the bugs and we have the fixes. The 3rd one > > is indeed an addition and in no way a fix and it can easily wait for 6.2. > > The 4th one is more of a usability fix but I agree that for this last one > > it's debatable, I was mostly seeing this as a possiility to avoid causing > > needless confusion. > > > > Hoping this clarifies my initial question. > > Very much so, thank you! > > I was not considering the bug fixed by the first two patches to be > serious, my mistake, apologies for my misclassification. No worries, I wasn't probably clear upfront about the purpose. > Given that background, I would rebase these two, test them, and send > off a pull request, probably early next week. > > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation Perfect, thank you! > I would push the other two commits into the upcoming merge window. OK! > Or might the discussion between you and Rasmus result in changes to > either of those first two commits? If so, I should of course wait for > that discussion to resolve. We'll see, but in any case it would just be a minor detail, but I'll give you a quick response so that you don't have to deal with multiple versions of the patch, we all know that it's painful. Thanks! Willy
On Thu, Oct 27, 2022 at 07:13:08PM +0200, Willy Tarreau wrote: > On Thu, Oct 27, 2022 at 10:04:53AM -0700, Paul E. McKenney wrote: > > > > My intent is to push these nolicb patches into the upcoming v6.2 > > > > merge window: > > > > > > > > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 > > > > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation > > > > e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp() > > > > 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test > > > > > > > > I didn't see the problem until I queued the third patch (e1bbfe393c900), > > > > and it is still in -rcu, not in v6.1. > > > > > > > > What am I missing here? > > > > > > I thought that since some of them are fixes, they would be pushed during > > > 6.1-rc so that we don't release 6.1 with known defects. For example Rasmus' > > > fix for memcmp() or the strlen() fix would IMHO make sense for this > > > release since we're aware of the bugs and we have the fixes. The 3rd one > > > is indeed an addition and in no way a fix and it can easily wait for 6.2. > > > The 4th one is more of a usability fix but I agree that for this last one > > > it's debatable, I was mostly seeing this as a possiility to avoid causing > > > needless confusion. > > > > > > Hoping this clarifies my initial question. > > > > Very much so, thank you! > > > > I was not considering the bug fixed by the first two patches to be > > serious, my mistake, apologies for my misclassification. > > No worries, I wasn't probably clear upfront about the purpose. > > > Given that background, I would rebase these two, test them, and send > > off a pull request, probably early next week. > > > > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 > > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation > > Perfect, thank you! > > > I would push the other two commits into the upcoming merge window. > > OK! > > > Or might the discussion between you and Rasmus result in changes to > > either of those first two commits? If so, I should of course wait for > > that discussion to resolve. > > We'll see, but in any case it would just be a minor detail, but I'll > give you a quick response so that you don't have to deal with multiple > versions of the patch, we all know that it's painful. If I don't hear otherwise from you by the end of tomorrow (Friday), Pacific Time, I will rebase those two patches in preparation for sending a pull request for the regression. I will of course run the pull-message text past you before sending the pull request. Thanx, Paul
Hi Paul, On Thu, Oct 27, 2022 at 11:26:29AM -0700, Paul E. McKenney wrote: > > We'll see, but in any case it would just be a minor detail, but I'll > > give you a quick response so that you don't have to deal with multiple > > versions of the patch, we all know that it's painful. > > If I don't hear otherwise from you by the end of tomorrow (Friday), > Pacific Time, I will rebase those two patches in preparation for sending > a pull request for the regression. I will of course run the pull-message > text past you before sending the pull request. No news on this front, so feel free to pick what you already have. Thank you! Willy
On Fri, Oct 28, 2022 at 09:34:05PM +0200, Willy Tarreau wrote: > Hi Paul, > > On Thu, Oct 27, 2022 at 11:26:29AM -0700, Paul E. McKenney wrote: > > > We'll see, but in any case it would just be a minor detail, but I'll > > > give you a quick response so that you don't have to deal with multiple > > > versions of the patch, we all know that it's painful. > > > > If I don't hear otherwise from you by the end of tomorrow (Friday), > > Pacific Time, I will rebase those two patches in preparation for sending > > a pull request for the regression. I will of course run the pull-message > > text past you before sending the pull request. > > No news on this front, so feel free to pick what you already have. Very good, thank you! I currently expect to send something like the pull request shown below early next week. Thoughts? Thanx, Paul ------------------------------------------------------------------------ Hello, Linus, This pull request fixes a couple of nolibc string-function bugs noted by kernel test robot, Rasmus Villemoes, Willy Tarreau. The following changes since commit 9abf2313adc1ca1b6180c508c25f22f9395cc780: Linux 6.1-rc1 (2022-10-16 15:36:24 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git tags/nolibc-urgent.2022.10.28a for you to fetch changes up to b3f4f51ea68a495f8a5956064c33dce711a2df91: tools/nolibc/string: Fix memcmp() implementation (2022-10-28 15:07:02 -0700) ---------------------------------------------------------------- Urgent nolibc pull request for v6.1 This pull request contains a couple of commits that fix string-function bugs introduced by: 96980b833a21 ("tools/nolibc/string: do not use __builtin_strlen() at -O0") 66b6f755ad45 ("rcutorture: Import a copy of nolibc") These appeared in v5.19 and v5.0, respectively, but it would be good to get these fixes in sooner rather than later. Commits providing the corresponding tests are in -rcu and I expect to submit them into the upcoming v6.2 merge window. ---------------------------------------------------------------- Rasmus Villemoes (1): tools/nolibc/string: Fix memcmp() implementation Willy Tarreau (1): tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12 tools/include/nolibc/string.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
On Fri, Oct 28, 2022 at 03:22:14PM -0700, Paul E. McKenney wrote: > I currently expect to send something like the pull request shown > below early next week. > > Thoughts? That's perfect, thank you Paul! Willy
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index 69ea659caca9..22f1e1d73fa8 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -95,6 +95,7 @@ all: run sysroot: sysroot/$(ARCH)/include sysroot/$(ARCH)/include: + $(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot $(QUIET_MKDIR)mkdir -p sysroot $(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone $(Q)mv sysroot/sysroot sysroot/$(ARCH) @@ -133,3 +134,5 @@ clean: $(Q)rm -rf initramfs $(call QUIET_CLEAN, run.out) $(Q)rm -rf run.out + +.PHONY: sysroot/$(ARCH)/include
Paul and myself got trapped a few times by not seeing the effects of applying a patch to the nolibc source code until a "make clean" was issued in the nolibc directory. It's particularly annoying when trying to confirm that a proposed patch really solves a problem (or that reverting it reintroduces the problem). The reason for the sysroot not being rebuilt was that it can be quite slow. But in fact it's only slow after a "make clean" issued at the kernel's topdir, because it's the main "make headers" that can take a tens of seconds; as long as "usr/include" still contains headers, the "headers_install" phase is only a quick "rsync", and rebuilding the whole nolibc sysroot takes a bit less than one second, which is perfectly acceptable for a test, even more once the time lost caused by misleading results if factored in. This patch marks the sysroot target as phony and starts by clearing the previous sysroot for the current architecture before reinstalling it. Thanks to this, applying a patch to nolibc makes the effect immediately visible to "make nolibc-test": $ time make -j -C tools/testing/selftests/nolibc nolibc-test make: Entering directory '/k/tools/testing/selftests/nolibc' MKDIR sysroot/x86/include make[1]: Entering directory '/k/tools/include/nolibc' make[2]: Entering directory '/k' make[2]: Leaving directory '/k' make[2]: Entering directory '/k' INSTALL /k/tools/testing/selftests/nolibc/sysroot/sysroot/include make[2]: Leaving directory '/k' make[1]: Leaving directory '/k/tools/include/nolibc' CC nolibc-test make: Leaving directory '/k/tools/testing/selftests/nolibc' real 0m0.869s user 0m0.716s sys 0m0.149s Cc: "Paul E. McKenney" <paulmck@kernel.org> Link: https://lore.kernel.org/all/20221021155645.GK5600@paulmck-ThinkPad-P17-Gen-1/ Signed-off-by: Willy Tarreau <w@1wt.eu> --- tools/testing/selftests/nolibc/Makefile | 3 +++ 1 file changed, 3 insertions(+)