Re: [PATCH] only use 48-bit lba when necessary

Re: [PATCH] only use 48-bit lba when necessary

[Chuck Ebbert]

Matt Mackall wrote:


>FYI, GCC as of 3.2.3 doesn't yet reduce the if(...) form to branchless
>code but the & and && versions come out the same with -O2.


  The operands of & can be evaluated in any order, while && requires
left-to-right and does not evaluate the right operand if the left one
is false.  Only the simplest cases could possibly generate the same
code.

--
 Chuck

Re: [PATCH] only use 48-bit lba when necessary

[Matt Mackall]

On Thu, Apr 17, 2003 at 09:34:37PM -0400, Chuck Ebbert wrote:
> Matt Mackall wrote:
> 
> 
> >FYI, GCC as of 3.2.3 doesn't yet reduce the if(...) form to branchless
> >code but the & and && versions come out the same with -O2.
> 
>   The operands of & can be evaluated in any order, while && requires
> left-to-right and does not evaluate the right operand if the left one
> is false.  Only the simplest cases could possibly generate the same
> code.

Actually, any where the arguments to && are already boolean (pretty
common) and have no side effects (pretty common) will be equivalent
and could very well result in the same code. Only the simplest cases
are interesting anyway, otherwise the branchless & obfuscation is a
loss due to extra evaluation.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] only use 48-bit lba when necessary

[Timothy Miller]

Chuck Ebbert wrote:

>Matt Mackall wrote:
>
>
>  
>
>>FYI, GCC as of 3.2.3 doesn't yet reduce the if(...) form to branchless
>>code but the & and && versions come out the same with -O2.
>>    
>>
>
>
>  The operands of & can be evaluated in any order, while && requires
>left-to-right and does not evaluate the right operand if the left one
>is false.  Only the simplest cases could possibly generate the same
>code.
>
>  
>
I have a vague memory of reading a kerneltrap.org article or comment 
thread which discussed this.  The determination was that a compiler 
could choose to fully evaluate the logical expression if there were no 
side-effects.

Re: [PATCH] only use 48-bit lba when necessary

[Chuck Ebbert]

>>   The operands of & can be evaluated in any order, while && requires
>> left-to-right and does not evaluate the right operand if the left one
>> is false.  Only the simplest cases could possibly generate the same
>> code.
>
> The code must execute AS IF the right operand is only evaluated if the left
> operand is true.
>
> If an optimizer can prove that evaluating an operand has no side effects
> (which a halfway-decent optimizer can usually do for simple expressions),
> then it is free to evaluate it in any way that will produce the same
> result.


  No, that's not quite right.  Take this code for example:

   struct foo *bar;

   if (bar && bar->baz == 6) /* something */;

If bar were zero, then evaluating the right side of the && would cause
a fault.  (This is not a side effect.)

  So the AS IF part if your statement is right but you have to consider
more than just side effects.

 --
 Chuck

Re: [PATCH] only use 48-bit lba when necessary

[linux-kernel]

Matt Mackall wrote:
>> FYI, GCC as of 3.2.3 doesn't yet reduce the if(...) form to branchless
>> code but the & and && versions come out the same with -O2.

And Chuck Ebbert replied:
>   The operands of & can be evaluated in any order, while && requires
> left-to-right and does not evaluate the right operand if the left one
> is false.  Only the simplest cases could possibly generate the same
> code.

The code must execute AS IF the right operand is only evaluated if the left
operand is true.

If an optimizer can prove that evaluating an operand has no side effects
(which a halfway-decent optimizer can usually do for simple expressions),
then it is free to evaluate it in any way that will produce the same
result.

For example, if both operands of && have no side effects, then the compiler
is free to convert

if ( expensive() && cheap() ) {...}

into

if ( cheap() && expensive() ) {...}

although I expect most assume the programmer was sensible enough to
put the most commonly failed tests first.


The compiler is also free to do the same to

if ( !!cheap() & !!expensive() ) {...}

Again, it has to behave AS IF !!expensive() were evaluated in every
case, but if it can prove that there are no side effects, it can
skip expensive() entirely if there appears to be reason to do so.


The upshot of this is that, if the conditions have no side effects and
the compiler is decent, there should be NO DIFFERENCE in the compiled code.

Since it is only legal to convert between && and & if the right operand
has no side effects and does not depend on any side effects of the left
operand, the only time it makes sense to choose one over the other for
any reason but coding style is if A) the code is speed-critical, and B)
you know the conditions are met but you don't think the optimizer can
figure it out.

Which is pretty uncommon.


Regarding style, personally I'm used to C's zero/non-zero boolean tests
and prefer to write "while (p)" and "if (x & MASK)".  Using bare &
between such expressions is more difficult to read for two reasons:
A) the reader has to parse the "!!(...) " or "... != 0" clutter around
   each operand, and
B) the reader has to figure out that the "1 & 2" case never happens.

With |. that can often be avoided, but not with pointer operands.

Re: [PATCH] only use 48-bit lba when necessary

[Chuck Ebbert]

Juan Quintela wrote:


>Reason is that:
>
>if (expr)
>   var = true;
>else
>   var = false;
>
>is always a bad construct.
>
>var = expr;
>
>is a better construct to express that meaning.


 Yes, but:

   if (expr1 && expr2)
      var = true;
   else
      var = false;

is usually better turned into something that avoids jumps
when it's safe to evaluate both parts unconditionally:

   var = (expr1 != 0) & (expr2 != 0);

or (if you can stand it):

   var = !!expr1 & !!expr2;


--
 Chuck

Re: [PATCH] only use 48-bit lba when necessary

[Matt Mackall]

On Fri, Apr 04, 2003 at 12:02:00PM -0500, Chuck Ebbert wrote:
> Juan Quintela wrote:
> 
> 
> >Reason is that:
> >
> >if (expr)
> >   var = true;
> >else
> >   var = false;
> >
> >is always a bad construct.
> >
> >var = expr;
> >
> >is a better construct to express that meaning.
> 
> 
>  Yes, but:
> 
>    if (expr1 && expr2)
>       var = true;
>    else
>       var = false;
> 
> is usually better turned into something that avoids jumps
> when it's safe to evaluate both parts unconditionally:
> 
>    var = (expr1 != 0) & (expr2 != 0);
> 
> or (if you can stand it):
> 
>    var = !!expr1 & !!expr2;

Such ugly transformations are a job for compiler writers and may
occassionally be acceptable in some critical paths. The IO path, which
is literally dozens of function calls deep from read()/write() to
driver methods, does not qualify.

FYI, GCC as of 3.2.3 doesn't yet reduce the if(...) form to branchless code
but the & and && versions come out the same with -O2.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] only use 48-bit lba when necessary

[Timothy Miller]

Matt Mackall wrote:

>  
>
>> Yes, but:
>>
>>   if (expr1 && expr2)
>>      var = true;
>>   else
>>      var = false;
>>
>>is usually better turned into something that avoids jumps
>>when it's safe to evaluate both parts unconditionally:
>>
>>   var = (expr1 != 0) & (expr2 != 0);
>>
>>or (if you can stand it):
>>
>>   var = !!expr1 & !!expr2;
>>    
>>
>
>Such ugly transformations are a job for compiler writers and may
>occassionally be acceptable in some critical paths. The IO path, which
>is literally dozens of function calls deep from read()/write() to
>driver methods, does not qualify.
>

