Re: Antw: [PATCH] iscsi: Simplify serial number comparisons
On Apr 8, 2011, at 10:28 AM, Paul Koning wrote: > On Apr 8, 2011, at 1:22 PM, Rustad, Mark D wrote: > >> Ulrich, >> >> On Apr 7, 2011, at 11:35 PM, Ulrich Windl wrote: >> >>> I just wonder how safe the code is: >>> >>> Doesn't the difference of two unsigned ints give an unsigned value? The >>> assigning an unsigned int to a signed int will definitely reduce the >>> range... >> >> Actually, that isn't true. There are 2^32 possible values either way. The >> only difference is the interpretation. >> >>> I feel that >>> s32 diff = (s32) n1 - (s32) n2; >> >> Adding the casts does basically nothing here. They serve no purpose. In >> fact, the original expresses the intent perfectly: subtract two unsigned >> integers and treat the result as signed. That is precisely what is wanted. I >> used the local variable, instead of casting, to be as crystal clear about >> that as possible. > > Actually, casting n1 and n2 to signed is a bad idea. From what I remember, > unsigned arithmetic is defined to be free of overflow; signed arithmetic can > have overflow checks if the compiler and/or hardware chooses to do that. The > whole idea here is to rely on the wrapping properties of two's complement > arithmetic modulo 2^n; what you really do *not* want is an overflow trap when > that wrap occurs. Instead you want to do the wrap silently, which is what an > unsigned subtract does. That is a very good point, Paul. Some environments could do that. Good catch on my comments. -- Mark Rustad, LAN Access Division, Intel Corporation -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com. To unsubscribe from this group, send email to open-iscsi+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/open-iscsi?hl=en.
Re: Antw: [PATCH] iscsi: Simplify serial number comparisons
On Apr 8, 2011, at 1:22 PM, Rustad, Mark D wrote: > Ulrich, > > On Apr 7, 2011, at 11:35 PM, Ulrich Windl wrote: > >> I just wonder how safe the code is: >> >> Doesn't the difference of two unsigned ints give an unsigned value? The >> assigning an unsigned int to a signed int will definitely reduce the range... > > Actually, that isn't true. There are 2^32 possible values either way. The > only difference is the interpretation. > >> I feel that >> s32 diff = (s32) n1 - (s32) n2; > > Adding the casts does basically nothing here. They serve no purpose. In fact, > the original expresses the intent perfectly: subtract two unsigned integers > and treat the result as signed. That is precisely what is wanted. I used the > local variable, instead of casting, to be as crystal clear about that as > possible. Actually, casting n1 and n2 to signed is a bad idea. From what I remember, unsigned arithmetic is defined to be free of overflow; signed arithmetic can have overflow checks if the compiler and/or hardware chooses to do that. The whole idea here is to rely on the wrapping properties of two's complement arithmetic modulo 2^n; what you really do *not* want is an overflow trap when that wrap occurs. Instead you want to do the wrap silently, which is what an unsigned subtract does. paul -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com. To unsubscribe from this group, send email to open-iscsi+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/open-iscsi?hl=en.
Re: Antw: [PATCH] iscsi: Simplify serial number comparisons
Ulrich, On Apr 7, 2011, at 11:35 PM, Ulrich Windl wrote: > I just wonder how safe the code is: > > Doesn't the difference of two unsigned ints give an unsigned value? The > assigning an unsigned int to a signed int will definitely reduce the range... Actually, that isn't true. There are 2^32 possible values either way. The only difference is the interpretation. > I feel that > s32 diff = (s32) n1 - (s32) n2; Adding the casts does basically nothing here. They serve no purpose. In fact, the original expresses the intent perfectly: subtract two unsigned integers and treat the result as signed. That is precisely what is wanted. I used the local variable, instead of casting, to be as crystal clear about that as possible. > also doesn't make the problem go away, unless ypou promote an unsigned 32 bit > value to a signed 64 bit value. If you are really thinking in assembler, you > should add comments on that. The code looks dubious to me at least. Actually, introducing 64-bits just makes a different sort of complicated mess. You really want the 32-bit over/under-flow to happen naturally in this case. You see, although the counters are unsigned, their recycling nature makes them something other then true unsigned numbers. That is the reason that it gets so complicated to do it all in an unsigned domain. The whole point of these kinds of incrementing values is to be able to tell what value comes before or after another. That is all. The subtraction calculates the difference between two values, and if you take that difference and interpret it as a signed value, you get exactly what you want. It really is that simple. It is made complex by trying to do it all in an unsigned domain. If you imagine a circle with unsigned integers all around it counting up in a clockwise direction. Then choose an arbitrary two points on that circle. What you want to know is which one is ahead of the other. And you always want to use the shortest path between them to make that determination. The subtraction does precisely that. When you get a negative, that is how many points you have to go counterclockwise to get to the other point. When you get a positive, that is how many points you need to go clockwise. By definition it can never go the "long way" around, because such a number can't be represented in the field. The loss of precision is exactly what is wanted. To introduce a larger domain effectively places the 2^32 points on a longer line and creates confusion. In fact, the circle metaphor reveals the nature of these numbers. By contrast, pure unsigned numbers would be laid out on a line with definite ends. Doing the comparison in a purely unsigned domain is complicated because it tries to simulate connecting the ends of that line. Now, there could be architectural risks. For example a one's complement architecture would require special handling. But I'm not aware of any one's complement machines that run Linux. There could also be issues if there are architectures that don't do 32-bit math right. But I would expect this would be the least of the troubles that such machines would encounter trying to run Linux. It also would not work on BCD machines, but again, neither does Linux, or probably even C. I'd guess that I've probably programmed on most of the machines that this wouldn't work on. Also, I want to repeat that the only problem this change addresses is needless complexity. The code that is present works fine. But why work so hard when it is so easy? -- Mark Rustad, LAN Access Division, Intel Corporation -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com. To unsubscribe from this group, send email to open-iscsi+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/open-iscsi?hl=en.
Antw: [PATCH] iscsi: Simplify serial number comparisons
Hi!
I just wonder how safe the code is:
Doesn't the difference of two unsigned ints give an unsigned value? The
assigning an unsigned int to a signed int will definitely reduce the range...
I feel that
s32 diff = (s32) n1 - (s32) n2;
also doesn't make the problem go away, unless ypou promote an unsigned 32 bit
value to a signed 64 bit value. If you are really thinking in assembler, you
should add comments on that. The code looks dubious to me at least.
Regards,
Ulrich
>>> Mark Rustad schrieb am 07.04.2011 um 23:26 in
Nachricht <20110407212615.20399.18029.stgit@localhost6.localdomain6>:
> Unsigned serial number comparison is very simple if you simply put the
> difference into a signed integer of the same size and then compare that
> value with zero. All the complexity and confusion fall away.
>
> Signed-off-by: Mark Rustad
> ---
> include/scsi/iscsi_proto.h | 21 -
> 1 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
> index 0c6c1d6..98ebc3d 100644
> --- a/include/scsi/iscsi_proto.h
> +++ b/include/scsi/iscsi_proto.h
> @@ -35,30 +35,33 @@
> /*
> * Serial Number Arithmetic, 32 bits, RFC1982
> */
> -#define SNA32_CHECK 2147483648UL
>
> static inline int iscsi_sna_lt(u32 n1, u32 n2)
> {
> - return n1 != n2 && ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
> - (n1 > n2 && (n2 - n1 < SNA32_CHECK)));
> + s32 diff = n1 - n2;
> +
> + return diff < 0;
> }
>
> static inline int iscsi_sna_lte(u32 n1, u32 n2)
> {
> - return n1 == n2 || ((n1 < n2 && (n2 - n1 < SNA32_CHECK)) ||
> - (n1 > n2 && (n2 - n1 < SNA32_CHECK)));
> + s32 diff = n1 - n2;
> +
> + return diff <= 0;
> }
>
> static inline int iscsi_sna_gt(u32 n1, u32 n2)
> {
> - return n1 != n2 && (((n1 < n2) && ((n2 - n1) > SNA32_CHECK)) ||
> - ((n1 > n2) && ((n1 - n2) < SNA32_CHECK)));
> + s32 diff = n1 - n2;
> +
> + return diff > 0;
> }
>
> static inline int iscsi_sna_gte(u32 n1, u32 n2)
> {
> - return n1 == n2 || (((n1 < n2) && ((n2 - n1) > SNA32_CHECK)) ||
> - ((n1 > n2) && ((n1 - n2) < SNA32_CHECK)));
> + s32 diff = n1 - n2;
> +
> + return diff >= 0;
> }
>
> /*
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com.
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/open-iscsi?hl=en.
