On 05/02/2017 at 09:23 PM, Pratyush Anand wrote:
On Tuesday 02 May 2017 05:03 PM, Xunlei Pang wrote:
>> On Tuesday 02 May 2017 02:22 PM, Xunlei Pang wrote:
>>> Use the logic of dracut 04watchdog/module-setup.sh to check,
>>> then we only need to compare the content of 00-watchdog.conf,
>>> so we can save one operation of lsinitrd.
>>>
>>> Signed-off-by: Xunlei Pang <xlpang(a)redhat.com>
>>> ---
>>> kdumpctl | 66
++++++++++++++++++++++++++++++++++++++++++----------------------
>>> 1 file changed, 44 insertions(+), 22 deletions(-)
>> Since we already have methodology to cache requisite initrd information now, so
we can avoid copying whole logic from dracut now.
>> In the dracut code, we needed to traverse all the watchdog and find list of all
active watchdogs modules, while in kdump we can exit as early as a mismatch is found. Yes,
it would have been slower with lsinitrd, but now after lsinitrd gone, probably existing
logic would be simpler.
>>
>> We are adding 44 lines and deleting 22 lines. So, IMHO, If that does not help in
improving any performance gain then probably we can be happy with minimal change(to read
from cached lsinitrd) in existing code.
> One major reason is that I didn't find a neat way to collect previous watchdog
kernel module names(see "lsinitrd $TARGET_INITRD | grep $wdtdrv &>
/dev/null").
> Do you have any good idea?
I think this ["lsinitrd $TARGET_INITRD | grep $wdtdrv &> /dev/null"] was
unnecessarily complex even earlier. Following would have lighter, no?
"lsinitrd -f etc/cmdline.d/00-watchdog.conf $TARGET_INITRD | grep $wdtdrv &>
/dev/null"
Yes, read the code further, IMO the logic of the current implementation is more complex
than that of dracut 04watchdog.
After removing the ".ko.xz" suffix $wdtdrv will be the module name, I would
rather use modprobe instead of modinfo.
The the first half part of current implementation of check_wdt_modified() is similar to
the first half part of dracut 04watchdog,
but still has a little difference:
looks like the modprobe/modinfo may return multiple module names, we need to handle them
all like:
_wdtdrv=$(modprobe --set-version "$kdump_kver" -R $_wdtdrv 2>/dev/null)
if [[ $_wdtdrv ]]; then
for i in $_wdtdrv; do
_drivers[$i]=1
done
fi
What do you think the following change?
---
kdumpctl | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/kdumpctl b/kdumpctl
index 2a87bc2..1f3a5f4 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -565,29 +565,35 @@ check_dump_fs_modified()
check_wdt_modified()
{
+ local loaded_mods wdtdrv active dir i
+
is_wdt_mod_omitted
[[ $? -eq 0 ]] && return 0
[[ -d /sys/class/watchdog/ ]] || return 0
+ loaded_mods=$(lsinitrd $TARGET_INITRD -f etc/cmdline.d/00-watchdog.conf)
for dir in /sys/class/watchdog/*; do
[[ -d "$dir" ]] || continue
[[ -f "$dir/state" ]] || continue
wdtdrv=$(< "$dir/device/modalias")
- wdtdrv=$(modinfo $wdtdrv | grep filename | awk -F"kernel/" '{print
$2}')
+ wdtdrv=$(modprobe --set-version "$kdump_kver" -R $wdtdrv
2>/dev/null)
active=$(< "$dir/state")
- # rebuild when:
- # module for this watchdog is not found and watchdog is active
- # module for this watchdog is found and watchdog is inactive
- lsinitrd $TARGET_INITRD | grep $wdtdrv &> /dev/null
- if [ $? -ne 0 ]; then
- [[ "$active" = "active" ]] && return 1
- else
- [[ "$active" = "inactive" ]] && return 1
+ if [[ $wdtdrv ]]; then
+ for i in $wdtdrv; do
+ # rebuild when:
+ # module for this watchdog is not found and watchdog is active
+ # module for this watchdog is found and watchdog is inactive
+ echo $loaded_mods | grep $i &> /dev/null
+ if [ $? -ne 0 ]; then
+ [[ "$active" = "active" ]] && return
1
+ else
+ [[ "$active" = "inactive" ]] &&
return 1
+ fi
+ done
fi
done
# check if watchdog kernel module unloaded.
- loaded_mods=$(lsinitrd $TARGET_INITRD -f etc/cmdline.d/00-watchdog.conf)
[[ -n $loaded_mods ]] && loaded_mods=$(echo $loaded_mods | awk
-F"rd.driver.pre=" '{print $2}' | sed "s/,/ /g")
for mod in $loaded_mods ; do
lsmod | grep $mod &> /dev/null
--
1.8.3.1