What's ugly about them?  If I were a compiler developer, I would look 
for "!!" (which I'm sure many compilers do) and deal with it properly. 
 I have seen, however, that gcc produces the same machine code for { if 
(x) {} } as for { if (x != 0) {} }.  Additionally, I would put "!!" in C 
programming books so that people understand what it means when they come 
across it.  In my mind, it's the "make-it-a-bool" operator.

I certainly don't advocate optimizations that completely obfuscate the 
meaning of the code, but for ones which are relatively innocuous and 
make sense, why not?  When not to do that is when you know what the 
compiler is going to do with it.  If you can add more characters so that 
it makes it more understandable without impacting what the compiler 
produces, then by all means, do it.  Another way to "add more characters 
to make it readable without impacting code size" is to add comments.  :) 
 Not to say that I'm any saint in that area.

But I do appreciate it when people take the time to write good, 
explanatory comments.  I'm not saying that you should comment every line 
(do you comment your comments? :), but putting something before the 
function which explans it is always a good thing, IMHO.  Even when faced 
with the most readable code, I have some sort of mental block.  I like 
it when I get to read long english textual descriptions of the POINT 
behind a function before I read the code so I have an abstract framework 
into which I fit the details.  I have a love-hate relationship with details.

Also, It seems that not all compilers perform these "obvious" 
optimizations.  But if any of the gcc contributors are watching some of 
the recent lkml discussions, I have faith that they'll add some of those 
optimizations.

Anyhow, I have no emotional attachment to my opinions about comments.  I 
do it my way, you do it yours.  I see the merit in all sides of it.  The 
only problem is that if I have trouble reading your code, I will feel 
less inclined to read it.  Oh well.

Re: [PATCH] only use 48-bit lba when necessary

[Matt Mackall]

On Thu, Apr 17, 2003 at 11:24:11AM -0400, Timothy Miller wrote:
> 
> >>Yes, but:
> >>
> >>  if (expr1 && expr2)
> >>     var = true;
> >>  else
> >>     var = false;
> >>
> >>is usually better turned into something that avoids jumps
> >>when it's safe to evaluate both parts unconditionally:
> >>
> >>  var = (expr1 != 0) & (expr2 != 0);
> >>
> >>or (if you can stand it):
> >>
> >>  var = !!expr1 & !!expr2;
> >
> >Such ugly transformations are a job for compiler writers and may
> >occassionally be acceptable in some critical paths. The IO path, which
> >is literally dozens of function calls deep from read()/write() to
> >driver methods, does not qualify.
> 
> What's ugly about them? 

It doesn't pass the test of "would I use it if I didn't think it was
faster?"

As I pointed out, your variant is not faster with a reasonable
compiler, only less obvious. And none of this sort of optimization
will ever be measurably better in the IO path anyway. But every one of
these false optimizations is a barrier to the understanding that will
allow real cleanups to make fundamental improvements.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: [PATCH] only use 48-bit lba when necessary

[Timothy Miller]

Matt Mackall wrote:

>  
>
>>>      
>>>
>>What's ugly about them? 
>>    
>>
>
>It doesn't pass the test of "would I use it if I didn't think it was
>faster?"
>
>As I pointed out, your variant is not faster with a reasonable
>compiler, only less obvious. And none of this sort of optimization
>will ever be measurably better in the IO path anyway. But every one of
>these false optimizations is a barrier to the understanding that will
>allow real cleanups to make fundamental improvements.
>
>  
>
Agreed, but in this case, it's simply a matter of culture.  If some of 
these things were more common, then they would pass the test, like the 
use of "!!".

One thing for your side of the argument is that if something works, 
don't mess with it and risk breaking it, especially if it has no 
practical impact on performance.

If we really wanted to improve readability, we could start doing 
something like Windows developers are so fond of doing and use Hungarian 
Notation.  I know it's ugly, and we eschew anything to do with 
Microsoft, but once you get used to it, it can be very helpful in 
keeping things straight.

[PATCH] only use 48-bit lba when necessary

[Jens Axboe]

Hi,

48-bit lba has a non-significant overhead (twice the outb's, 12 instead
of 6 per command), so it makes sense to use 28-bit lba commands whenever
we can.

Patch is against 2.5.66-BK.

===== drivers/ide/ide-disk.c 1.36 vs edited =====
--- 1.36/drivers/ide/ide-disk.c	Wed Mar 26 21:23:01 2003
+++ edited/drivers/ide/ide-disk.c	Fri Apr  4 14:18:41 2003
@@ -367,12 +367,15 @@
 static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, sector_t block)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	u8 lba48		= (drive->addressing == 1) ? 1 : 0;
