| Message ID | 20240819152136.1185744-5-adrian.freihofer@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] kernel: refactor linux compression | expand |
I have been struggling to find time to sit and think through the implications of this. I have finally done that, at least a little and I have some questions below. Sorry about the delay in getting to this, I keep hoping someone else will help with this kind of review but it is very time consuming and I suspect nobody has quite the right set of knowledge to make it easier. On Mon, 2024-08-19 at 17:17 +0200, Adrian Freihofer via lists.openembedded.org wrote: > From: Adrian Freihofer <adrian.freihofer@siemens.com> > > Without this change, the kernel is always rebuilt if something changes > in the initramfs. If the kernel is rebuilt with an empty TMPDIR, the > process starts by fetching the large kernel git repository and > recompiling it from scratch. This is very inefficient. > > This cannot be improved if INITRAMFS_IMAGE_BUNDLE = "1". If the kernel > Makefile is needed to generate the initramfs bundle the kernel build > folder is required. For this cases nothing changes. When you say "kernel Makefile", I'm assuming to rebuild the kernel image, you need much more than just the Makefile but a complete kernel build tree? The Makefile is a solvable problem, the tree isn't! > But for a build configuration with INITRAMFS_IMAGE_BUNDLE = "" the > build folder is not needed. Creating the initramfs bundle requires: > linux.bin, DTBs and the initramfs. These artifacts are all available in > the deploy folder which is also sstate cached. > > Therefore do_assemble_fitimage_initramfs gets split into two tasks: > - do_assemble_fitimage_initramfs creates the bundled fitImage which runs > before do_deploy. This is without sstate and therefore the do_deploy > task still deploys the artifacts generated by this task. > - do_deploy_fitimage_unbundled creates the fitImage in unbundled mode. > This task runs after do_deploy. It uses the linux.bin artifact from > do_deploy to create the fitImage with initramfs. I'm a bit worried about having a second do_deploy_xxx task after do_deploy. This creates a recipe with two deploy tasks and whilst I can understand why you're doing it, I worry this is going to create confusion down the road. The reason I say this is that there are rules for "deploy" tasks, e.g. in meta-classes/sstate.bbclass:setscene_depvalid() has: # do_package/packagedata/package_qa/deploy don't need do_populate_sysroot if taskdependees[task][1] == "do_populate_sysroot" and taskdependees[dep][1] in ['do_package', 'do_packagedata', 'do_package_qa', 'do_deploy']: continue do_deploy_fitimage_unbundled isn't listed in SSTATETASKS. Whilst that is simple to fix, I'm not sure what side effects that is then going to cause. Also, there is this to consider: meta/classes-global/base.bbclass:do_build[recrdeptask] += "do_deploy" which is basically making sure "build" targets (the default) trigger all artefacts to be deployed. I think this new task will be missed by global code as it isn't covered there. The intent was to have one name which covered all cases, not having recipe specific entries in there. I'm starting to wonder if that unbundled case should be handled by a separate recipe, which can then have its own do_deploy task and depend on all the correct pieces. > This task is done with SSTATE_SKIP_CREATION, because this also works > with the eSDK installer (oe-selftest -r esdk is successful). It is > also more effective to quickly create the fit image instead of > handling it via sstate. Skipping sstate creation is a huge red warning sign in my mind. So far we've only had images and eSDKs using this option and I never wanted to add the option generically in the first place since it was going to lead to more exceptions like this. The implications of not creating the sstate can be problematic, for example the sstate tests then need to learn that some new subset of sstate artefacts will never be created. This also complicated the testing of eSDKs which do a similar verification which will also need exceptions. I can understand why you've done it but I am worried the side effects will be left for others (such as myself) to resolve as fit images get used in new codepaths :/. > Unfortunately, this change is not 100% backward compatible. Previously, > a dependency on do_deploy deployed all types of fit images. With this > change, deploying the unbundled fit image requires a dependency on > do_deploy_fitimage_unbundled. To address this the image.bbclass gets > improved to handle both cases with the KERNEL_DEPLOY_DEPEND variable. I think this proves my point about why adding a new deploy task is potentially problematic. Have you considered using an intermediate recipe for the unbundled case instead? I wish we could determine how much use the bundled and unbundled cases have. I'm also starting to wonder if the two cases should be separated apart into two clear workflows rather than the current entwined situation which is very hard to visualise and understand. Cheers, Richard
Hi Richard Thank you for the review. On Fri, 2024-08-23 at 14:00 +0100, Richard Purdie wrote: > I have been struggling to find time to sit and think through the > implications of this. I have finally done that, at least a little and > I > have some questions below. Sorry about the delay in getting to this, > I > keep hoping someone else will help with this kind of review but it is > very time consuming and I suspect nobody has quite the right set of > knowledge to make it easier. > > On Mon, 2024-08-19 at 17:17 +0200, Adrian Freihofer via > lists.openembedded.org wrote: > > From: Adrian Freihofer <adrian.freihofer@siemens.com> > > > > Without this change, the kernel is always rebuilt if something > > changes > > in the initramfs. If the kernel is rebuilt with an empty TMPDIR, > > the > > process starts by fetching the large kernel git repository and > > recompiling it from scratch. This is very inefficient. > > > > This cannot be improved if INITRAMFS_IMAGE_BUNDLE = "1". If the > > kernel > > Makefile is needed to generate the initramfs bundle the kernel > > build > > folder is required. For this cases nothing changes. > > When you say "kernel Makefile", I'm assuming to rebuild the kernel > image, you need much more than just the Makefile but a complete > kernel > build tree? The Makefile is a solvable problem, the tree isn't! Yes, I should have written kernel build tree. > > > But for a build configuration with INITRAMFS_IMAGE_BUNDLE = "" the > > build folder is not needed. Creating the initramfs bundle requires: > > linux.bin, DTBs and the initramfs. These artifacts are all > > available in > > the deploy folder which is also sstate cached. > > > > Therefore do_assemble_fitimage_initramfs gets split into two tasks: > > - do_assemble_fitimage_initramfs creates the bundled fitImage which > > runs > > before do_deploy. This is without sstate and therefore the > > do_deploy > > task still deploys the artifacts generated by this task. > > - do_deploy_fitimage_unbundled creates the fitImage in unbundled > > mode. > > This task runs after do_deploy. It uses the linux.bin artifact > > from > > do_deploy to create the fitImage with initramfs. > > I'm a bit worried about having a second do_deploy_xxx task after > do_deploy. This creates a recipe with two deploy tasks and whilst I > can > understand why you're doing it, I worry this is going to create > confusion down the road. > > The reason I say this is that there are rules for "deploy" tasks, > e.g. > in meta-classes/sstate.bbclass:setscene_depvalid() has: > > # do_package/packagedata/package_qa/deploy don't need > do_populate_sysroot > if taskdependees[task][1] == "do_populate_sysroot" and > taskdependees[dep][1] in ['do_package', 'do_packagedata', > 'do_package_qa', 'do_deploy']: > continue > > do_deploy_fitimage_unbundled isn't listed in SSTATETASKS. Whilst that Thank you for the hint. I don't know how this could have been overlooked until now. > is simple to fix, I'm not sure what side effects that is then going > to > cause. > > Also, there is this to consider: > > meta/classes-global/base.bbclass:do_build[recrdeptask] += "do_deploy" > > which is basically making sure "build" targets (the default) trigger > all artefacts to be deployed. I think this new task will be missed by > global code as it isn't covered there. The intent was to have one > name > which covered all cases, not having recipe specific entries in there. > > I'm starting to wonder if that unbundled case should be handled by a > separate recipe, which can then have its own do_deploy task and > depend > on all the correct pieces. > > > > This task is done with SSTATE_SKIP_CREATION, because this also > > works > > with the eSDK installer (oe-selftest -r esdk is successful). It > > is > > also more effective to quickly create the fit image instead of > > handling it via sstate. > > Skipping sstate creation is a huge red warning sign in my mind. So > far > we've only had images and eSDKs using this option and I never wanted > to > add the option generically in the first place since it was going to > lead to more exceptions like this. > > The implications of not creating the sstate can be problematic, for > example the sstate tests then need to learn that some new subset of > sstate artefacts will never be created. This also complicated the > testing of eSDKs which do a similar verification which will also need > exceptions. I can understand why you've done it but I am worried the > side effects will be left for others (such as myself) to resolve as > fit > images get used in new codepaths :/. > > > Unfortunately, this change is not 100% backward compatible. > > Previously, > > a dependency on do_deploy deployed all types of fit images. With > > this > > change, deploying the unbundled fit image requires a dependency on > > do_deploy_fitimage_unbundled. To address this the image.bbclass > > gets > > improved to handle both cases with the KERNEL_DEPLOY_DEPEND > > variable. > > I think this proves my point about why adding a new deploy task is > potentially problematic. Have you considered using an intermediate > recipe for the unbundled case instead? Yes, I have thought about and tried 3 different implementations: - This one here which is supposed to be backward compatible. - Renaming the sstated kernel do_deploy task to e.g. do_deploy_stage1. Then bring the do_deploy task back as an empty task depending on the do_deploy_stage1. This looks equal to the current implementation but allows to inject the do_deploy_fitimage_unbundled task from the kernel- fitimage.bbclass. However, this has similar side effects. For example the eSDK assumes that the do_deploy task is the last sstated task in the chain which provides the relevant sstate artifact. - Add a separate recipe for the unbundled fitImage. This could simplify the implementation, but is probably somewhat cumbersome from the user's point of view. Bundled fitImage remains in the kernel but unbundled needs dealing with different recipes. > > I wish we could determine how much use the bundled and unbundled > cases > have. I'm also starting to wonder if the two cases should be > separated > apart into two clear workflows rather than the current entwined > situation which is very hard to visualise and understand. Yes, that's probably the conclusion so far. The kernel recipes have reached a very complex state. Especially the implementation of the unbundled fitImage support is a bit strange and adds complexity that could possibly be simplified by splitting it into a separate recipe. I will try to come up with a different implementation where the undbundled fitImage is created by a dedicated recipe. This recipe will basically have the do_deploy_fitimage_unbundled implementation as its do_deploy task which depends on the kernel do_deploy. Not sure if such a recipe could also provide a package for the fitImage kernel. Currently the fitImage is deployed only. Other kernel image types are packaged and deployed. The challenge will then probably be to hide the extra complexity of 2 recipes as good as possible and to refactor the tests accordingly. Kind regards, Adrian > > Cheers, > > Richard
diff --git a/meta/classes-recipe/image.bbclass b/meta/classes-recipe/image.bbclass index 32bafcdbbc3..f5754fb6fd9 100644 --- a/meta/classes-recipe/image.bbclass +++ b/meta/classes-recipe/image.bbclass @@ -147,7 +147,17 @@ do_rootfs[vardeps] += "${@rootfs_variables(d)}" # This follows many common usecases and user expectations. # But if you are building an image which doesn't need the kernel image at all, # you can unset this variable manually. -KERNEL_DEPLOY_DEPEND ?= "virtual/kernel:do_deploy" +def get_kernel_do_deploy(d): + kerneltypes = d.getVar('KERNEL_IMAGETYPES') or "" + kerneltype = d.getVar('KERNEL_IMAGETYPE') or "" + fitimage = kerneltype == 'fitImage' or 'fitImage' in kerneltypes.split() + initramfs_image = d.getVar('INITRAMFS_IMAGE') or "" + bundled = d.getVar('INITRAMFS_IMAGE_BUNDLE') == '1' + if fitimage and initramfs_image and not bundled: + return "virtual/kernel:do_deploy_fitimage_unbundled" + return "virtual/kernel:do_deploy" + +KERNEL_DEPLOY_DEPEND ?= "${@get_kernel_do_deploy(d)}" do_build[depends] += "${KERNEL_DEPLOY_DEPEND}" diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass index 2dda5aec056..66f7bc35501 100644 --- a/meta/classes-recipe/kernel-fitimage.bbclass +++ b/meta/classes-recipe/kernel-fitimage.bbclass @@ -49,18 +49,28 @@ python __anonymous () { d.appendVarFlag('do_assemble_fitimage', 'depends', ' u-boot-tools-native:do_populate_sysroot dtc-native:do_populate_sysroot') + # A fitImage with the initramfs bundled in the kernel Image is deployed by do_deploy. + # In case of a build from an empty TMPDIR a clean kernel re-build is required just because creating the bundled + # kernel image requires the kernel Makefile and therefore the kernel's populated build directory. + # In case of an unbundled fitImage assembling the fitImage works independenly from the kernel build framework. + # This allow to take the kernel binary from the sstated deploy directory. But it requires to assemble the + # fitImage in a separate task (do_deploy_fitimage_unbundled) running after do_deploy. initramfs_image = d.getVar('INITRAMFS_IMAGE') bundled = bb.utils.to_boolean(d.getVar('INITRAMFS_IMAGE_BUNDLE')) if initramfs_image: if bundled: - bb.build.addtask('do_assemble_fitimage_initramfs', 'do_deploy', 'do_bundle_initramfs', d) + fit_assemble_task = 'do_assemble_fitimage_initramfs' + bb.build.addtask(fit_assemble_task, 'do_deploy', 'do_bundle_initramfs', d) else: - bb.build.addtask('do_assemble_fitimage_initramfs', 'do_deploy', 'do_install', d) + fit_assemble_task = 'do_deploy_fitimage_unbundled' + bb.build.addtask(fit_assemble_task, 'do_build', 'do_deploy', d) + bb.build.addtask(fit_assemble_task + '_setscene', '', '', d) + d.appendVar('SSTATETASKS', ' ' + fit_assemble_task) - d.appendVarFlag('do_assemble_fitimage_initramfs', 'depends', + d.appendVarFlag(fit_assemble_task, 'depends', ' u-boot-tools-native:do_populate_sysroot dtc-native:do_populate_sysroot ${INITRAMFS_IMAGE}:do_image_complete') if providerdtb: - d.appendVarFlag('do_assemble_fitimage_initramfs', 'depends', ' virtual/dtb:do_populate_sysroot') + d.appendVarFlag(fit_assemble_task, 'depends', ' virtual/dtb:do_populate_sysroot') } @@ -579,7 +589,7 @@ fitimage_assemble() { setupcount="" bootscr_id="" default_dtb_image="" - rm -f "$1" "arch/${ARCH}/boot/$(basename $2)" + rm -f "$1" "$2" if [ -n "${UBOOT_SIGN_IMG_KEYNAME}" -a "${UBOOT_SIGN_KEYNAME}" = "${UBOOT_SIGN_IMG_KEYNAME}" ]; then bbfatal "Keys used to sign images and configuration nodes must be different." @@ -808,15 +818,30 @@ do_install:append() { } do_assemble_fitimage_initramfs() { - if [ "${INITRAMFS_IMAGE_BUNDLE}" = "1" ]; then - fitimage_assemble "fit-image-${INITRAMFS_IMAGE}.its" "${KERNEL_OUTPUT_DIR}/fitImage-bundle" "" - ln -sf fitImage-bundle "${B}/${KERNEL_OUTPUT_DIR}/fitImage" - else - fitimage_assemble "fit-image-${INITRAMFS_IMAGE}.its" "${KERNEL_OUTPUT_DIR}/fitImage-${INITRAMFS_IMAGE}" 1 - fi + fitimage_assemble "fit-image-${INITRAMFS_IMAGE}.its" "${KERNEL_OUTPUT_DIR}/fitImage-bundle" "" + ln -sf fitImage-bundle "${B}/${KERNEL_OUTPUT_DIR}/fitImage" } do_assemble_fitimage_initramfs[dirs] = "${B}" +do_deploy_fitimage_unbundled() { + fitimage_assemble "${DEPLOY_DIR_IMAGE}/fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}.its"\ + "${DEPLOY_DIR_IMAGE}/fitImage-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}${KERNEL_FIT_BIN_EXT}" 1 + if [ -n "${KERNEL_FIT_LINK_NAME}" ] ; then + ln -snf "fitImage-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}${KERNEL_FIT_BIN_EXT}" "${DEPLOY_DIR_IMAGE}/fitImage-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_LINK_NAME}" + ln -snf fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}.its "${DEPLOY_DIR_IMAGE}/fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_LINK_NAME}" + fi +} +DEPLOYDIR_FITIMAGE_UNBUNDLED = "${WORKDIR}/deploy-fitimage-unbundled-${PN}" +SSTATE_SKIP_CREATION:task-deploy-fitimage-unbundled = '1' +do_deploy_fitimage_unbundled[sstate-inputdirs] = "${DEPLOYDIR_FITIMAGE_UNBUNDLED}" +do_deploy_fitimage_unbundled[sstate-outputdirs] = "${DEPLOY_DIR_IMAGE}" +python do_deploy_fitimage_unbundled_setscene () { + sstate_setscene(d) +} +do_deploy_fitimage_unbundled[dirs] = "${DEPLOY_DIR_IMAGE}" +do_deploy_fitimage_unbundled[cleandirs] = "${DEPLOYDIR_FITIMAGE_UNBUNDLED}" +do_deploy_fitimage_unbundled[stamp-extra-info] = "${MACHINE_ARCH}" + do_kernel_generate_rsa_keys() { if [ "${UBOOT_SIGN_ENABLE}" = "0" ] && [ "${FIT_GENERATE_KEYS}" = "1" ]; then bbwarn "FIT_GENERATE_KEYS is set to 1 even though UBOOT_SIGN_ENABLE is set to 0. The keys will not be generated as they won't be used." @@ -868,9 +893,24 @@ kernel_do_deploy[vardepsexclude] = "DATETIME" kernel_do_deploy:append() { # Update deploy directory if echo ${KERNEL_IMAGETYPES} | grep -wq "fitImage"; then - - if [ "${INITRAMFS_IMAGE_BUNDLE}" != "1" ]; then - # deploy the artifacts of do_assemble_fitimage + if [ -n "${INITRAMFS_IMAGE}" ] && [ "${INITRAMFS_IMAGE_BUNDLE}" != "1" ]; then + # do_deploy_fitimage_unbundled needs the linux.bin file for the unbundled fitImage deployment + bbnote "Deploying linux.bin and linux.comp file for do_deploy_fitimage_unbundled..." + uboot_prep_kimage + install -m 0644 ${B}/linux.bin $deployDir/linux.bin + install -m 0644 ${B}/linux.comp $deployDir/linux.comp + if [ -e "${B}/${KERNEL_OUTPUT_DIR}/setup.bin" ]; then + install -m 0644 "${B}/${KERNEL_OUTPUT_DIR}/setup.bin" "$deployDir/setup.bin" + fi + elif [ -n "${INITRAMFS_IMAGE}" ]; then + # deploy the artifacts created by do_assemble_fitimage_initramfs for bundled mode + bbnote "Copying fit-image-${INITRAMFS_IMAGE}.its source file..." + install -m 0644 ${B}/fit-image-${INITRAMFS_IMAGE}.its "$deployDir/fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}.its" + if [ -n "${KERNEL_FIT_LINK_NAME}" ] ; then + ln -snf fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}.its "$deployDir/fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_LINK_NAME}" + fi + else + # deploy the artifacts of do_assemble_fitimage (fitImage without initramfs) bbnote "Copying fit-image.its source file..." install -m 0644 ${B}/fit-image.its "$deployDir/fitImage-its-${KERNEL_FIT_NAME}.its" if [ -n "${KERNEL_FIT_LINK_NAME}" ] ; then @@ -883,23 +923,5 @@ kernel_do_deploy:append() { ln -snf fitImage-linux.bin-${KERNEL_FIT_NAME}${KERNEL_FIT_BIN_EXT} "$deployDir/fitImage-linux.bin-${KERNEL_FIT_LINK_NAME}" fi fi - - if [ -n "${INITRAMFS_IMAGE}" ]; then - # deploy the artifacts of do_assemble_fitimage_initramfs for bundled as well as un-bundled mode - bbnote "Copying fit-image-${INITRAMFS_IMAGE}.its source file..." - install -m 0644 ${B}/fit-image-${INITRAMFS_IMAGE}.its "$deployDir/fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}.its" - if [ -n "${KERNEL_FIT_LINK_NAME}" ] ; then - ln -snf fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}.its "$deployDir/fitImage-its-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_LINK_NAME}" - fi - - # deploy the artifacts of do_assemble_fitimage_initramfs for bundled mode only - if [ "${INITRAMFS_IMAGE_BUNDLE}" != "1" ]; then - bbnote "Copying fitImage-${INITRAMFS_IMAGE} file..." - install -m 0644 ${B}/${KERNEL_OUTPUT_DIR}/fitImage-${INITRAMFS_IMAGE} "$deployDir/fitImage-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}${KERNEL_FIT_BIN_EXT}" - if [ -n "${KERNEL_FIT_LINK_NAME}" ] ; then - ln -snf fitImage-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_NAME}${KERNEL_FIT_BIN_EXT} "$deployDir/fitImage-${INITRAMFS_IMAGE_NAME}-${KERNEL_FIT_LINK_NAME}" - fi - fi - fi fi } diff --git a/meta/classes-recipe/kernel.bbclass b/meta/classes-recipe/kernel.bbclass index 217f42b4f4f..9c125ff0731 100644 --- a/meta/classes-recipe/kernel.bbclass +++ b/meta/classes-recipe/kernel.bbclass @@ -490,7 +490,8 @@ kernel_do_install() { # So, at the level of the install task we should not try to install the fitImage. fitImage is still not # generated yet. # After the generation of the fitImage, the deploy task copies the fitImage from the build directory to - # the deploy folder. + # the deploy folder. If INITRAMFS_IMAGE_BUNDLE != 1 the fitImage with initramfs is deployed after the + # deploy task. # for imageType in ${KERNEL_IMAGETYPES} ; do