Message ID | 1469440019-29358-1-git-send-email-rajeev_kumar@mentor.com |
---|---|
State | Rejected |
Headers | show |
Rajeev, Am 25.07.2016 um 11:46 schrieb Rajeev Kumar: > This patch simply moves the verbose status messages associated with > UBI's fastmap feature to debugfs, thereby decreasing UBI > initialization time from 97 mS to 16 mS. Note that the first time > fastmap is invoked, building the fastmap took 146 mS. In order to > reproduce the test conditions, build with CONFIG_MTD_UBI_FASTMAP=y and > CONFIG_DYNAMIC_DEBUG=y and append the following to bootargs: > > initcall_debug ubi.mtd=0 ubi.fm_autoconvert=1 > > ubi.mtd=0 is needed at every boot if you want ubi to be > autoloaded. ubi.fm_autoconvert switch is needed only once, when > fastmap is created. So, you're working on a system which has to boot as fast as possible and writing kernel logs hurts the performance. (Slow UART?) Why do you change logging only in UBI (Fastmap)? Many other subsystems print during boot and would also hurt the performance. In such a situation I'd assume that changing the kernel log level or using the quiet kernel parameter would help more. Thanks, //richard
Richard On 07/25/2016 04:01 PM, Richard Weinberger wrote: > Rajeev, > > Am 25.07.2016 um 11:46 schrieb Rajeev Kumar: >> This patch simply moves the verbose status messages associated with >> UBI's fastmap feature to debugfs, thereby decreasing UBI >> initialization time from 97 mS to 16 mS. Note that the first time >> fastmap is invoked, building the fastmap took 146 mS. In order to >> reproduce the test conditions, build with CONFIG_MTD_UBI_FASTMAP=y and >> CONFIG_DYNAMIC_DEBUG=y and append the following to bootargs: >> >> initcall_debug ubi.mtd=0 ubi.fm_autoconvert=1 >> >> ubi.mtd=0 is needed at every boot if you want ubi to be >> autoloaded. ubi.fm_autoconvert switch is needed only once, when >> fastmap is created. > > So, you're working on a system which has to boot as fast as possible > and writing kernel logs hurts the performance. (Slow UART?) > Why do you change logging only in UBI (Fastmap)? Many other subsystems > print during boot and would also hurt the performance. > In such a situation I'd assume that changing the kernel log level or > using the quiet kernel parameter would help more. > Agreed, but the question is do we really need these all values for UBI(Fastmap) on console every time system is booted ? Thanks ~Rajeev > Thanks, > //richard >
Rajeev, Am 25.07.2016 um 13:23 schrieb Rajeev Kumar: > Richard > > On 07/25/2016 04:01 PM, Richard Weinberger wrote: >> Rajeev, >> >> Am 25.07.2016 um 11:46 schrieb Rajeev Kumar: >>> This patch simply moves the verbose status messages associated with >>> UBI's fastmap feature to debugfs, thereby decreasing UBI >>> initialization time from 97 mS to 16 mS. Note that the first time >>> fastmap is invoked, building the fastmap took 146 mS. In order to >>> reproduce the test conditions, build with CONFIG_MTD_UBI_FASTMAP=y and >>> CONFIG_DYNAMIC_DEBUG=y and append the following to bootargs: >>> >>> initcall_debug ubi.mtd=0 ubi.fm_autoconvert=1 >>> >>> ubi.mtd=0 is needed at every boot if you want ubi to be >>> autoloaded. ubi.fm_autoconvert switch is needed only once, when >>> fastmap is created. >> >> So, you're working on a system which has to boot as fast as possible >> and writing kernel logs hurts the performance. (Slow UART?) >> Why do you change logging only in UBI (Fastmap)? Many other subsystems >> print during boot and would also hurt the performance. >> In such a situation I'd assume that changing the kernel log level or >> using the quiet kernel parameter would help more. >> > > Agreed, but the question is do we really need these all values for UBI(Fastmap) on console every time system is booted ? These days I'd not print all of them. But we do already and I know setups which parse dmesg and grep for some of these values. Either to see whether attach worked or just to gather debug infos for QA. I don't want to risk breaking existing stuff. Since your problem can be solved perfectly fine by using loglevels I think it is better to stay on the safe side and keep them as-is. Thanks, //richard
On Mon, Jul 25, 2016 at 01:32:30PM +0200, Richard Weinberger wrote: > These days I'd not print all of them. But we do already and I know > setups which parse dmesg and grep for some of these values. That's horrid and should not be tolerated, please never grep the kernel log for anything. > Either to see whether attach worked or just to gather debug infos for > QA. > I don't want to risk breaking existing stuff. We do not claim that kernel log messages are an "ABI" that can not be changed or broken at all. thanks, greg k-h
Am 25.07.2016 um 18:47 schrieb Greg KH: > On Mon, Jul 25, 2016 at 01:32:30PM +0200, Richard Weinberger wrote: >> These days I'd not print all of them. But we do already and I know >> setups which parse dmesg and grep for some of these values. > > That's horrid and should not be tolerated, please never grep the kernel > log for anything. > >> Either to see whether attach worked or just to gather debug infos for >> QA. >> I don't want to risk breaking existing stuff. > > We do not claim that kernel log messages are an "ABI" that can not be > changed or broken at all. I know, but today I'm less sadistic than usual and allow users to keep their stuff. Rajeev's issue is not related to UBI and can be solved in a generic and sane way using loglevels. So there is no need to change the existing logging. Thanks, //richard
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index ef36182..59229c4 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -949,8 +949,8 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, ubi->fm_disabled = 1; } - ubi_msg(ubi, "default fastmap pool size: %d", ubi->fm_pool.max_size); - ubi_msg(ubi, "default fastmap WL pool size: %d", + dbg_gen("default fastmap pool size: %d", ubi->fm_pool.max_size); + dbg_gen("default fastmap WL pool size: %d", ubi->fm_wl_pool.max_size); #else ubi->fm_disabled = 1; @@ -1008,23 +1008,23 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, goto out_debugfs; } - ubi_msg(ubi, "attached mtd%d (name \"%s\", size %llu MiB)", + dbg_gen("attached mtd%d (name \"%s\", size %llu MiB)", mtd->index, mtd->name, ubi->flash_size >> 20); - ubi_msg(ubi, "PEB size: %d bytes (%d KiB), LEB size: %d bytes", + dbg_gen("PEB size: %d bytes (%d KiB), LEB size: %d bytes", ubi->peb_size, ubi->peb_size >> 10, ubi->leb_size); - ubi_msg(ubi, "min./max. I/O unit sizes: %d/%d, sub-page size %d", + dbg_gen("min./max. I/O unit sizes: %d/%d, sub-page size %d", ubi->min_io_size, ubi->max_write_size, ubi->hdrs_min_io_size); - ubi_msg(ubi, "VID header offset: %d (aligned %d), data offset: %d", + dbg_gen("VID header offset: %d (aligned %d), data offset: %d", ubi->vid_hdr_offset, ubi->vid_hdr_aloffset, ubi->leb_start); - ubi_msg(ubi, "good PEBs: %d, bad PEBs: %d, corrupted PEBs: %d", + dbg_gen("good PEBs: %d, bad PEBs: %d, corrupted PEBs: %d", ubi->good_peb_count, ubi->bad_peb_count, ubi->corr_peb_count); - ubi_msg(ubi, "user volume: %d, internal volumes: %d, max. volumes count: %d", + dbg_gen("user volume: %d, internal volumes: %d, max. volumes count: %d", ubi->vol_count - UBI_INT_VOL_COUNT, UBI_INT_VOL_COUNT, ubi->vtbl_slots); - ubi_msg(ubi, "max/mean erase counter: %d/%d, WL threshold: %d, image sequence number: %u", + dbg_gen("max/mean erase counter: %d/%d, WL threshold: %d, image sequence number: %u", ubi->max_ec, ubi->mean_ec, CONFIG_MTD_UBI_WL_THRESHOLD, ubi->image_seq); - ubi_msg(ubi, "available PEBs: %d, total reserved PEBs: %d, PEBs reserved for bad PEB handling: %d", + dbg_gen("available PEBs: %d, total reserved PEBs: %d, PEBs reserved for bad PEB handling: %d", ubi->avail_pebs, ubi->rsvd_pebs, ubi->beb_rsvd_pebs); /* diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c index 990898b..285a65a 100644 --- a/drivers/mtd/ubi/fastmap.c +++ b/drivers/mtd/ubi/fastmap.c @@ -1053,9 +1053,9 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai, ubi->fm = fm; ubi->fm_pool.max_size = ubi->fm->max_pool_size; ubi->fm_wl_pool.max_size = ubi->fm->max_wl_pool_size; - ubi_msg(ubi, "attached by fastmap"); - ubi_msg(ubi, "fastmap pool size: %d", ubi->fm_pool.max_size); - ubi_msg(ubi, "fastmap WL pool size: %d", + dbg_bld("attached by fastmap"); + dbg_bld("fastmap pool size: %d", ubi->fm_pool.max_size); + dbg_bld("fastmap WL pool size: %d", ubi->fm_wl_pool.max_size); ubi->fm_disabled = 0; ubi->fast_attach = 1;
This patch simply moves the verbose status messages associated with UBI's fastmap feature to debugfs, thereby decreasing UBI initialization time from 97 mS to 16 mS. Note that the first time fastmap is invoked, building the fastmap took 146 mS. In order to reproduce the test conditions, build with CONFIG_MTD_UBI_FASTMAP=y and CONFIG_DYNAMIC_DEBUG=y and append the following to bootargs: initcall_debug ubi.mtd=0 ubi.fm_autoconvert=1 ubi.mtd=0 is needed at every boot if you want ubi to be autoloaded. ubi.fm_autoconvert switch is needed only once, when fastmap is created. How to restore the verbose output for testing: ----------------------------------------------- root:~# ubidetach -p /dev/mtd0 root:~# mount -t debugfs none /mnt/dyndbg root:~# echo 'file build.c +p' > /mnt/dyndbg/dynamic_debug/control root:~# echo 'file fastmap.c +p' > /mnt/dyndbg/dynamic_debug/control root:~# ubiattach -p /dev/mtd0 root:~# mount -t ubifs /dev/ubi0_0 /mnt/card ----------------------------------------------- This patch is tested against 3.14 kernel and only build test is performed against current upstream master branch. Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com> --- drivers/mtd/ubi/build.c | 20 ++++++++++---------- drivers/mtd/ubi/fastmap.c | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-)