| Message ID | 20211203023935.3424042-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: allow zero poll queues | expand |
On Fri, 3 Dec 2021 10:39:35 +0800, Ming Lei wrote: > There isn't any reason to not allow zero poll queues from user > viewpoint. > > Also sometimes we need to compare io poll between poll mode and irq > mode, so not allowing poll queues is bad. > > > [...] Applied, thanks! [1/1] null_blk: allow zero poll queues commit: 2bfdbe8b7ebd17b5331071071a910fbabc64b436 Best regards,
On Dec 03, 2021 / 10:39, Ming Lei wrote: > There isn't any reason to not allow zero poll queues from user > viewpoint. > > Also sometimes we need to compare io poll between poll mode and irq > mode, so not allowing poll queues is bad. > > Fixes: 15dfc662ef31 ("null_blk: Fix handling of submit_queues and poll_queues attributes") > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> Hi Ming, It is good to know that the zero poll queues is useful. Having said that, I observe zero division error [1] with your patch and the commands below. Don' we need some more code changes to avoid the error? # modprobe null_blk # cd /sys/kernel/config/nullb # mkdir test # echo 0 > test/poll_queues # echo 1 > test/power Segmentation fault [1] [ 78.497149] divide error: 0000 [#1] PREEMPT SMP KASAN PTI [ 78.503281] CPU: 7 PID: 1273 Comm: bash Not tainted 5.16.0-rc3+ #2 [ 78.510178] Hardware name: Supermicro X10SLL-F/X10SLL-F, BIOS 3.0 04/24/2015 [ 78.517926] RIP: 0010:blk_mq_map_queues+0x35e/0x650 [ 78.523531] Code: 72 6b 48 8b 44 24 10 41 8d 77 01 0f b6 08 48 8b 44 24 08 83 e0 07 83 c0 03 38 c8 7c 08 84 c9 0f 85 c9 02 00 00 44 89 f8 31 d2 <f7> f3 48 8b 04 24 03 50 0c 4c 89 c8 48 c1 e8 03 42 0f b6 0c 30 4c [ 78.542990] RSP: 0018:ffff88816584fa90 EFLAGS: 00010246 [ 78.548934] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 78.556776] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff88810020a938 [ 78.564613] RBP: ffffffffafc08ab4 R08: 0000000000000000 R09: ffff88816c11ad40 [ 78.572456] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88816c11ad40 [ 78.580290] R13: 0000000000000008 R14: dffffc0000000000 R15: 0000000000000000 [ 78.588124] FS: 00007f7b8110f740(0000) GS:ffff8886edb80000(0000) knlGS:0000000000000000 [ 78.596923] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 78.603380] CR2: 0000563b46186d40 CR3: 000000013ade6004 CR4: 00000000001706e0 [ 78.611224] Call Trace: [ 78.614378] <TASK> [ 78.617191] ? lock_is_held_type+0xe4/0x140 [ 78.622105] null_map_queues+0x247/0x400 [null_blk] [ 78.627725] blk_mq_alloc_tag_set+0x511/0xe90 [ 78.632810] null_add_dev+0x1a88/0x20e0 [null_blk] [ 78.638343] nullb_device_power_store+0xe4/0x240 [null_blk] [ 78.644637] ? null_add_dev+0x20e0/0x20e0 [null_blk] [ 78.650327] ? alloc_pages+0x13b/0x260 [ 78.654795] ? null_add_dev+0x20e0/0x20e0 [null_blk] [ 78.660485] configfs_write_iter+0x2af/0x480 [ 78.665484] new_sync_write+0x359/0x5e0 [ 78.670040] ? new_sync_read+0x5d0/0x5d0 [ 78.674675] ? perf_instruction_pointer+0x180/0x1a0 [ 78.680265] ? lock_release+0x6d0/0x6d0 [ 78.684812] ? inode_security+0x54/0xf0 [ 78.689366] ? lock_is_held_type+0xe4/0x140 [ 78.694275] vfs_write+0x61e/0x920 [ 78.698401] ksys_write+0xe9/0x1b0 [ 78.702520] ? __ia32_sys_read+0xa0/0xa0 [ 78.707158] ? syscall_enter_from_user_mode+0x21/0x70 [ 78.712933] do_syscall_64+0x3b/0x90 [ 78.717219] entry_SYSCALL_64_after_hwframe+0x44/0xae [ 78.722990] RIP: 0033:0x7f7b812138d7 [ 78.727280] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24 [ 78.746736] RSP: 002b:00007ffdad700d68 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 [ 78.755013] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f7b812138d7 [ 78.762855] RDX: 0000000000000002 RSI: 0000563b46186d40 RDI: 0000000000000001 [ 78.770690] RBP: 0000563b46186d40 R08: 0000000000000000 R09: 00007f7b812c84e0 [ 78.778524] R10: 00007f7b812c83e0 R11: 0000000000000246 R12: 0000000000000002 [ 78.786361] R13: 00007f7b8130d5a0 R14: 0000000000000002 R15: 00007f7b8130d7a0 [ 78.794215] </TASK> [ 78.797106] Modules linked in: null_blk xt_CHECKSUM xt_MASQUERADE xt_conntrack ipt_REJECT nf_nat_tftp nf_conntrack_tftp bridge stp llc nft_objref nf_conntrack_netbios_ns nf_conntrack_broadcast nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_tables ebtable_nat ebtable_broute ip6table_nat ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 iptable_mangle iptable_raw iptable_security rfkill target_core_user target_core_mod ip_set nfnetlink ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter qrtr sunrpc intel_rapl_msr intel_rapl_common x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel iTCO_wdt intel_pmc_bxt iTCO_vendor_support at24 kvm irqbypass rapl intel_cstate joydev intel_uncore pcspkr i2c_i801 intel_pch_thermal i2c_smbus lpc_ich ses enclosure ie31200_edac video zram ip_tables crct10dif_pclmul crc32_pclmul [ 78.797496] crc32c_intel ast drm_vram_helper drm_kms_helper ghash_clmulni_intel cec drm_ttm_helper ttm drm mpt3sas igb e1000e dca i2c_algo_bit raid_class scsi_transport_sas fuse [ 78.899941] ---[ end trace d8088ef1fdc436e4 ]--- [ 79.005147] RIP: 0010:blk_mq_map_queues+0x35e/0x650 [ 79.010730] Code: 72 6b 48 8b 44 24 10 41 8d 77 01 0f b6 08 48 8b 44 24 08 83 e0 07 83 c0 03 38 c8 7c 08 84 c9 0f 85 c9 02 00 00 44 89 f8 31 d2 <f7> f3 48 8b 04 24 03 50 0c 4c 89 c8 48 c1 e8 03 42 0f b6 0c 30 4c [ 79.030177] RSP: 0018:ffff88816584fa90 EFLAGS: 00010246 [ 79.036122] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 79.043974] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff88810020a938 [ 79.051825] RBP: ffffffffafc08ab4 R08: 0000000000000000 R09: ffff88816c11ad40 [ 79.059669] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88816c11ad40 [ 79.067520] R13: 0000000000000008 R14: dffffc0000000000 R15: 0000000000000000 [ 79.075375] FS: 00007f7b8110f740(0000) GS:ffff8886edb80000(0000) knlGS:0000000000000000 [ 79.084170] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 79.090638] CR2: 0000563b46186d40 CR3: 000000013ade6004 CR4: 00000000001706e0
Hi Shinichiro, On Fri, Dec 03, 2021 at 10:01:33AM +0000, Shinichiro Kawasaki wrote: > On Dec 03, 2021 / 10:39, Ming Lei wrote: > > There isn't any reason to not allow zero poll queues from user > > viewpoint. > > > > Also sometimes we need to compare io poll between poll mode and irq > > mode, so not allowing poll queues is bad. > > > > Fixes: 15dfc662ef31 ("null_blk: Fix handling of submit_queues and poll_queues attributes") > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > Hi Ming, > > It is good to know that the zero poll queues is useful. Having said that, I > observe zero division error [1] with your patch and the commands below. Don' we > need some more code changes to avoid the error? > > # modprobe null_blk > # cd /sys/kernel/config/nullb > # mkdir test > # echo 0 > test/poll_queues > # echo 1 > test/power > Segmentation fault I guess the following change may fix the error: diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 20534a2daf17..96c55d06401d 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -1892,7 +1892,7 @@ static int null_init_tag_set(struct nullb *nullb, struct blk_mq_tag_set *set) if (g_shared_tag_bitmap) set->flags |= BLK_MQ_F_TAG_HCTX_SHARED; set->driver_data = nullb; - if (g_poll_queues) + if (poll_queues) set->nr_maps = 3; else set->nr_maps = 1; Thanks, Ming
On Dec 03, 2021 / 20:07, Ming Lei wrote: > Hi Shinichiro, > > On Fri, Dec 03, 2021 at 10:01:33AM +0000, Shinichiro Kawasaki wrote: > > On Dec 03, 2021 / 10:39, Ming Lei wrote: > > > There isn't any reason to not allow zero poll queues from user > > > viewpoint. > > > > > > Also sometimes we need to compare io poll between poll mode and irq > > > mode, so not allowing poll queues is bad. > > > > > > Fixes: 15dfc662ef31 ("null_blk: Fix handling of submit_queues and poll_queues attributes") > > > Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > > > Hi Ming, > > > > It is good to know that the zero poll queues is useful. Having said that, I > > observe zero division error [1] with your patch and the commands below. Don' we > > need some more code changes to avoid the error? > > > > # modprobe null_blk > > # cd /sys/kernel/config/nullb > > # mkdir test > > # echo 0 > test/poll_queues > > # echo 1 > test/power > > Segmentation fault > > I guess the following change may fix the error: > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index 20534a2daf17..96c55d06401d 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -1892,7 +1892,7 @@ static int null_init_tag_set(struct nullb *nullb, struct blk_mq_tag_set *set) > if (g_shared_tag_bitmap) > set->flags |= BLK_MQ_F_TAG_HCTX_SHARED; > set->driver_data = nullb; > - if (g_poll_queues) > + if (poll_queues) > set->nr_maps = 3; > else > set->nr_maps = 1; > Yes, I confirmed that this change avoids the error. Thank you!
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 54f7d490f8eb..b4ff5ae1f70c 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -340,9 +340,9 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev, return 0; /* - * Make sure at least one queue exists for each of submit and poll. + * Make sure at least one submit queue exists. */ - if (!submit_queues || !poll_queues) + if (!submit_queues) return -EINVAL; /* @@ -1917,8 +1917,6 @@ static int null_validate_conf(struct nullb_device *dev) if (dev->poll_queues > g_poll_queues) dev->poll_queues = g_poll_queues; - else if (dev->poll_queues == 0) - dev->poll_queues = 1; dev->prev_poll_queues = dev->poll_queues; dev->queue_mode = min_t(unsigned int, dev->queue_mode, NULL_Q_MQ);
There isn't any reason to not allow zero poll queues from user viewpoint. Also sometimes we need to compare io poll between poll mode and irq mode, so not allowing poll queues is bad. Fixes: 15dfc662ef31 ("null_blk: Fix handling of submit_queues and poll_queues attributes") Cc: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/block/null_blk/main.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)