Bug: ll_rw_blk.c, elevator.c and displaying "default" IO Schedule r at boot-time (Cosmetic only)

Bug: ll_rw_blk.c, elevator.c and displaying "default" IO Schedule r at boot-time (Cosmetic only)

[Roberts-Thomson, James]

Hi,

I've been trying to investigate an IO performance issue on my machine, as
part of this I've noticed what is (presumably only a cosmetic) issue with
the messages displayed at kernel boot-time.

In the "good old days" (i.e. older 2.6.x kernel versions), one of the many
messages displayed at kernel boot-time was "elevator: using XXX as default
io scheduler", where XXX was one of the IO schedulers (cfq, anticipatory,
deadline, etc) depending on kernel .config at compile time.

I noticed in 2.6.11, this message has vanished (although this may have
happened in an earlier kernel), and I now get some messages "io scheduler
XXX registered".  Unfortunately, the "default" scheduler is no longer
tagged.

The Bug, however, is that code to tag the default clearly exists in
elevator.c, thus:

In function "elv_register":

        printk(KERN_INFO "io scheduler %s registered", e->elevator_name);
        if (!strcmp(e->elevator_name, chosen_elevator))
                printk(" (default)");
        printk("\n");

Some investigation has shown that when this code is called at kernel boot
time with no "elevator=xxx" kernel argument, chosen_elevator is undefined.
The code that defines chosen_elevator (elevator_setup_default) is only
called for the first time AFTER all the compiled in schedulers call
"elv_register".

However, if "elevator=xxx" is passed as a kernel argument, the code in
elv_register works.

Presumably, this code causes that behaviour:

static int __init elevator_setup(char *str)
{
        strncpy(chosen_elevator, str, sizeof(chosen_elevator) - 1);
        return 0;
}

__setup("elevator=", elevator_setup);

I imagine the fix for the bug is to have "elevator_setup_default" called
before any of the compiled in schedulers call "elv_register"; but I lack
sufficient knowledge of how the kernel hangs together to be able to patch
anything to do this.  Presumably, ll_rw_blk.c is responsible for the calling
order - somehow.

For completeness, the appropriate section from my .config:

#
# IO Schedulers
#
CONFIG_IOSCHED_NOOP=y
CONFIG_IOSCHED_AS=y
CONFIG_IOSCHED_DEADLINE=y
CONFIG_IOSCHED_CFQ=y
# CONFIG_ATA_OVER_ETH is not set

If anyone can come up with a patch, I'm happy to test and report back....

Thanks,

James Roberts-Thomson
----------
Hardware:  The parts of a computer system that can be kicked.

LKML Readers:  Please ignore the following disclaimer - this email is
explicitly declared to be non confidential and does not contain privileged
information.

This communication is confidential and may contain privileged material.
If you are not the intended recipient you must not use, disclose, copy or retain it.
If you have received it in error please immediately notify me by return email
and delete the emails.
Thank you.

Re: Bug: ll_rw_blk.c, elevator.c and displaying "default" IO Schedule r at boot-time (Cosmetic only)

[Jens Axboe]

On Wed, Mar 09 2005, Roberts-Thomson, James wrote:
> Hi,
> 
> I've been trying to investigate an IO performance issue on my machine, as
> part of this I've noticed what is (presumably only a cosmetic) issue with
> the messages displayed at kernel boot-time.
> 
> In the "good old days" (i.e. older 2.6.x kernel versions), one of the many
> messages displayed at kernel boot-time was "elevator: using XXX as default
> io scheduler", where XXX was one of the IO schedulers (cfq, anticipatory,
> deadline, etc) depending on kernel .config at compile time.
> 
> I noticed in 2.6.11, this message has vanished (although this may have
> happened in an earlier kernel), and I now get some messages "io scheduler
> XXX registered".  Unfortunately, the "default" scheduler is no longer
> tagged.

Does this work?

--- 2.6.11/drivers/block/elevator.c	2005-01-22 15:22:55.000000000 +1100
+++ test/drivers/block/elevator.c	2005-01-31 22:38:36.000000000 +1100
@@ -180,6 +180,8 @@
 
 __setup("elevator=", elevator_setup);
 
