[PATCH 1/1] nvme-fabrics: use default value for nr_io_queues setting

[PATCH 1/1] nvme-fabrics: use default value for nr_io_queues setting

[Max Gurtovoy]

We set default value for nr_io_queues to be num_online_cpus(). No need
to call num_online_cpus() again since we can use the default value we
set earlier.

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/fabrics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 903eb45..fbfb1ba 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -708,7 +708,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			}
 
 			opts->nr_io_queues = min_t(unsigned int,
-					num_online_cpus(), token);
+					opts->nr_io_queues, token);
 			break;
 		case NVMF_OPT_KATO:
 			if (match_int(args, &token)) {
-- 
1.8.3.1

[PATCH 1/1] nvme-fabrics: use default value for nr_io_queues setting

[Sagi Grimberg]

> We set default value for nr_io_queues to be num_online_cpus(). No need
> to call num_online_cpus() again since we can use the default value we
> set earlier.

We should not override user arguments. plus num_online_cpus can change
from X to 1 and back to X...

[PATCH 1/1] nvme-fabrics: use default value for nr_io_queues setting

[Max Gurtovoy]

On 6/24/2018 7:01 PM, Sagi Grimberg wrote:
> 
>> We set default value for nr_io_queues to be num_online_cpus(). No need
>> to call num_online_cpus() again since we can use the default value we
>> set earlier.
> 
> We should not override user arguments. plus num_online_cpus can change
> from X to 1 and back to X...

but we set "opts->nr_io_queues = num_online_cpus();" in the beginning of 
nvmf_parse_options function.

I just don't want to call num_online_cpus() twice in the same function..

[PATCH 1/1] nvme-fabrics: use default value for nr_io_queues setting

[Sagi Grimberg]

>>> We set default value for nr_io_queues to be num_online_cpus(). No need
>>> to call num_online_cpus() again since we can use the default value we
>>> set earlier.
>>
>> We should not override user arguments. plus num_online_cpus can change
>> from X to 1 and back to X...
> 
> but we set "opts->nr_io_queues = num_online_cpus();" in the beginning of 
> nvmf_parse_options function.

I misread this and thought that this was in the rdma transport, sorry.

I assume that still theoretically it can change during parse_options.
What is this needed again? Wouldn't it be better to remove it (or bound
it with num_possible_cpus) and have the transport clamp down to
num_online_cpus?

[PATCH 1/1] nvme-fabrics: use default value for nr_io_queues setting

[Max Gurtovoy]

On 6/24/2018 7:16 PM, Sagi Grimberg wrote:
> 
>>>> We set default value for nr_io_queues to be num_online_cpus(). No need
>>>> to call num_online_cpus() again since we can use the default value we
>>>> set earlier.
>>>
>>> We should not override user arguments. plus num_online_cpus can change
>>> from X to 1 and back to X...
>>
>> but we set "opts->nr_io_queues = num_online_cpus();" in the beginning 
>> of nvmf_parse_options function.
> 
> I misread this and thought that this was in the rdma transport, sorry.
> 
> I assume that still theoretically it can change during parse_options.
> What is this needed again? Wouldn't it be better to remove it (or bound
> it with num_possible_cpus) and have the transport clamp down to
> num_online_cpus?
> 
> 

The transport clamp it. I just noticed we set it twice by calling 
num_online_cpus() in case we connect with --nr-io-queues options instead 
of using the cached value.
If you're ok with this we can leave it "as is".

[PATCH 1/1] nvme-fabrics: use default value for nr_io_queues setting

[Sagi Grimberg]

>>>>> We set default value for nr_io_queues to be num_online_cpus(). No need
>>>>> to call num_online_cpus() again since we can use the default value we
>>>>> set earlier.
>>>>
>>>> We should not override user arguments. plus num_online_cpus can change
>>>> from X to 1 and back to X...
>>>
>>> but we set "opts->nr_io_queues = num_online_cpus();" in the beginning 
>>> of nvmf_parse_options function.
>>
>> I misread this and thought that this was in the rdma transport, sorry.
>>
>> I assume that still theoretically it can change during parse_options.
>> What is this needed again? Wouldn't it be better to remove it (or bound
>> it with num_possible_cpus) and have the transport clamp down to
>> num_online_cpus?
>>
>>
> 
> The transport clamp it. I just noticed we set it twice by calling 
> num_online_cpus() in case we connect with --nr-io-queues options instead 
> of using the cached value.
> If you're ok with this we can leave it "as is".

I think it would be good to have default of num_online_cpus and bound
the user with num_possible_cpus instead (if the user asked for more than
online). The transport will clamp it down regardless, but if/when more
cpus come in, transport can re-adjust in the next reset.

thoughts?