Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ported Marvell armhf build on amd64 host for debian buster to use cross-comp… #501

Merged
merged 4 commits into from
Nov 18, 2021

Conversation

gregshpit
Copy link
Contributor

@gregshpit gregshpit commented Jul 1, 2021

…ilation instead of qemu emulation

Motivation:
Current armhf Sonic build on amd64 host uses qemu emulation. Due to the nature of the emulation it takes a very long time, about 22-24 hours to complete the build. The change I did to improve the building time ports Sonic armhf build on amd64 host for Marvell platform for debian buster to use cross-compilation on arm64 host for armhf target. The overall Sonic armhf building time using cross-compilation is about 6 hours.

The Sonic configure and build for the armhf cross-compilation is as following:

NOJESSIE=1 NOSTRETCH=1 BLDENV=buster CROSS_BLDENV=1 make configure PLATFORM=marvell-armhf PLATFORM_ARCH=armhf
NOJESSIE=1 NOSTRETCH=1 BLDENV=buster CROSS_BLDENV=1 make target/sonic-marvell-armhf.bin

Sonic module should check if $CROSS_BUILD_ENVIRON is 'y' to make sure that it is cross-compilation build.

@qiluo-msft
Copy link
Contributor

I cannot read "The change I did to improve the building time ports Sonic armhf build on amd64 host for Marvell platform for debian buster to use cross-compilation on arm64 host for armhf target." Could you clarify or refine the words?

What is wrong or non-optimized with original code?

@@ -29,7 +29,11 @@ AC_ARG_ENABLE(debug,
*) AC_MSG_ERROR(bad value ${enableval} for --enable-debug) ;;
esac],[debug=false])
AM_CONDITIONAL(DEBUG, test x$debug = xtrue)
AM_CONDITIONAL(ARCH64, test `getconf LONG_BIT` = "64")
if test x$CONFIGURED_ARCH = xarmhf && test x$CROSS_BUILD_ENVIRON = xy; then
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 14, 2021

Choose a reason for hiding this comment

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

x$CONFIGURED_ARCH

Is it possible to generalize this rule?
The two varaibles are host arch and target arch. I believe you can get word size from them. #WontFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

x$CONFIGURED_ARCH

Is it possible to generalize this rule?
The two varaibles are host arch and target arch. I believe you can get word size from them.

Usually you use "getconf LONG_BIT" linux command in the building environment to figure out whether the target architecture is 64 bit. However when you are using cross-compilation inside native amd64 slave image running "getconf LONG_BIT" will return info about host rather than target. The only way I see is to check directly the target architecture and whether the building environment is cross-compilation. Please suggest your solution if this is not good enough.

@gregshpit
Copy link
Contributor Author

I cannot read "The change I did to improve the building time ports Sonic armhf build on amd64 host for Marvell platform for debian buster to use cross-compilation on arm64 host for armhf target." Could you clarify or refine the words?

What is wrong or non-optimized with original code?

I'll try to explain. Currently (before my change) when you want to build Sonic for armhf on amd64 host you first create slave docker image for armhf (meaning all executable inside the docker image are of armhf architecture). And then you build Sonic inside this slave armhf (architecture) docker as if you are on the native armhf host. However you can't run armhf binary (from within the slave docker) on the amd64 host directly. You need to use qemu emulation for every bit of armhf executable. For example when you compile with gcc, this gcc is armhf gcc and to run it you need to run "qemu gcc" using emulation. Emulation is extremely slow, so it causes the entire Sonic build for armhf on amd64 host take 22-24 hours. My change basically replaces the emulation with cross-compilation that is as fast as native compilation. So using the cross-compilation make Sonic build for armhf on amd64 host to complete in about 6 hours only.
Let me know if it is clear enough.

@gregshpit
Copy link
Contributor Author

Dear reviewers,

Long time has passed since this PR was raised. Please make progress on the review.

Thanks,

Gregory

@lguohan
Copy link
Contributor

lguohan commented Sep 19, 2021

@qiluo-msft, what is blocking this pr? what do you want @gregshpit to do?

qiluo-msft
qiluo-msft previously approved these changes Sep 19, 2021
@qiluo-msft
Copy link
Contributor

Please resolve the conflict.

@gregshpit
Copy link
Contributor Author

Please resolve the conflict.

Done.

qiluo-msft
qiluo-msft previously approved these changes Sep 19, 2021
Rebased with the module master repository.
@gregshpit
Copy link
Contributor Author

Hi, please merge this PR asap. I had to make another commit to sync with the master.
Thanks,

Gregory

qiluo-msft
qiluo-msft previously approved these changes Oct 11, 2021
@qiluo-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Revert to the previous commit, discarding the last mistaken commit
@gregshpit
Copy link
Contributor Author

Hi @qiluo-msft ,

Can we finally merge my changes ? Is there anything you expect from me todo ?

Thanks,

Gregory

@qiluo-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gregshpit
Copy link
Contributor Author

HI @qiluo-msft,

As far as I understand the tests failure is not related to my changes.
The error issued is: "The job running on agent Azure Pipelines 7 ran longer than the maximum time of 60 minutes.".
If it's true please merge my changes asap.

Also I asked you already several times to answer my comments in this PR sonic-net/sonic-buildimage#8035. I need your help in resolving 3 remaining issues you asked me to fix. Please answer me asap. I can't proceed to final commit without it.

Thanks a lot,

Gregory

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 501 in repo Azure/sonic-swss-common

@liat-grozovik
Copy link
Collaborator

/azpw run Azure.sonic-swss-common

@prsunny
Copy link
Contributor

prsunny commented Nov 18, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lguohan lguohan merged commit 8c93532 into sonic-net:master Nov 18, 2021
prgeor pushed a commit to prgeor/sonic-swss-common that referenced this pull request Feb 27, 2025
…SmartSwitch (sonic-net#501)

* Modify APIs and add new APIs for supporting reboot on a SmartSwitch

* Rename dpu_name to module_name to address the review comments
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.

6 participants