+static int default_msg = 0;
+
 int elevator_init(request_queue_t *q, char *name)
 {
 	struct elevator_type *e = NULL;
@@ -195,6 +197,12 @@
 	if (!e)
 		return -EINVAL;
 
+	if (!default_msg && !strcmp(e->elevator_name, chosen_elevator)) {
+		printk(KERN_INFO "using %s as default io scheduler\n",
+						chosen_elevator);
+		default_msg = 1;
+	}
+
 	eq = kmalloc(sizeof(struct elevator_queue), GFP_KERNEL);
 	if (!eq) {
 		elevator_put(e->elevator_type);
@@ -513,10 +521,7 @@
 	list_add_tail(&e->list, &elv_list);
 	spin_unlock_irq(&elv_list_lock);
 
-	printk(KERN_INFO "io scheduler %s registered", e->elevator_name);
-	if (!strcmp(e->elevator_name, chosen_elevator))
-		printk(" (default)");
-	printk("\n");
+	printk(KERN_INFO "io scheduler %s registered\n", e->elevator_name);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(elv_register);

-- 
Jens Axboe

Re: Bug: ll_rw_blk.c, elevator.c and displaying "default" IO Schedule r at boot-time (Cosmetic only)

[Srihari Vijayaraghavan]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

Roberts-Thomson, James wrote:
> I've been trying to investigate an IO performance issue on my machine, as
> part of this I've noticed what is (presumably only a cosmetic) issue with
> the messages displayed at kernel boot-time.

Agree, purely cosmetic.

> In the "good old days" (i.e. older 2.6.x kernel versions), one of the many
> messages displayed at kernel boot-time was "elevator: using XXX as default
> io scheduler", where XXX was one of the IO schedulers (cfq, anticipatory,
> deadline, etc) depending on kernel .config at compile time.
> 
> I noticed in 2.6.11, this message has vanished (although this may have
> happened in an earlier kernel), and I now get some messages "io scheduler
> XXX registered".  Unfortunately, the "default" scheduler is no longer
> tagged.
> 
> The Bug, however, is that code to tag the default clearly exists in
> elevator.c, thus:
> 
> In function "elv_register":
> 
>         printk(KERN_INFO "io scheduler %s registered", e->elevator_name);
>         if (!strcmp(e->elevator_name, chosen_elevator))
>                 printk(" (default)");
>         printk("\n");
> 
> Some investigation has shown that when this code is called at kernel boot
> time with no "elevator=xxx" kernel argument, chosen_elevator is undefined.
> The code that defines chosen_elevator (elevator_setup_default) is only
> called for the first time AFTER all the compiled in schedulers call
> "elv_register".
> 
> However, if "elevator=xxx" is passed as a kernel argument, the code in
> elv_register works.

Absolutely right. I am afraid, I introduced this bug (which is no worse than 
the bug I was trying to fix), while I was trying to get the elevator.c to 
print the default selection info correct, when a user uses "elevator=" boot 
parameter.

Please refer to the attachment, which contains an email, and a patch I sent to 
Jens the moment I realised the mistake (unfortunately I had no reply from 
Jens, perhaps he was/is very busy). I acknowledge that it is an ugly fix, for 
it seems silly to use an extra variable. I could not think of a better idea I 
am afraid.

I strongly believe that while elv_register() is the right place to print info 
about an elevator that is being registered, elevator_init() perhaps is the 
right place to print what is the chosen/default elevator. But the catch 22 is 
that elv_init() is called once for every block device, so I cannot see an 
easy solution to print just once.

I will try to think more about it, and if I could come up with a better idea, 
I shall inform you and LKML.

Thank you.
Hari

[-- Attachment #2: [PATCH] elevator: print default selection --]
[-- Type: text/plain, Size: 2519 bytes --]

From harisri@internode.on.net Mon Jan 31 23:10:25 2005
From: Srihari Vijayaraghavan <harisri@internode.on.net>
To: Jens Axboe <axboe@suse.de>
Subject: Re: [PATCH] elevator: print default selection
Date: Mon, 31 Jan 2005 23:10:25 +1100
User-Agent: KMail/1.7.1
References: <20050112080947.GA2736@suse.de>
In-Reply-To: <20050112080947.GA2736@suse.de>
X-KMail-Link-Message: 494
X-KMail-Link-Type: reply
MIME-Version: 1.0
Content-Disposition: inline
Status: RO
X-Status: RSC
X-KMail-EncryptionState: N
X-KMail-SignatureState: N
Content-Type: Multipart/Mixed;
  boundary="Boundary-00=_xAi/BH2etUUo8ex"
Message-Id: <200501312310.25771.harisri@internode.on.net>
X-KMail-MDN-Sent:  

--Boundary-00=_xAi/BH2etUUo8ex
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

At a cost of one extra variable, this one works great for all cases:

Hopefully the extra steps in elevator_init() won't hinder the performance, as 
elevator_init() seems to be the safest place to determine the default 
elevator, as all elevators are registered, when it is called.

Sorry if it is no good.

Thank you.
Hari.

--Boundary-00=_xAi/BH2etUUo8ex
Content-Type: text/x-diff;
  charset="iso-8859-1";
  name="elevator-default-selection"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="elevator-default-selection"

--- 2.6.11/drivers/block/elevator.c	2005-01-22 15:22:55.000000000 +1100
+++ test/drivers/block/elevator.c	2005-01-31 22:38:36.000000000 +1100
@@ -180,6 +180,8 @@
 
 __setup("elevator=", elevator_setup);
 
+static int default_msg = 0;
+
 int elevator_init(request_queue_t *q, char *name)
 {
 	struct elevator_type *e = NULL;
@@ -195,6 +197,12 @@
 	if (!e)
 		return -EINVAL;
 
+	if (!default_msg && !strcmp(e->elevator_name, chosen_elevator)) {
+		printk(KERN_INFO "using %s as default io scheduler\n",
+						chosen_elevator);
+		default_msg = 1;
+	}
+
 	eq = kmalloc(sizeof(struct elevator_queue), GFP_KERNEL);
 	if (!eq) {
 		elevator_put(e->elevator_type);
@@ -513,10 +521,7 @@
 	list_add_tail(&e->list, &elv_list);
 	spin_unlock_irq(&elv_list_lock);
 
-	printk(KERN_INFO "io scheduler %s registered", e->elevator_name);
-	if (!strcmp(e->elevator_name, chosen_elevator))
-		printk(" (default)");
-	printk("\n");
+	printk(KERN_INFO "io scheduler %s registered\n", e->elevator_name);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(elv_register);

--Boundary-00=_xAi/BH2etUUo8ex--