Skip to content

iperf3-mt: new package#22516

Merged
BKPepe merged 1 commit intoopenwrt:masterfrom
jonasjelonek:iperf3-mt
Nov 4, 2023
Merged

iperf3-mt: new package#22516
BKPepe merged 1 commit intoopenwrt:masterfrom
jonasjelonek:iperf3-mt

Conversation

@jonasjelonek
Copy link
Contributor

@jonasjelonek jonasjelonek commented Oct 26, 2023

This adds a multithreaded variant of iperf3 as a package. This variant is still experimental, developed in the mt branch of the iperf repository and expected to be merged when it is considered stable.

Maintainer: me / @jonasjelonek
Compile tested: aarch64_cortex-a53 (BananaPi R64) + x86/64 (APU2C4), OpenWrt snapshot
Run tested: aarch64_cortex-a53 (BananaPi R64) + x86/64 (APU2C4), OpenWrt snapshot

Description:
The main iperf3 that we already have as a package, has no support for multithreading yet. However, the developers are working already on that in the mt branch of the iperf Github repository and release this as 3.XX-mt versions. This multithreaded iperf3 is still considered experimental, however, in my tests I couldn't notice any instabilities so far and the load is distributed across multiple CPU cores when using multiple streams.

Starting point for this Makefile is the one of the normal iperf3. I switched to Github codeload because the developers only publish non-beta versions of the multithreaded variant in the other one. This PR starts with 3.15-mt-beta1, the latest non-beta published in the location that the iperf3 package uses is 3.13-mt1. I think we can go with this beta to not lack behind the non-MT version.

@jonasjelonek
Copy link
Contributor Author

jonasjelonek commented Oct 26, 2023

cc @PolynomialDivision @thuehn

@BKPepe
Copy link
Member

BKPepe commented Oct 27, 2023

This one fails for PowerPC and also for mips (same error):

OpenWrt-libtool: link: powerpc-openwrt-linux-musl-gcc -g -Os -pipe -mcpu=464fp -fno-caller-saves -fno-plt -fhonour-copts -ffile-prefix-map=/builder/build_dir/target-powerpc_464fp_musl/iperf-nossl/iperf-iperf-3.15-mt-beta1=iperf-iperf-3.15-mt-beta1 -Wformat -Werror=format-security -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z -Wl,now -Wl,-z -Wl,relro -D_GNU_SOURCE -Wall -g -fuse-ld=bfd -znow -zrelro -o .libs/iperf3 iperf3-main.o  -L/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/usr/lib -L/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/lib ./.libs/libiperf.so -lpthread -pthread
/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/bin/../lib/gcc/powerpc-openwrt-linux-musl/12.3.0/../../../../powerpc-openwrt-linux-musl/bin/ld.bfd: ./.libs/libiperf.so: undefined reference to `__atomic_store_8'
/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/bin/../lib/gcc/powerpc-openwrt-linux-musl/12.3.0/../../../../powerpc-openwrt-linux-musl/bin/ld.bfd: ./.libs/libiperf.so: undefined reference to `__atomic_fetch_add_8'
/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/bin/../lib/gcc/powerpc-openwrt-linux-musl/12.3.0/../../../../powerpc-openwrt-linux-musl/bin/ld.bfd: ./.libs/libiperf.so: undefined reference to `__atomic_load_8'
collect2: error: ld returned 1 exit status

@jonasjelonek
Copy link
Contributor Author

This one fails for PowerPC and also for mips (same error):

OpenWrt-libtool: link: powerpc-openwrt-linux-musl-gcc -g -Os -pipe -mcpu=464fp -fno-caller-saves -fno-plt -fhonour-copts -ffile-prefix-map=/builder/build_dir/target-powerpc_464fp_musl/iperf-nossl/iperf-iperf-3.15-mt-beta1=iperf-iperf-3.15-mt-beta1 -Wformat -Werror=format-security -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z -Wl,now -Wl,-z -Wl,relro -D_GNU_SOURCE -Wall -g -fuse-ld=bfd -znow -zrelro -o .libs/iperf3 iperf3-main.o  -L/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/usr/lib -L/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/lib ./.libs/libiperf.so -lpthread -pthread
/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/bin/../lib/gcc/powerpc-openwrt-linux-musl/12.3.0/../../../../powerpc-openwrt-linux-musl/bin/ld.bfd: ./.libs/libiperf.so: undefined reference to `__atomic_store_8'
/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/bin/../lib/gcc/powerpc-openwrt-linux-musl/12.3.0/../../../../powerpc-openwrt-linux-musl/bin/ld.bfd: ./.libs/libiperf.so: undefined reference to `__atomic_fetch_add_8'
/builder/staging_dir/toolchain-powerpc_464fp_gcc-12.3.0_musl/bin/../lib/gcc/powerpc-openwrt-linux-musl/12.3.0/../../../../powerpc-openwrt-linux-musl/bin/ld.bfd: ./.libs/libiperf.so: undefined reference to `__atomic_load_8'
collect2: error: ld returned 1 exit status

