Hello Kairui, Thanks for the patch. Can you please list the changes in v2 (as compared to v1) in the commit message. On Wed, Jul 25, 2018 at 4:25 PM, Kairui Song <kasong(a)redhat.com&gt; wrote: > Currently, we only rebuilt kdump initramfs on config file change, > fs change, or watchdog related change. This will not cover the case > that hardware changed but fs layout and other configurations still > stays the same, and kdump may fail. Hmm. This is a chicken and egg issue. We can have modules being unloaded (waiting for dependencies or timeouts to be met) or loaded while we executing 'lsmod' via "kdumpctl restart" , then we will miss those modules being unloaded/loaded.

Can we look at ways to determine the same as well or at least warn the user that this is probably not the final/correct representation of the modules being unloaded/loaded when 'kdumpctl restart' is being executed. Thanks, Bhupesh > To cover such case, we can detect and compare loaded kernel modules, > if a hardware change requires the image to be rebuilt, loaded kernel > modules must have changed. > > Starting from commit 7047294 dracut will record loaded kernel modules > when the image is built if hostonly mode is enabled. With this patch, > kdumpctl will compare the recorded value with currently loaded kernel > modules, and rebuild the image on change. > > "kdumpctl start" will be a bit slower, as we have to call lsinitrd one > more time to get the loaded kernel modules list. I measure the time > consumption and we have an overall 0.2s increased loading time. > > Time consumption of command "kdumpctl restart": > > Before: > real 0m0.587s > user 0m0.481s > sys 0m0.102s > > After: > real 0m0.731s > user 0m0.591s > sys 0m0.133s > > Time comsumption of command "kdumpctl restart" with image rebuild: > > Before (force rebuild): > real 0m10.972s > user 0m8.966s > sys 0m1.318s > > After (inserted ~100 new modules): > real 0m11.220s > user 0m9.387s > sys 0m1.337s > > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt; > --- > kdumpctl | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/kdumpctl b/kdumpctl > index 13b8209..6a01c13 100755 > --- a/kdumpctl > +++ b/kdumpctl > @@ -458,6 +458,31 @@ check_wdt_modified() > return 1 > } > > +check_kmodules_modified() > +{ > + # always sort again to avoid LANG/LC inconsistent problem > + local _old_modules="$(lsinitrd $TARGET_INITRD -f /usr/lib/dracut/loaded-kernel-modules.txt | sort)" > + local _new_modules="$(get_loaded_kernel_modules | sort)" > + > + [[ -z $_old_modules ]] && echo "Warning: Previous loaded kernel module list is absent or empty" > + > + local _added_modules=$(comm -13 <(echo "$_old_modules") <(echo "$_new_modules")) > + local _dropped_modules=$(comm -23 <(echo "$_old_modules") <(echo "$_new_modules")) > + > + if [ "$_old_modules" != "$_new_modules" ]; then > + echo "Detected change(s) of loaded kernel modules list:" > + [[ -n $_added_modules ]] && for _module in $_added_modules; do > + echo " +$_module" > + done > + [[ -n $_dropped_modules ]] && for _module in $_dropped_modules; do > + echo " -$_module" > + done > + return 1 > + fi > + > + return 0 > +} > + > # returns 0 if system is not modified > # returns 1 if system is modified > # returns 2 if system modification is invalid > @@ -485,6 +510,11 @@ check_system_modified() > return 1 > fi > > + check_kmodules_modified > + if [ $? -ne 0 ]; then > + return 1 > + fi > + > return 0 > } > > -- > 2.17.1 > _______________________________________________ > kexec mailing list -- kexec(a)lists.fedoraproject.org > To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org > Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines > List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.o... _______________________________________________ kexec mailing list -- kexec(a)lists.fedoraproject.org To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.o...

On Thu, Jul 26, 2018 at 7:49 AM, Dave Young <dyoung(a)redhat.com&gt; wrote: > On 07/26/18 at 01:51am, Bhupesh Sharma wrote: >> Hello Kairui, >> >> Thanks for the patch. >> >> Can you please list the changes in v2 (as compared to v1) in the commit message. >> >> On Wed, Jul 25, 2018 at 4:25 PM, Kairui Song <kasong(a)redhat.com&gt; wrote: >> > Currently, we only rebuilt kdump initramfs on config file change, >> > fs change, or watchdog related change. This will not cover the case >> > that hardware changed but fs layout and other configurations still >> > stays the same, and kdump may fail. >> >> Hmm. This is a chicken and egg issue. We can have modules being >> unloaded (waiting for dependencies or timeouts to be met) or loaded >> while we executing 'lsmod' via "kdumpctl restart" , then we will miss >> those modules being unloaded/loaded. > > We only handle the current loaded ones when service is starting. setup > can change after service started, it will be only detected when service > restarts eg. next reboot. > > One way is restart the kdump service for every module load uevents and > handle it in udev rules, but the side effect is we will have a lot of > restart and rebuild, it is not acceptable. So we depend on service > restart to detect the new changes. Right. So I am not sure we get desired results with the current approach as well - if we have to wait for the next reboot. May be we can think of a more comprehensive approach for handling this. Thanks, Bhupesh >> Can we look at ways to determine the same as well or at least warn the >> user that this is probably not the final/correct representation of the >> modules being unloaded/loaded when 'kdumpctl restart' is being >> executed. >> >> Thanks, >> Bhupesh >> >> > To cover such case, we can detect and compare loaded kernel modules, >> > if a hardware change requires the image to be rebuilt, loaded kernel >> > modules must have changed. >> > >> > Starting from commit 7047294 dracut will record loaded kernel modules >> > when the image is built if hostonly mode is enabled. With this patch, >> > kdumpctl will compare the recorded value with currently loaded kernel >> > modules, and rebuild the image on change. >> > >> > "kdumpctl start" will be a bit slower, as we have to call lsinitrd one >> > more time to get the loaded kernel modules list. I measure the time >> > consumption and we have an overall 0.2s increased loading time. >> > >> > Time consumption of command "kdumpctl restart": >> > >> > Before: >> > real 0m0.587s >> > user 0m0.481s >> > sys 0m0.102s >> > >> > After: >> > real 0m0.731s >> > user 0m0.591s >> > sys 0m0.133s >> > >> > Time comsumption of command "kdumpctl restart" with image rebuild: >> > >> > Before (force rebuild): >> > real 0m10.972s >> > user 0m8.966s >> > sys 0m1.318s >> > >> > After (inserted ~100 new modules): >> > real 0m11.220s >> > user 0m9.387s >> > sys 0m1.337s >> > >> > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt; >> > --- >> > kdumpctl | 30 ++++++++++++++++++++++++++++++ >> > 1 file changed, 30 insertions(+) >> > >> > diff --git a/kdumpctl b/kdumpctl >> > index 13b8209..6a01c13 100755 >> > --- a/kdumpctl >> > +++ b/kdumpctl >> > @@ -458,6 +458,31 @@ check_wdt_modified() >> > return 1 >> > } >> > >> > +check_kmodules_modified() >> > +{ >> > + # always sort again to avoid LANG/LC inconsistent problem >> > + local _old_modules="$(lsinitrd $TARGET_INITRD -f /usr/lib/dracut/loaded-kernel-modules.txt | sort)" >> > + local _new_modules="$(get_loaded_kernel_modules | sort)" >> > + >> > + [[ -z $_old_modules ]] && echo "Warning: Previous loaded kernel module list is absent or empty" >> > + >> > + local _added_modules=$(comm -13 <(echo "$_old_modules") <(echo "$_new_modules")) >> > + local _dropped_modules=$(comm -23 <(echo "$_old_modules") <(echo "$_new_modules")) >> > + >> > + if [ "$_old_modules" != "$_new_modules" ]; then >> > + echo "Detected change(s) of loaded kernel modules list:" >> > + [[ -n $_added_modules ]] && for _module in $_added_modules; do >> > + echo " +$_module" >> > + done >> > + [[ -n $_dropped_modules ]] && for _module in $_dropped_modules; do >> > + echo " -$_module" >> > + done >> > + return 1 >> > + fi >> > + >> > + return 0 >> > +} >> > + >> > # returns 0 if system is not modified >> > # returns 1 if system is modified >> > # returns 2 if system modification is invalid >> > @@ -485,6 +510,11 @@ check_system_modified() >> > return 1 >> > fi >> > >> > + check_kmodules_modified >> > + if [ $? -ne 0 ]; then >> > + return 1 >> > + fi >> > + >> > return 0 >> > } >> > >> > -- >> > 2.17.1 >> > _______________________________________________ >> > kexec mailing list -- kexec(a)lists.fedoraproject.org >> > To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org >> > Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html >> > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines >> > List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.o... >> _______________________________________________ >> kexec mailing list -- kexec(a)lists.fedoraproject.org >> To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org >> Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html >> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines >> List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.o... > > Thanks > Dave

On 07/26/18 at 02:51pm, Kairui Song wrote: > Hi all, > > I think we only care about the module loaded after the service just started. > kdump server starts after fs, network and remote-fs are ready, I think most > of the modules we care about should be > already loaded by that time. > > There might be other services try to load new modules which are not > guaranteed to finish before kdump service, > maybe we can update kdump's systemd service spec file to start after > certain services if we care about the module being loaded. > > And if there are modules we don't care being loaded at random order, maybe > kdump will rebuild the image unnecessarily, maybe we can > introduce a config option to turn off kernel module checking to avoid such > circumstances. The kdumpctl script become more complex now, I would suggest to only look into this when there are some actual bug report. For usual use cases all modules been loaded during udev probe, it is very early. Also people can still use "extra_modules" in kdump.conf. Since we are here, can you relook the bug report see what exact module inserted and what hardware changed after kdump initrd build done? The details of the actual problem can be added in patch log. > > How do you think about this solution? > > On Thu, Jul 26, 2018 at 1:39 PM Bhupesh Sharma <bhsharma(a)redhat.com&gt; wrote: > > > On Thu, Jul 26, 2018 at 7:49 AM, Dave Young <dyoung(a)redhat.com&gt; wrote: > > > On 07/26/18 at 01:51am, Bhupesh Sharma wrote: > > >> Hello Kairui, > > >> > > >> Thanks for the patch. > > >> > > >> Can you please list the changes in v2 (as compared to v1) in the commit > > message. > > >> > > >> On Wed, Jul 25, 2018 at 4:25 PM, Kairui Song <kasong(a)redhat.com&gt; wrote: > > >> > Currently, we only rebuilt kdump initramfs on config file change, > > >> > fs change, or watchdog related change. This will not cover the case > > >> > that hardware changed but fs layout and other configurations still > > >> > stays the same, and kdump may fail. > > >> > > >> Hmm. This is a chicken and egg issue. We can have modules being > > >> unloaded (waiting for dependencies or timeouts to be met) or loaded > > >> while we executing 'lsmod' via "kdumpctl restart" , then we will miss > > >> those modules being unloaded/loaded. > > > > > > We only handle the current loaded ones when service is starting. setup > > > can change after service started, it will be only detected when service > > > restarts eg. next reboot. > > > > > > One way is restart the kdump service for every module load uevents and > > > handle it in udev rules, but the side effect is we will have a lot of > > > restart and rebuild, it is not acceptable. So we depend on service > > > restart to detect the new changes. > > > > Right. So I am not sure we get desired results with the current > > approach as well - if we have to wait for the next reboot. > > > > May be we can think of a more comprehensive approach for handling this. > > > > Thanks, > > Bhupesh > > > > >> Can we look at ways to determine the same as well or at least warn the > > >> user that this is probably not the final/correct representation of the > > >> modules being unloaded/loaded when 'kdumpctl restart' is being > > >> executed. > > >> > > >> Thanks, > > >> Bhupesh > > >> > > >> > To cover such case, we can detect and compare loaded kernel modules, > > >> > if a hardware change requires the image to be rebuilt, loaded kernel > > >> > modules must have changed. > > >> > > > >> > Starting from commit 7047294 dracut will record loaded kernel modules > > >> > when the image is built if hostonly mode is enabled. With this patch, > > >> > kdumpctl will compare the recorded value with currently loaded kernel > > >> > modules, and rebuild the image on change. > > >> > > > >> > "kdumpctl start" will be a bit slower, as we have to call lsinitrd one > > >> > more time to get the loaded kernel modules list. I measure the time > > >> > consumption and we have an overall 0.2s increased loading time. > > >> > > > >> > Time consumption of command "kdumpctl restart": > > >> > > > >> > Before: > > >> > real 0m0.587s > > >> > user 0m0.481s > > >> > sys 0m0.102s > > >> > > > >> > After: > > >> > real 0m0.731s > > >> > user 0m0.591s > > >> > sys 0m0.133s > > >> > > > >> > Time comsumption of command "kdumpctl restart" with image rebuild: > > >> > > > >> > Before (force rebuild): > > >> > real 0m10.972s > > >> > user 0m8.966s > > >> > sys 0m1.318s > > >> > > > >> > After (inserted ~100 new modules): > > >> > real 0m11.220s > > >> > user 0m9.387s > > >> > sys 0m1.337s > > >> > > > >> > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt; > > >> > --- > > >> > kdumpctl | 30 ++++++++++++++++++++++++++++++ > > >> > 1 file changed, 30 insertions(+) > > >> > > > >> > diff --git a/kdumpctl b/kdumpctl > > >> > index 13b8209..6a01c13 100755 > > >> > --- a/kdumpctl > > >> > +++ b/kdumpctl > > >> > @@ -458,6 +458,31 @@ check_wdt_modified() > > >> > return 1 > > >> > } > > >> > > > >> > +check_kmodules_modified() > > >> > +{ > > >> > + # always sort again to avoid LANG/LC inconsistent problem > > >> > + local _old_modules="$(lsinitrd $TARGET_INITRD -f > > /usr/lib/dracut/loaded-kernel-modules.txt | sort)" > > >> > + local _new_modules="$(get_loaded_kernel_modules | sort)" > > >> > + > > >> > + [[ -z $_old_modules ]] && echo "Warning: Previous loaded > > kernel module list is absent or empty" > > >> > + > > >> > + local _added_modules=$(comm -13 <(echo "$_old_modules") > > <(echo "$_new_modules")) > > >> > + local _dropped_modules=$(comm -23 <(echo "$_old_modules") > > <(echo "$_new_modules")) > > >> > + > > >> > + if [ "$_old_modules" != "$_new_modules" ]; then > > >> > + echo "Detected change(s) of loaded kernel modules > > list:" > > >> > + [[ -n $_added_modules ]] && for _module in > > $_added_modules; do > > >> > + echo " +$_module" > > >> > + done > > >> > + [[ -n $_dropped_modules ]] && for _module in > > $_dropped_modules; do > > >> > + echo " -$_module" > > >> > + done > > >> > + return 1 > > >> > + fi > > >> > + > > >> > + return 0 > > >> > +} > > >> > + > > >> > # returns 0 if system is not modified > > >> > # returns 1 if system is modified > > >> > # returns 2 if system modification is invalid > > >> > @@ -485,6 +510,11 @@ check_system_modified() > > >> > return 1 > > >> > fi > > >> > > > >> > + check_kmodules_modified > > >> > + if [ $? -ne 0 ]; then > > >> > + return 1 > > >> > + fi > > >> > + > > >> > return 0 > > >> > } > > >> > > > >> > -- > > >> > 2.17.1 > > >> > _______________________________________________ > > >> > kexec mailing list -- kexec(a)lists.fedoraproject.org > > >> > To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org > > >> > Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html > > >> > List Guidelines: > > https://fedoraproject.org/wiki/Mailing_list_guidelines > > >> > List Archives: > > https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.o... > > >> _______________________________________________ > > >> kexec mailing list -- kexec(a)lists.fedoraproject.org > > >> To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org > > >> Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html > > >> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines > > >> List Archives: > > https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.o... > > > > > > Thanks > > > Dave > > > > > -- > Best Regards, > Kairui Song Thanks Dave