| Message ID | 20201204095703.4714-1-patrickdepinguin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [PATCHv2,1/2] package/zstd: build multithreaded library if supported | expand |
Hello Thomas, On Fri, 4 Dec 2020 10:57:01 +0100 Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > libzstd.so is built without multi-threading support by default. > The 'HAVE_THREAD' flag is not respected by lib/Makefile, only by > programs/Makefile. > > Use the %-mt recipe in lib/Makefile to enable multithreading. > > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> Your patch is fixing this for the target variant of zstd, but to me it seems like the host variant also builds the library and programs separately, so it is probably affected by the same issue, isn't it ? Also, in your PATCH 2/2 you mention that zstd also supports cmake and meson. Would switching to cmake-package or meson-package simplify a bit this issue and generally what zstd.mk looks like ? Thanks! Thomas
Hi Thomas, El sáb, 2 ene 2021 a las 18:49, Thomas Petazzoni (<thomas.petazzoni@bootlin.com>) escribió: > > Hello Thomas, > > On Fri, 4 Dec 2020 10:57:01 +0100 > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > > > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > > > libzstd.so is built without multi-threading support by default. > > The 'HAVE_THREAD' flag is not respected by lib/Makefile, only by > > programs/Makefile. > > > > Use the %-mt recipe in lib/Makefile to enable multithreading. > > > > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > Your patch is fixing this for the target variant of zstd, but to me it > seems like the host variant also builds the library and programs > separately, so it is probably affected by the same issue, isn't it ? The main driver for this series was the ever-expanding size of zstd, worsened by the fact that the binary was using static linking. For host, there is generally no size restriction, at least not of this order. I'm not sure about host-mtd, and host-squashfs, but for the host-zstd command-line tool itself, it is important that the binary and library are either both built for single-threaded or both for multi-threaded. This means that we'd have to pass HAVE_THREAD=1 for the host compilation of zstd as well. Probably possible, not sure why it is not done today. > > Also, in your PATCH 2/2 you mention that zstd also supports cmake and > meson. Would switching to cmake-package or meson-package simplify a bit > this issue and generally what zstd.mk looks like ? The developers mention in certain issues that 'make' is the official build system, and that the other ones (like cmake and meson) are 'community supported'. So while their make logic is not clean at all, I would not necessarily trust non-official solutions better. Meanwhile, a FreeBSD developer proposed an alternative patch on the corresponding issue: https://github.com/facebook/zstd/issues/2261#issuecomment-753632465 So I suggest to hold this patch for a while, I'll try to get it moving upstream. Thanks, Thomas > > Thanks! > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hello, On Tue, 5 Jan 2021 17:20:39 +0100 Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > > Your patch is fixing this for the target variant of zstd, but to me it > > seems like the host variant also builds the library and programs > > separately, so it is probably affected by the same issue, isn't it ? > > The main driver for this series was the ever-expanding size of zstd, > worsened by the fact that the binary was using static linking. > For host, there is generally no size restriction, at least not of this order. Right, but my point was not about size here but correctness/functionality. > I'm not sure about host-mtd, and host-squashfs, but for the host-zstd > command-line tool itself, it is important that the binary and library > are either both built for single-threaded or both for multi-threaded. > This means that we'd have to pass HAVE_THREAD=1 for the host > compilation of zstd as well. Probably possible, not sure why it is not > done today. This is the point I missed: the host variant is not built with any HAVE_THREAD= value, so I suppose it's built single-threaded, which is why it works fine as it is today. So your patch is correct in fixing just the target package to use the %-mt make targets, because only the target package is built with thread support when available. Optionally, we could have a separate patch that turns on building with HAVE_THREAD=1 for the host package, but it's a separate matter. Makes sense. > > Also, in your PATCH 2/2 you mention that zstd also supports cmake and > > meson. Would switching to cmake-package or meson-package simplify a bit > > this issue and generally what zstd.mk looks like ? > > The developers mention in certain issues that 'make' is the official > build system, and that the other ones (like cmake and meson) are > 'community supported'. > So while their make logic is not clean at all, I would not necessarily > trust non-official solutions better. > > Meanwhile, a FreeBSD developer proposed an alternative patch on the > corresponding issue: > https://github.com/facebook/zstd/issues/2261#issuecomment-753632465 > > So I suggest to hold this patch for a while, I'll try to get it moving upstream. OK. This is for PATCH 2/2, right ? Thomas
On Tue, Jan 5, 2021, 17:36 Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > On Tue, 5 Jan 2021 17:20:39 +0100 > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > > > > Your patch is fixing this for the target variant of zstd, but to me it > > > seems like the host variant also builds the library and programs > > > separately, so it is probably affected by the same issue, isn't it ? > > > > The main driver for this series was the ever-expanding size of zstd, > > worsened by the fact that the binary was using static linking. > > For host, there is generally no size restriction, at least not of this > order. > > Right, but my point was not about size here but > correctness/functionality. > > > I'm not sure about host-mtd, and host-squashfs, but for the host-zstd > > command-line tool itself, it is important that the binary and library > > are either both built for single-threaded or both for multi-threaded. > > This means that we'd have to pass HAVE_THREAD=1 for the host > > compilation of zstd as well. Probably possible, not sure why it is not > > done today. > > This is the point I missed: the host variant is not built with any > HAVE_THREAD= value, so I suppose it's built single-threaded, which is > why it works fine as it is today. > > So your patch is correct in fixing just the target package to use the > %-mt make targets, because only the target package is built with thread > support when available. > > Optionally, we could have a separate patch that turns on building with > HAVE_THREAD=1 for the host package, but it's a separate matter. > > Makes sense. > > > > Also, in your PATCH 2/2 you mention that zstd also supports cmake and > > > meson. Would switching to cmake-package or meson-package simplify a bit > > > this issue and generally what zstd.mk looks like ? > > > > The developers mention in certain issues that 'make' is the official > > build system, and that the other ones (like cmake and meson) are > > 'community supported'. > > So while their make logic is not clean at all, I would not necessarily > > trust non-official solutions better. > > > > Meanwhile, a FreeBSD developer proposed an alternative patch on the > > corresponding issue: > > https://github.com/facebook/zstd/issues/2261#issuecomment-753632465 > > > > So I suggest to hold this patch for a while, I'll try to get it moving > upstream. > > OK. This is for PATCH 2/2, right ? > Yes, sorry for the confusion. Patch 1 can proceed, and patch 2 to be held. Thanks, Thomas
On Fri, 4 Dec 2020 10:57:01 +0100 Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > libzstd.so is built without multi-threading support by default. > The 'HAVE_THREAD' flag is not respected by lib/Makefile, only by > programs/Makefile. > > Use the %-mt recipe in lib/Makefile to enable multithreading. > > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > --- > v2: split off of next patch Applied to master, thanks. It would be good to have a patch that enables the multi-threading for the host variant of zstd. Thomas
Hi Thomas, El dom, 17 ene 2021 a las 15:30, Thomas Petazzoni (<thomas.petazzoni@bootlin.com>) escribió: > > On Fri, 4 Dec 2020 10:57:01 +0100 > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > > > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > > > libzstd.so is built without multi-threading support by default. > > The 'HAVE_THREAD' flag is not respected by lib/Makefile, only by > > programs/Makefile. > > > > Use the %-mt recipe in lib/Makefile to enable multithreading. > > > > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > --- > > v2: split off of next patch > > Applied to master, thanks. It would be good to have a patch that > enables the multi-threading for the host variant of zstd. > Thanks, I just sent such patch for host-zstd. I learned that upstream has added a clean way of building the zstd cli programs against its own libzstd, currently in its 'dev' branch (not yet released). This will provide an alternative implementation than my 2/2 patch. With that new 'zstd-dll' target we can link both target and host zstd dynamically. I have the Buildroot changes ready, but am waiting for a new release of zstd (1.4.9 or 1.5.x ?). Best regards, Thomas
diff --git a/package/zstd/zstd.mk b/package/zstd/zstd.mk index 35002da332..37c1a8d687 100644 --- a/package/zstd/zstd.mk +++ b/package/zstd/zstd.mk @@ -48,6 +48,14 @@ ZSTD_BUILD_LIBS = libzstd.a libzstd ZSTD_INSTALL_LIBS = install-static install-shared endif +# The HAVE_THREAD flag is read by the 'programs' makefile but not by the 'lib' +# one. Building a multi-threaded binary with a library (which defaults to +# single-threaded) gives a runtime error when compressing files. +# The 'lib' makefile provides specific '%-mt' targets for this purpose. +ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y) +ZSTD_BUILD_LIBS := $(addsuffix -mt,$(ZSTD_BUILD_LIBS)) +endif + define ZSTD_BUILD_CMDS $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) $(ZSTD_OPTS) \ -C $(@D)/lib $(ZSTD_BUILD_LIBS)