Yes, I already have a fix for this issue, libatomic is missing in TARGET_LDFLAGS and as dependency for libiperf3-mt. Will force-push this soon.

@jonasjelonek
Copy link
Contributor Author

jonasjelonek commented Oct 27, 2023

@BKPepe
CI fails again although it works locally. I suspect this to be caused by an issue I already noticed with the normal iperf3.

When the ssl variant is selected, --disable-shared is added to the CONFIGURE_ARGS. When libiperf3 is selected at the same time, this seems to prevent the creation of libiperf3.so.*, causing the build to fail with the error libiperf.so.* not found.

Can't we remove this--disable-shared?. As far as I understand this was introduced for iperf3 to avoid shipping two libiperf.so.* versions. Because I added the conflict between iperf3-mt and iperf3-mt-ssl as per your review, isn't that two-versions problem solved already?

EDIT: I tried removing that --disable-shared, it leads to missing dependencies libcrypto and libssl for libiperf then. Adding libopenssl as a fixed dependency to libiperf3-mt doesn't seem like a good solution to me. Could be solved by having a separate package like libiperf3-mt-ssl, but I guess that was the original intention of statically compiling the ssl variant.
Or is there a way to add libopenssl as a dependency to libiperf3-mt only in case the ssl variant is built?

This adds a multithreaded variant of iperf3 as a package. This variant
is still experimental, developed in the mt branch of the iperf
repository and expected to be merged when it is considered stable.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
@jonasjelonek
Copy link
Contributor Author

jonasjelonek commented Oct 27, 2023

Nevermind my message suspecting the issue to cause CI fail , succeeds now. Another DEPENDS on libatomic was missing.

However the issue I described is still present, and comes up when previously iperf3(-mt) was selected, but then one deselects it, selects iperf3(-mt)-ssl and does not deselect libiperf3(-mt).

Should I account for this here or in a separate PR for both iperf3 and iperf3-mt?

@jonasjelonek jonasjelonek requested a review from BKPepe October 29, 2023 09:17
@BKPepe
Copy link
Member

BKPepe commented Oct 30, 2023

When the ssl variant is selected, --disable-shared is added to the CONFIGURE_ARGS. When libiperf3 is selected at the same time, this seems to prevent the creation of libiperf3.so., causing the build to fail with the error libiperf.so. not found.

I believe that this same issue is already present in iperf3. For now, I propose to create issue in this repository about it. So we would not forget about it and someone including me can look at it. I am not able to look at it now, sorry! I am quite busy with other things.

Comment on lines +89 to +95
define Package/iperf3-mt/install/Default
$(INSTALL_DIR) $(1)/usr/bin
$(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/bin/iperf3 $(1)/usr/bin/
endef

Package/iperf3-mt/install = $(Package/iperf3-mt/install/Default)
Package/iperf3-mt-ssl/install = $(Package/iperf3-mt/install/Default)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking about this. It might be nitpicking and I know the intention, why it is done like this, but wouldn't it be better to do it like it is done in iperf3? I mean, we will add one more row, but otherwise it might be better to read.

My proposal:

define Package/iperf3-mt/install
	$(INSTALL_DIR) $(1)/usr/bin
	$(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/bin/iperf3 $(1)/usr/bin/
endef

define Package/iperf3-mt-ssl/install
	$(INSTALL_DIR) $(1)/usr/bin
	$(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/bin/iperf3 $(1)/usr/bin/
endef

Nothing wrong about this, if you dont want it, I am fine with that.

Copy link
Contributor Author

@jonasjelonek jonasjelonek Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no clear opinion on this.
I saw that in the mtr Makefile, and it looked a bit cleaner to me, but I guess that's subjective. :)

Copy link
Member

@BKPepe BKPepe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we reached the end here. 👍 Thanks. If there are no objections within 48hours, I will merge this. This looks good.

@diizzyy
Copy link
Contributor

diizzyy commented Nov 4, 2023

While I haven't done any benchmarking at all I would guess that it makes sense to force -O2 in this case if it improves performance that is.

@jonasjelonek
Copy link
Contributor Author

jonasjelonek commented Nov 4, 2023

While I haven't done any benchmarking at all I would guess that it makes sense to force -O2 in this case if it improves performance that is.

I haven't done either so far. Probably this can be discussed in a separate PR, if I understand this correctly it could also be discussed for the normal iperf3.

@jonasjelonek
Copy link
Contributor Author

ping @BKPepe

@BKPepe BKPepe merged commit f369a2a into openwrt:master Nov 4, 2023
@jonasjelonek jonasjelonek deleted the iperf3-mt branch November 4, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants