remove magic numbers in block queue initialization
PATCH 1/1 remove magic numbers in block queue initialization
Andrew,
Your less-requests patch signaled a way-too-many magic numbers alarm (not the patches fault, of course, but it pointed it out).
Attached patch removes the minimum queue length, maximum queue length, factor of queue length that is number of batch requests, and the maximum number of batch request magic numbers and replaces them with defines and some comments.
Look OK?
Robert Love
Replace magic numbers in block queue init with sexy defines.
drivers/block/ll_rw_blk.c | 32 ++++++++++++++++++++++++-------- 1 files changed, 24 insertions(+), 8 deletions(-)
drivers/block/ll_rw_blk.c +24 -8
@@ -2109,6 +2109,22 @@__blk_put_request (req->q, req); } +/* +* The maximum and minimum free requests slots in the queue are +* dynamically calculated as a function of total memory. Below is the +* upper and lower bound to those calculations. We do not want the +* queue too large, as more memory than desired can be under writeback. +*/ +#define MAX_QUEUE_REQUESTS 128 +#define MIN_QUEUE_REQUESTS 16 + +/* +* Number of requests to batch together is calculated as the queue size +* over BATCH_QUEUE_FACTOR. This number is capped at MAX_BATCH_REQUESTS +*/ +#define BATCH_QUEUE_FACTOR 8 +#define MAX_BATCH_REQUESTS 8 +int __init blk_dev_init (void ) {int total_ram =nr_free_pages () << (PAGE_SHIFT -10 );
@@ -2125,14 +2141,14 @@* We use this many requests for reads, and this many for writes. */ queue_nr_requests = (total_ram >>9 ) & ~7 ; -if (queue_nr_requests <16 ) - queue_nr_requests =16 ; -if (queue_nr_requests >128 ) - queue_nr_requests =128 ; - - batch_requests = queue_nr_requests /8 ; -if (batch_requests >8 ) - batch_requests =8 ; +if (queue_nr_requests <MIN_QUEUE_REQUESTS ) + queue_nr_requests =MIN_QUEUE_REQUESTS ; +if (queue_nr_requests >MAX_QUEUE_REQUESTS ) + queue_nr_requests =MAX_QUEUE_REQUESTS ; + + batch_requests = queue_nr_requests /BATCH_QUEUE_FACTOR ; +if (batch_requests >MAX_BATCH_REQUESTS ) + batch_requests =MAX_BATCH_REQUESTS ;printk ("block request queues:\n" );printk (" %d requests per read queue\n" , queue_nr_requests);
Joe Perches just pointed out we can make the second if' an else if'. Probably does not affect the object code, but since we are editing this here now, why not...
Updated patch applied.
Robert Love
Replace magic numbers in block queue init with sexy defines. Also change an if to an else if in the name of micro-ops.
drivers/block/ll_rw_blk.c | 32 ++++++++++++++++++++++++-------- 1 files changed, 24 insertions(+), 8 deletions(-)
drivers/block/ll_rw_blk.c +24 -8
@@ -2109,6 +2109,22 @@__blk_put_request (req->q, req); } +/* +* The maximum and minimum free requests slots in the queue are +* dynamically calculated as a function of total memory. Below is the +* upper and lower bound to those calculations. We do not want the +* queue too large, as more memory than desired can be under writeback. +*/ +#define MAX_QUEUE_REQUESTS 128 +#define MIN_QUEUE_REQUESTS 16 + +/* +* Number of requests to batch together is calculated as the queue size +* over BATCH_QUEUE_FACTOR. This number is capped at MAX_BATCH_REQUESTS +*/ +#define BATCH_QUEUE_FACTOR 8 +#define MAX_BATCH_REQUESTS 8 +int __init blk_dev_init (void ) {int total_ram =nr_free_pages () << (PAGE_SHIFT -10 );
@@ -2125,14 +2141,14 @@* We use this many requests for reads, and this many for writes. */ queue_nr_requests = (total_ram >>9 ) & ~7 ; -if (queue_nr_requests <16 ) - queue_nr_requests =16 ; -if (queue_nr_requests >128 ) - queue_nr_requests =128 ; - - batch_requests = queue_nr_requests /8 ; -if (batch_requests >8 ) - batch_requests =8 ; +if (queue_nr_requests <MIN_QUEUE_REQUESTS ) + queue_nr_requests =MIN_QUEUE_REQUESTS ; +else if (queue_nr_requests >MAX_QUEUE_REQUESTS ) + queue_nr_requests =MAX_QUEUE_REQUESTS ; + + batch_requests = queue_nr_requests /BATCH_QUEUE_FACTOR ; +if (batch_requests >MAX_BATCH_REQUESTS ) + batch_requests =MAX_BATCH_REQUESTS ;printk ("block request queues:\n" );printk (" %d requests per read queue\n" , queue_nr_requests);
Quoted 6 lines · Robert Love
Andrew,
Your less-requests patch signaled a way-too-many magic numbers alarm (not the patches fault, of course, but it pointed it out).
Quoted 7 lines
Attached patch removes the minimum queue length, maximum queue length, factor of queue length that is number of batch requests, and the maximum number of batch request magic numbers and replaces them with defines and some comments.
Look OK?
Spose so. Sometime soon these need to be per-queue rather than global, and set to intelligent defaults by the driver and runtime tunable via files in /whereveritsmounted.
Quoted 3 lines · Andrew Morton
Spose so. Sometime soon these need to be per-queue rather than global, and set to intelligent defaults by the driver and runtime tunable via files in /whereveritsmounted.
Yah I was thinking of making them sysctl^Wsysfs files.
That can be done in the future, I guess.
Thanks,
Robert Love
Quoted 11 lines · Robert Love
Andrew,
Your less-requests patch signaled a way-too-many magic numbers alarm (not the patches fault, of course, but it pointed it out).
Attached patch removes the minimum queue length, maximum queue length, factor of queue length that is number of batch requests, and the maximum number of batch request magic numbers and replaces them with defines and some comments.
Look OK?
No, please leave these alone, testing is on-going in these parts right now.
Signature · 1 line
Jens Axboe
Quoted 2 lines · Jens Axboe
No, please leave these alone, testing is on-going in these parts right now.
Did you look at the patch? It just pulls the magic numbers out and uses defines. Should make testing easier.
Robert Love
Quoted 7 lines · Robert Love
Quoted 2 lines · Jens Axboe
No, please leave these alone, testing is on-going in these parts right now.
Did you look at the patch? It just pulls the magic numbers out and uses defines. Should make testing easier.
Yes I looked at the patch. Considering that the magic numbers are most likely going away, it's of no use.
Signature · 1 line
Jens Axboe