+	u8 lba48		= 0;
 	task_ioreg_t command	= WIN_NOP;
 	ata_nsector_t		nsectors;
 
 	nsectors.all		= (u16) rq->nr_sectors;
 
+	if (drive->addressing == 1 && block > 0xfffffff)
+		lba48 = 1;
+
 	if (driver_blocked)
 		panic("Request while ide driver is blocked?");
 
@@ -392,7 +395,7 @@
 		hwif->OUTB(drive->ctl, IDE_CONTROL_REG);
 
 	if (drive->select.b.lba) {
-		if (drive->addressing == 1) {
+		if (lba48) {
 			task_ioreg_t tasklets[10];
 
 			if (blk_rq_tagged(rq)) {
@@ -502,6 +505,7 @@
 		command = ((drive->mult_count) ?
 			   ((lba48) ? WIN_MULTREAD_EXT : WIN_MULTREAD) :
 			   ((lba48) ? WIN_READ_EXT : WIN_READ));
+
 		ide_execute_command(drive, command, &read_intr, WAIT_CMD, NULL);
 		return ide_started;
 	} else if (rq_data_dir(rq) == WRITE) {
@@ -577,6 +581,11 @@
  */
 static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, sector_t block)
 {
+	int lba48bit = 0;
+
+	if (drive->addressing == 1 && block > 0xfffffff)
+		lba48bit = 1;
+
 	BUG_ON(drive->blocked);
 	if (!blk_fs_request(rq)) {
 		blk_dump_rq_flags(rq, "do_rw_disk - bad command");
@@ -602,7 +611,7 @@
 		return ide_started;
 	}
 
-	if (drive->addressing == 1)		/* 48-bit LBA */
+	if (lba48bit && block > 0xfffffff)
 		return lba_48_rw_disk(drive, rq, (unsigned long long) block);
 	if (drive->select.b.lba)		/* 28-bit LBA */
 		return lba_28_rw_disk(drive, rq, (unsigned long) block);
@@ -611,9 +620,13 @@
 	return chs_rw_disk(drive, rq, (unsigned long) block);
 }
 
-static task_ioreg_t get_command (ide_drive_t *drive, int cmd)
+static task_ioreg_t get_command (ide_drive_t *drive, struct request *rq)
 {
-	int lba48bit = (drive->addressing == 1) ? 1 : 0;
+	int cmd = rq_data_dir(rq);
+	int lba48bit = 0;
+
+	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+		lba48bit = 1;
 
 	if ((cmd == READ) && drive->using_tcq)
 		return lba48bit ? WIN_READDMA_QUEUED_EXT : WIN_READDMA_QUEUED;
@@ -640,7 +653,7 @@
 	ide_task_t		args;
 	int			sectors;
 	ata_nsector_t		nsectors;
-	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
+	task_ioreg_t command	= get_command(drive, rq);
 	unsigned int track	= (block / drive->sect);
 	unsigned int sect	= (block % drive->sect) + 1;
 	unsigned int head	= (track % drive->head);
@@ -672,6 +685,7 @@
 	args.tfRegister[IDE_SELECT_OFFSET]	|= drive->select.all;
 	args.tfRegister[IDE_COMMAND_OFFSET]	= command;
 	args.command_type			= ide_cmd_type_parser(&args);
+	args.addressing				= 0;
 	args.rq					= (struct request *) rq;
 	rq->special				= (ide_task_t *)&args;
 	return do_rw_taskfile(drive, &args);
@@ -682,7 +696,7 @@
 	ide_task_t		args;
 	int			sectors;
 	ata_nsector_t		nsectors;
-	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
+	task_ioreg_t command	= get_command(drive, rq);
 
 	nsectors.all = (u16) rq->nr_sectors;
 
@@ -710,6 +724,7 @@
 	args.tfRegister[IDE_SELECT_OFFSET]	|= drive->select.all;
 	args.tfRegister[IDE_COMMAND_OFFSET]	= command;
 	args.command_type			= ide_cmd_type_parser(&args);
+	args.addressing				= 0;
 	args.rq					= (struct request *) rq;
 	rq->special				= (ide_task_t *)&args;
 	return do_rw_taskfile(drive, &args);
@@ -726,7 +741,7 @@
 	ide_task_t		args;
 	int			sectors;
 	ata_nsector_t		nsectors;
-	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
+	task_ioreg_t command	= get_command(drive, rq);
 
 	nsectors.all = (u16) rq->nr_sectors;
 
@@ -762,6 +777,7 @@
 	args.hobRegister[IDE_SELECT_OFFSET_HOB]	= drive->select.all;
 	args.hobRegister[IDE_CONTROL_OFFSET_HOB]= (drive->ctl|0x80);
 	args.command_type			= ide_cmd_type_parser(&args);
+	args.addressing				= 1;
 	args.rq					= (struct request *) rq;
 	rq->special				= (ide_task_t *)&args;
 	return do_rw_taskfile(drive, &args);
===== drivers/ide/ide-dma.c 1.13 vs edited =====
--- 1.13/drivers/ide/ide-dma.c	Fri Mar  7 18:14:31 2003
+++ edited/drivers/ide/ide-dma.c	Fri Apr  4 14:15:37 2003
@@ -653,7 +653,7 @@
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct request *rq	= HWGROUP(drive)->rq;
 	unsigned int reading	= 1 << 3;
-	u8 lba48		= (drive->addressing == 1) ? 1 : 0;
+	u8 lba48		= 0;
 	task_ioreg_t command	= WIN_NOP;
 
 	/* try pio */
@@ -663,11 +663,14 @@
 	if (drive->media != ide_disk)
 		return 0;
 
+	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+		lba48 = 1;
+
 	command = (lba48) ? WIN_READDMA_EXT : WIN_READDMA;
-	
+
 	if (drive->vdma)
 		command = (lba48) ? WIN_READ_EXT: WIN_READ;
-		
+
 	if (rq->flags & REQ_DRIVE_TASKFILE) {
 		ide_task_t *args = rq->special;
 		command = args->tfRegister[IDE_COMMAND_OFFSET];
@@ -685,7 +688,7 @@
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct request *rq	= HWGROUP(drive)->rq;
 	unsigned int reading	= 0;
-	u8 lba48		= (drive->addressing == 1) ? 1 : 0;
+	u8 lba48		= 0;
 	task_ioreg_t command	= WIN_NOP;
 
 	/* try PIO instead of DMA */
@@ -695,10 +698,13 @@
 	if (drive->media != ide_disk)
 		return 0;
 
+	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+		lba48 = 1;
+
 	command = (lba48) ? WIN_WRITEDMA_EXT : WIN_WRITEDMA;
 	if (drive->vdma)
 		command = (lba48) ? WIN_WRITE_EXT: WIN_WRITE;
-		
+
 	if (rq->flags & REQ_DRIVE_TASKFILE) {
 		ide_task_t *args = rq->special;
 		command = args->tfRegister[IDE_COMMAND_OFFSET];
===== drivers/ide/ide-taskfile.c 1.14 vs edited =====
--- 1.14/drivers/ide/ide-taskfile.c	Wed Mar 26 21:22:22 2003
+++ edited/drivers/ide/ide-taskfile.c	Fri Apr  4 14:20:26 2003
@@ -147,7 +147,7 @@
 	ide_hwif_t *hwif	= HWIF(drive);
 	task_struct_t *taskfile	= (task_struct_t *) task->tfRegister;
 	hob_struct_t *hobfile	= (hob_struct_t *) task->hobRegister;
-	u8 HIHI			= (drive->addressing == 1) ? 0xE0 : 0xEF;
+	u8 HIHI			= task->addressing == 1 ? 0xE0 : 0xEF;
 
 #ifdef CONFIG_IDE_TASK_IOCTL_DEBUG
 	void debug_taskfile(drive, task);
@@ -160,7 +160,7 @@
 	}
 	SELECT_MASK(drive, 0);
 
-	if (drive->addressing == 1) {
+	if (task->addressing == 1) {
 		hwif->OUTB(hobfile->feature, IDE_FEATURE_REG);
 		hwif->OUTB(hobfile->sector_count, IDE_NSECTOR_REG);
 		hwif->OUTB(hobfile->sector_number, IDE_SECTOR_REG);
@@ -332,7 +332,7 @@
 	args->tfRegister[IDE_STATUS_OFFSET]  = stat;
 	if ((drive->id->command_set_2 & 0x0400) &&
 	    (drive->id->cfs_enable_2 & 0x0400) &&
-	    (drive->addressing == 1)) {
+	    (args->addressing == 1)) {
 		hwif->OUTB(drive->ctl|0x80, IDE_CONTROL_REG_HOB);
 		args->hobRegister[IDE_FEATURE_OFFSET_HOB] = hwif->INB(IDE_FEATURE_REG);
 		args->hobRegister[IDE_NSECTOR_OFFSET_HOB] = hwif->INB(IDE_NSECTOR_REG);
@@ -1795,13 +1795,13 @@
 	 */
 	if (task->tf_out_flags.all == 0) {
 		task->tf_out_flags.all = IDE_TASKFILE_STD_OUT_FLAGS;
-		if (drive->addressing == 1)
+		if (task->addressing == 1)
 			task->tf_out_flags.all |= (IDE_HOB_STD_OUT_FLAGS << 8);
         }
 
 	if (task->tf_in_flags.all == 0) {
 		task->tf_in_flags.all = IDE_TASKFILE_STD_IN_FLAGS;
-		if (drive->addressing == 1)
+		if (task->addressing == 1)
 			task->tf_in_flags.all |= (IDE_HOB_STD_IN_FLAGS  << 8);
         }
 
===== include/linux/ide.h 1.42 vs edited =====
--- 1.42/include/linux/ide.h	Thu Mar 20 19:23:19 2003
+++ edited/include/linux/ide.h	Fri Apr  4 14:07:50 2003
@@ -1408,6 +1408,7 @@
 	ide_reg_valid_t		tf_in_flags;
 	int			data_phase;
 	int			command_type;
+	int			addressing;	/* 1 for 48-bit */
 	ide_pre_handler_t	*prehandler;
 	ide_handler_t		*handler;
 	ide_post_handler_t	*posthandler;

-- 
Jens Axboe

Re: [PATCH] only use 48-bit lba when necessary

[Juan Quintela]

>>>>> "jens" == Jens Axboe <axboe@suse.de> writes:

jens> Hi,
jens> 48-bit lba has a non-significant overhead (twice the outb's, 12 instead
jens> of 6 per command), so it makes sense to use 28-bit lba commands whenever
jens> we can.

jens> Patch is against 2.5.66-BK.

jens> ===== drivers/ide/ide-disk.c 1.36 vs edited =====
jens> --- 1.36/drivers/ide/ide-disk.c	Wed Mar 26 21:23:01 2003
jens> +++ edited/drivers/ide/ide-disk.c	Fri Apr  4 14:18:41 2003
jens> @@ -367,12 +367,15 @@
jens> static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, sector_t block)
jens> {
jens> ide_hwif_t *hwif	= HWIF(drive);
jens> -	u8 lba48		= (drive->addressing == 1) ? 1 : 0;
jens> +	u8 lba48		= 0;
jens> task_ioreg_t command	= WIN_NOP;
jens> ata_nsector_t		nsectors;
 
jens> nsectors.all		= (u16) rq->nr_sectors;
 
jens> +	if (drive->addressing == 1 && block > 0xfffffff)
jens> +		lba48 = 1;
jens> +

lba48 = (drive->addressing == 1) && (block > 0xfffffff);

should do the trick.

jens> +	int lba48bit = 0;
jens> +
jens> +	if (drive->addressing == 1 && block > 0xfffffff)
jens> +		lba48bit = 1;

see above (more cases of that stripped).

jens> +	if (lba48bit && block > 0xfffffff)

that test should be equivalent to:

if (lba48bit)

(at least the cvs copy of that function don't modify block isnce the
definition of lba48bit).

Talking about consistency, wouldn't be better to use always the same
name, lba48 or lba48bit?

Later, Juan.

-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy

Re: [PATCH] only use 48-bit lba when necessary

[Jens Axboe]

On Fri, Apr 04 2003, Juan Quintela wrote:
> >>>>> "jens" == Jens Axboe <axboe@suse.de> writes:
> 
> jens> Hi,
> jens> 48-bit lba has a non-significant overhead (twice the outb's, 12 instead
> jens> of 6 per command), so it makes sense to use 28-bit lba commands whenever
> jens> we can.
> 
> jens> Patch is against 2.5.66-BK.
> 
> jens> ===== drivers/ide/ide-disk.c 1.36 vs edited =====
> jens> --- 1.36/drivers/ide/ide-disk.c	Wed Mar 26 21:23:01 2003
> jens> +++ edited/drivers/ide/ide-disk.c	Fri Apr  4 14:18:41 2003
> jens> @@ -367,12 +367,15 @@
> jens> static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, sector_t block)
> jens> {
> jens> ide_hwif_t *hwif	= HWIF(drive);
> jens> -	u8 lba48		= (drive->addressing == 1) ? 1 : 0;
> jens> +	u8 lba48		= 0;
> jens> task_ioreg_t command	= WIN_NOP;
> jens> ata_nsector_t		nsectors;
>  
> jens> nsectors.all		= (u16) rq->nr_sectors;
>  
> jens> +	if (drive->addressing == 1 && block > 0xfffffff)
> jens> +		lba48 = 1;
> jens> +
> 
> lba48 = (drive->addressing == 1) && (block > 0xfffffff);
> 
> should do the trick.

I'm not going to use such nonsense, sorry. The spelled out versions are
a lot more readable. The command ?: constructs used in ide-disk are a
joke, imo.

> jens> +	if (lba48bit && block > 0xfffffff)
> 
> that test should be equivalent to:
> 
> if (lba48bit)

Yes that is correct, that should be changed.

> Talking about consistency, wouldn't be better to use always the same
> name, lba48 or lba48bit?

There was no consistency to begin with, if anything my patch is
consistent with the inconsistent source :)

-- 
Jens Axboe

Re: [PATCH] only use 48-bit lba when necessary

[Juan Quintela]

Hi

jens> +	if (drive->addressing == 1 && block > 0xfffffff)
jens> +		lba48 = 1;
jens> +
>> 
>> lba48 = (drive->addressing == 1) && (block > 0xfffffff);
>> 
>> should do the trick.

jens> I'm not going to use such nonsense, sorry. The spelled out versions are
jens> a lot more readable. The command ?: constructs used in ide-disk are a
jens> joke, imo.

Read it again, please.  Told me wehre are the ?: command.

Reason is that:

if (expr)
   var = true;
else
   var = false;

is always a bad construct.

var = expr;

is a better construct to express that meaning.

And yes, your is a variation of the same theme:

var = false;
if (expr)
   var = true;

Later, Juan "who also didn't like ?: operator"



-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy

Re: [PATCH] only use 48-bit lba when necessary

[Jens Axboe]

On Fri, Apr 04 2003, Juan Quintela wrote:
> 
> Hi
> 
> jens> +	if (drive->addressing == 1 && block > 0xfffffff)
> jens> +		lba48 = 1;
> jens> +
> >> 
> >> lba48 = (drive->addressing == 1) && (block > 0xfffffff);
> >> 
> >> should do the trick.
> 
> jens> I'm not going to use such nonsense, sorry. The spelled out versions are
> jens> a lot more readable. The command ?: constructs used in ide-disk are a
> jens> joke, imo.
> 
> Read it again, please.  Told me wehre are the ?: command.

Oh, you are right. It was the one-liner style that threw me off.

> Reason is that:
> 
> if (expr)
>    var = true;
> else
>    var = false;
> 
> is always a bad construct.
>
> var = expr;
> 
> is a better construct to express that meaning.
> 
> And yes, your is a variation of the same theme:
> 
> var = false;
> if (expr)
>    var = true;

Yes, but mine is more readable. IMO of course, that's the way it is with
styles.

-- 
Jens Axboe

Re: [PATCH] only use 48-bit lba when necessary

[John Bradford]

> > Reason is that:
> > 
> > if (expr)
> >    var = true;
> > else
> >    var = false;
> > 
> > is always a bad construct.
> >
> > var = expr;
> > 
> > is a better construct to express that meaning.
> > 
> > And yes, your is a variation of the same theme:
> > 
> > var = false;
> > if (expr)
> >    var = true;
> 
> Yes, but mine is more readable. IMO of course, that's the way it is with
> styles.

if (!expr || expr) var=(!expr ? !expr : expr);

:-)

John.

Re: [PATCH] only use 48-bit lba when necessary

[Andries Brouwer]

On Fri, Apr 04, 2003 at 02:29:36PM +0200, Jens Axboe wrote:

> 48-bit lba has a non-significant overhead (twice the outb's, 12 instead

s/non-// ?

> of 6 per command), so it makes sense to use 28-bit lba commands whenever
> we can.
> 

> +	if (drive->addressing == 1 && block > 0xfffffff)
> +		lba48 = 1;

Hmm. I wonder whether we should be more cautious, and ask for lba48
as soon as some part of the interval is past this limit.
(say, block+nsectors > 0xfffffff)

I don't know whether the standard spells out what happens
at the boundary, but for example the LBA low/mid/high, DEV is required
to contain the sector number at the place the error occurred,
and that is possible only if one stays below the 28-byte sector limit.

Andries

Re: [PATCH] only use 48-bit lba when necessary

[Jens Axboe]

On Fri, Apr 04 2003, Andries Brouwer wrote:
> On Fri, Apr 04, 2003 at 02:29:36PM +0200, Jens Axboe wrote:
> 
> > 48-bit lba has a non-significant overhead (twice the outb's, 12 instead
> 
> s/non-// ?

Of course :)

> > of 6 per command), so it makes sense to use 28-bit lba commands whenever
> > we can.
> > 
> 
> > +	if (drive->addressing == 1 && block > 0xfffffff)
> > +		lba48 = 1;
> 
> Hmm. I wonder whether we should be more cautious, and ask for lba48
> as soon as some part of the interval is past this limit.
> (say, block+nsectors > 0xfffffff)
> 
> I don't know whether the standard spells out what happens
> at the boundary, but for example the LBA low/mid/high, DEV is required
> to contain the sector number at the place the error occurred,
> and that is possible only if one stays below the 28-byte sector limit.

That might not be a bad idea, just to be on the safe side. I'll do that.

-- 
Jens Axboe