iscsi_sna_lte vs RFC1982
Hannes Reinecke
debugging my infamous iSCSI IO stall I stumbled across this:
drivers/scsi/libiscsi.c:
/* Serial Number Arithmetic, 32 bits, less than, RFC1982 */
static int iscsi_sna_lt(u32 n1, u32 n2)
{
return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
(n1 > n2 && (n2 - n1 < SNA32_CHECK)));
}
however, reading through RFC1982 it actually states:
3.2. Comparison
...
s1 is said to be less than s2 if, and only if, s1 is not equal to s2,
and
(i1 < i2 and i2 - i1 < 2^(SERIAL_BITS - 1)) or
(i1 > i2 and i1 - i2 > 2^(SERIAL_BITS - 1))
Do I read that correctly and shouldn't our code actually read:
return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
(n1 > n2 && (n1 - n2 > SNA32_CHECK)));
?
Otherwise we'd be off by one, or am I missing something here?
Slightly confused,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
ha...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
Boaz Harrosh
> Hi Mike,
>
> debugging my infamous iSCSI IO stall I stumbled across this:
>
> drivers/scsi/libiscsi.c:
>
> /* Serial Number Arithmetic, 32 bits, less than, RFC1982 */
> static int iscsi_sna_lt(u32 n1, u32 n2)
> {
> return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
> (n1 > n2 && (n2 - n1 < SNA32_CHECK)));
> }
>
> however, reading through RFC1982 it actually states:
>
> 3.2. Comparison
>
> ...
>
> s1 is said to be less than s2 if, and only if, s1 is not equal to s2,
> and
>
> (i1 < i2 and i2 - i1 < 2^(SERIAL_BITS - 1)) or
> (i1 > i2 and i1 - i2 > 2^(SERIAL_BITS - 1))
>
> Do I read that correctly and shouldn't our code actually read:
>
> return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
> (n1 > n2 && (n1 - n2 > SNA32_CHECK)));
>
Even if the code is the same please send a patch to change it.
As is, it is totally unclear and confusing. And if you are at it
please also:
@@ -84,7 +84,7 @@ MODULE_PARM_DESC(debug_libiscsi_eh,
} while (0);
/* Serial Number Arithmetic, 32 bits, less than, RFC1982 */
-#define SNA32_CHECK 2147483648UL
+#define SNA32_CHECK 0x80000000UL
static int iscsi_sna_lt(u32 n1, u32 n2)
{
---
But lets see:
Say yours is correct:
n1 - n2 > 0x80000000
Lets apply a -(x) to both sides: (if 2 > 1 then -2 < -1)
-(n1 - n2) < -0x8000000
but since -0x80000000 == 0x80000000 on 32 bits then
n2 - n1 < 0x800000
which is as stated above (n2 - n1 < SNA32_CHECK)
> ?
>
> Otherwise we'd be off by one, or am I missing something here?
>
> Slightly confused,
>
Unless in 64bit machines we get funny things with u32 promoted to s64.
I'd say better change it.
> Hannes
Thanks
Boaz
Charlie Brady
On Mon, 6 Jul 2009, Hannes Reinecke wrote:
> Do I read that correctly and shouldn't our code actually read:
>
> return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
> (n1 > n2 && (n1 - n2 > SNA32_CHECK)));
>
> ?
>
> Otherwise we'd be off by one, or am I missing something here?
return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
(n1 > n2 && (n1 - n2 > SNA32_CHECK)));
is equivalent to
return ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
(n1 > n2 && (n1 - n2 > SNA32_CHECK)));
Both are false if n1 == n2.
Ulrich Windl
> return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
> (n1 > n2 && (n1 - n2 > SNA32_CHECK)));
>
> is equivalent to
>
> return ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
> (n1 > n2 && (n1 - n2 > SNA32_CHECK)));
>
> Both are false if n1 == n2.
>
Maybe it's because of the short-cut evaluation that wants to optimize the case "n1
== n2", but I don't know.
Ulrich
Charlie Brady
What is your point?
My point is that it would achieve nothing to change:
return ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
(n1 > n2 && (n1 - n2 > SNA32_CHECK)));
to:
return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
(n1 > n2 && (n1 - n2 > SNA32_CHECK)));
and that it is not a bug to say:
return ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
(n1 > n2 && (n1 - n2 > SNA32_CHECK)));
when you mean: