On Fri, May 13, 2022 at 2:10 PM HAGIO KAZUHITO(萩尾 一仁) <[email protected]> wrote:
> -----Original Message----- > > Hi, Kazu > > Thank you for the comment. > > > > On Thu, May 12, 2022 at 1:59 PM HAGIO KAZUHITO(萩尾 一仁) < > [email protected] <mailto:[email protected]> > > > wrote: > > > > > > Hi Lianbo, > > > > Thank you for working on this, we know the stats are very useful > but > > it's a tough work to support :-( > > > > I have a few questions about basic design: > > > > - Is it possible to re-use sbitmap_for_each_set() etc. in > sbitmap.c? > > They were implemented like the sbitmap functions in kernel, maybe > > we can imitate bt_for_each() in kernel. Otherwise, we will have to > > update sbitmap.c and dev.c when a change in sbitmap structure > occurs. > > or did you find any reason that they cannot be used? > > > > > > > > The main reason is that they have different code logic, and not sure if > that > > is easy to reuse, just like the sbitmap_bitmap_show(), which doesn't > reuse > > the sbitmap_for_each_set() in the sbitmap.c. ^^ > > Got it. > > crash's sbitmap_bitmap_show() imitates kernel's sbitmap_bitmap_show(), > they print the whole bitmap: > # cat /sys/kernel/debug/block/sda/hctx0/tags_bitmap > 00000000: 0000 0000 > > crash's sbitmap_for_each_set() imitates kernel's sbitmap_for_each_set(), > they do callback on each set bit. This time we want to check this like > blk_mq_queue_tag_busy_iter(), it should be used. > > > But if it can be easily reused, that would be a good idea. > > Yes, if we can imitate bt_for_each(), it will make tracking changes easier. > Let me try it. > > Note: the following kernel commit(mentioned in the cover-letter) may > make > > that the sbitmap can not work as expected. > > [3] commit <3301bc53358a> ("lib/sbitmap: kill 'depth' from sbitmap_word") > > Thanks for the info. so do you mean we need to fix sbitmap.c for 5.18? > I think this can be done later. > Yes, you are right. It's another issue, which can be fixed with a separate patch. > > > > > > > > - Is it possible to use the new logic when blk-mq has sbitmap, > even if > > a kernel has rq_dispatched and rq_completed? > > > > > > > > If we would like to replace the rq_dispatched/rq_completed with the > sbitmap > > to calculate the disk I/O statistics, that might be extra work. > > > > But anyway, if it's worth doing, we might consider doing it at the next > stage. > > What do you think? > > sorry, cannot get it. I just mean this flow: > > if blk-mq uses sbitmap then > get_mq_diskio_from_hw_queues(q, &tmp); > return; > else if rq_dispatched/rq_completed exists then > ... > get_one_mctx_diskio(mctx_addr, &tmp); > It means that crash will preferentially use the sbitmap to calculate the disk I/O statistics since kernel started to introduce the sbitmap for the blk-mq feature. > What is the extra work? > The extra work is that: crash has to support parsing sbitmap on old kernel, and need to follow up the relevant kernel structure changes, it will make the implementation of the get_mq_diskio_from_hw_queues() become more complex. That is my concern. If I misunderstood, please correct me. Thanks. Lianbo > > > > BTW: I will improve the v1 and post v2 later, because I have got some > feedback > > from Nitin and John pittman. Also discussed with you in another email > thread. > > Thanks, I see. > > Kazu > > >
-- Crash-utility mailing list [email protected] https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
