Signed-off-by: Jarek Poplawski <[email protected]>
---
diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
--- 2.6.21-mm1-/kernel/timer.c 2007-05-08 11:54:48.000000000 +0200
+++ 2.6.21-mm1/kernel/timer.c 2007-05-08 12:05:11.000000000 +0200
@@ -92,24 +92,24 @@ static DEFINE_PER_CPU(tvec_base_t *, tve
/* Functions below help us manage 'deferrable' flag */
static inline unsigned int tbase_get_deferrable(tvec_base_t *base)
{
- return ((unsigned int)(unsigned long)base & TBASE_DEFERRABLE_FLAG);
+ return (unsigned int)((unsigned long)base & TBASE_DEFERRABLE_FLAG);
}
static inline tvec_base_t *tbase_get_base(tvec_base_t *base)
{
- return ((tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG));
+ return (tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG);
}
static inline void timer_set_deferrable(struct timer_list *timer)
{
- timer->base = ((tvec_base_t *)((unsigned long)(timer->base) |
- TBASE_DEFERRABLE_FLAG));
+ timer->base = (tvec_base_t *)((unsigned long)timer->base |
+ TBASE_DEFERRABLE_FLAG);
}
static inline void
timer_set_base(struct timer_list *timer, tvec_base_t *new_base)
{
- timer->base = (tvec_base_t *)((unsigned long)(new_base) |
+ timer->base = (tvec_base_t *)((unsigned long)new_base |
tbase_get_deferrable(timer->base));
}
On Tue, 8 May 2007 12:33:48 +0200
Jarek Poplawski <[email protected]> wrote:
>
> Signed-off-by: Jarek Poplawski <[email protected]>
>
> ---
>
> diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
> --- 2.6.21-mm1-/kernel/timer.c 2007-05-08 11:54:48.000000000 +0200
> +++ 2.6.21-mm1/kernel/timer.c 2007-05-08 12:05:11.000000000 +0200
> @@ -92,24 +92,24 @@ static DEFINE_PER_CPU(tvec_base_t *, tve
> /* Functions below help us manage 'deferrable' flag */
> static inline unsigned int tbase_get_deferrable(tvec_base_t *base)
> {
> - return ((unsigned int)(unsigned long)base & TBASE_DEFERRABLE_FLAG);
> + return (unsigned int)((unsigned long)base & TBASE_DEFERRABLE_FLAG);
> }
>
> static inline tvec_base_t *tbase_get_base(tvec_base_t *base)
> {
> - return ((tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG));
> + return (tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG);
> }
>
> static inline void timer_set_deferrable(struct timer_list *timer)
> {
> - timer->base = ((tvec_base_t *)((unsigned long)(timer->base) |
> - TBASE_DEFERRABLE_FLAG));
> + timer->base = (tvec_base_t *)((unsigned long)timer->base |
> + TBASE_DEFERRABLE_FLAG);
> }
>
> static inline void
> timer_set_base(struct timer_list *timer, tvec_base_t *new_base)
> {
> - timer->base = (tvec_base_t *)((unsigned long)(new_base) |
> + timer->base = (tvec_base_t *)((unsigned long)new_base |
> tbase_get_deferrable(timer->base));
> }
>
The change makes sense, but does it actually "fix" anything?
On Tue, May 08, 2007 at 04:33:58PM -0700, Andrew Morton wrote:
> On Tue, 8 May 2007 12:33:48 +0200
> Jarek Poplawski <[email protected]> wrote:
>
> >
> > Signed-off-by: Jarek Poplawski <[email protected]>
> >
> > ---
> >
> > diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
> > --- 2.6.21-mm1-/kernel/timer.c 2007-05-08 11:54:48.000000000 +0200
> > +++ 2.6.21-mm1/kernel/timer.c 2007-05-08 12:05:11.000000000 +0200
> > @@ -92,24 +92,24 @@ static DEFINE_PER_CPU(tvec_base_t *, tve
> > /* Functions below help us manage 'deferrable' flag */
> > static inline unsigned int tbase_get_deferrable(tvec_base_t *base)
> > {
> > - return ((unsigned int)(unsigned long)base & TBASE_DEFERRABLE_FLAG);
> > + return (unsigned int)((unsigned long)base & TBASE_DEFERRABLE_FLAG);
> > }
...
> The change makes sense, but does it actually "fix" anything?
>
Yes - this first place fixes logical error, so it's a sin
- even if not punishable in practice. (It's also unnecessary
test for long to int conversion.)
Other changes are only BTW.
Regards,
Jarek P.
>-----Original Message-----
>From: Jarek Poplawski [mailto:[email protected]]
>Sent: Tuesday, May 08, 2007 10:32 PM
>To: Andrew Morton
>Cc: Pallipadi, Venkatesh; [email protected]; Oleg Nesterov
>Subject: Re: [PATCH -mm] timer: parenthesis fix in
>tbase_get_deferrable() etc.
>
>On Tue, May 08, 2007 at 04:33:58PM -0700, Andrew Morton wrote:
>> On Tue, 8 May 2007 12:33:48 +0200
>> Jarek Poplawski <[email protected]> wrote:
>>
>> >
>> > Signed-off-by: Jarek Poplawski <[email protected]>
>> >
>> > ---
>> >
>> > diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
>> > --- 2.6.21-mm1-/kernel/timer.c 2007-05-08
>11:54:48.000000000 +0200
>> > +++ 2.6.21-mm1/kernel/timer.c 2007-05-08
>12:05:11.000000000 +0200
>> > @@ -92,24 +92,24 @@ static DEFINE_PER_CPU(tvec_base_t *, tve
>> > /* Functions below help us manage 'deferrable' flag */
>> > static inline unsigned int tbase_get_deferrable(tvec_base_t *base)
>> > {
>> > - return ((unsigned int)(unsigned long)base &
>TBASE_DEFERRABLE_FLAG);
>> > + return (unsigned int)((unsigned long)base &
>TBASE_DEFERRABLE_FLAG);
>> > }
>...
>> The change makes sense, but does it actually "fix" anything?
>>
>
>Yes - this first place fixes logical error, so it's a sin
>- even if not punishable in practice. (It's also unnecessary
>test for long to int conversion.)
>
I am sorry, I don't understand. What is the logical error in the first
one?
Actually, your change makes it different from what was originally
indended.
Original intention was to type convert base to a 32 bit value and
bitwise& with FLAG. Even though compiler may optimize both the above to
same code, I don't see what is the error.
Thanks,
Venki
On 5/9/07, Pallipadi, Venkatesh <[email protected]> wrote:
>
> >-----Original Message-----
> >From: Jarek Poplawski [mailto:[email protected]]
> >Sent: Tuesday, May 08, 2007 10:32 PM
> >To: Andrew Morton
> >Cc: Pallipadi, Venkatesh; [email protected]; Oleg Nesterov
> >Subject: Re: [PATCH -mm] timer: parenthesis fix in
> >tbase_get_deferrable() etc.
> >
> >On Tue, May 08, 2007 at 04:33:58PM -0700, Andrew Morton wrote:
> >> On Tue, 8 May 2007 12:33:48 +0200
> >> Jarek Poplawski <[email protected]> wrote:
> >>
> >> >
> >> > Signed-off-by: Jarek Poplawski <[email protected]>
> >> >
> >> > ---
> >> >
> >> > diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
> >> > --- 2.6.21-mm1-/kernel/timer.c 2007-05-08
> >11:54:48.000000000 +0200
> >> > +++ 2.6.21-mm1/kernel/timer.c 2007-05-08
> >12:05:11.000000000 +0200
> >> > @@ -92,24 +92,24 @@ static DEFINE_PER_CPU(tvec_base_t *, tve
> >> > /* Functions below help us manage 'deferrable' flag */
> >> > static inline unsigned int tbase_get_deferrable(tvec_base_t *base)
> >> > {
> >> > - return ((unsigned int)(unsigned long)base &
> >TBASE_DEFERRABLE_FLAG);
> >> > + return (unsigned int)((unsigned long)base &
> >TBASE_DEFERRABLE_FLAG);
> >> > }
> >...
> >> The change makes sense, but does it actually "fix" anything?
> >>
> >
> >Yes - this first place fixes logical error, so it's a sin
> >- even if not punishable in practice. (It's also unnecessary
> >test for long to int conversion.)
> >
>
> I am sorry, I don't understand. What is the logical error in the first
> one?
>
> Actually, your change makes it different from what was originally
> indended.
> Original intention was to type convert base to a 32 bit value and
> bitwise& with FLAG.
But that is not what the original code is doing. If you wanted to
typecast "base" to "a 32 bit value" then you should've used u32
instead.
Anyway, if you originally intended to actually typecast "base" to
unsigned int, then you could do that directly without typecasting it
first to unsigned long (unnecessarily) and then to unsigned int. Of
course, if your system implements a pointer as something bigger than
unsigned int (which is what you eventually convert "base" to), then
you're screwed anyway and the intermediate typecast to unsigned long
doesn't buy you anything at all.
The other 3 changes in this patch were clearly meaningless, though.
>-----Original Message-----
>From: Satyam Sharma [mailto:[email protected]]
>Sent: Wednesday, May 09, 2007 11:30 AM
>To: Pallipadi, Venkatesh
>Cc: Jarek Poplawski; Andrew Morton;
>[email protected]; Oleg Nesterov
>Subject: Re: [PATCH -mm] timer: parenthesis fix in
>tbase_get_deferrable() etc.
>
>On 5/9/07, Pallipadi, Venkatesh <[email protected]> wrote:
>>
>> >-----Original Message-----
>> >From: Jarek Poplawski [mailto:[email protected]]
>> >Sent: Tuesday, May 08, 2007 10:32 PM
>> >To: Andrew Morton
>> >Cc: Pallipadi, Venkatesh; [email protected];
>Oleg Nesterov
>> >Subject: Re: [PATCH -mm] timer: parenthesis fix in
>> >tbase_get_deferrable() etc.
>> >
>> >On Tue, May 08, 2007 at 04:33:58PM -0700, Andrew Morton wrote:
>> >> On Tue, 8 May 2007 12:33:48 +0200
>> >> Jarek Poplawski <[email protected]> wrote:
>> >>
>> >> >
>> >> > Signed-off-by: Jarek Poplawski <[email protected]>
>> >> >
>> >> > ---
>> >> >
>> >> > diff -Nurp 2.6.21-mm1-/kernel/timer.c 2.6.21-mm1/kernel/timer.c
>> >> > --- 2.6.21-mm1-/kernel/timer.c 2007-05-08
>> >11:54:48.000000000 +0200
>> >> > +++ 2.6.21-mm1/kernel/timer.c 2007-05-08
>> >12:05:11.000000000 +0200
>> >> > @@ -92,24 +92,24 @@ static DEFINE_PER_CPU(tvec_base_t *, tve
>> >> > /* Functions below help us manage 'deferrable' flag */
>> >> > static inline unsigned int
>tbase_get_deferrable(tvec_base_t *base)
>> >> > {
>> >> > - return ((unsigned int)(unsigned long)base &
>> >TBASE_DEFERRABLE_FLAG);
>> >> > + return (unsigned int)((unsigned long)base &
>> >TBASE_DEFERRABLE_FLAG);
>> >> > }
>> >...
>> >> The change makes sense, but does it actually "fix" anything?
>> >>
>> >
>> >Yes - this first place fixes logical error, so it's a sin
>> >- even if not punishable in practice. (It's also unnecessary
>> >test for long to int conversion.)
>> >
>>
>> I am sorry, I don't understand. What is the logical error in
>the first
>> one?
>>
>> Actually, your change makes it different from what was originally
>> indended.
>> Original intention was to type convert base to a 32 bit value and
>> bitwise& with FLAG.
>
>But that is not what the original code is doing. If you wanted to
>typecast "base" to "a 32 bit value" then you should've used u32
>instead.
>
>Anyway, if you originally intended to actually typecast "base" to
>unsigned int, then you could do that directly without typecasting it
>first to unsigned long (unnecessarily) and then to unsigned int. Of
>course, if your system implements a pointer as something bigger than
>unsigned int (which is what you eventually convert "base" to), then
>you're screwed anyway and the intermediate typecast to unsigned long
>doesn't buy you anything at all.
On a 64 bit system, converting pointer to int causes unnecessary
compiler
warning, and intermediate long conversion was to avoid that. I will have
to rephrase my comment to remove 32 bit value and use int, as that is
what
the function returns.
Thanks,
Venki
On Wed, May 09, 2007 at 11:59:39PM +0530, Satyam Sharma wrote:
> On 5/9/07, Pallipadi, Venkatesh <[email protected]> wrote:
> >
> >>-----Original Message-----
> >>From: Jarek Poplawski [mailto:[email protected]]
> >>Sent: Tuesday, May 08, 2007 10:32 PM
> >>To: Andrew Morton
> >>Cc: Pallipadi, Venkatesh; [email protected]; Oleg Nesterov
> >>Subject: Re: [PATCH -mm] timer: parenthesis fix in
> >>tbase_get_deferrable() etc.
> >>
> >>On Tue, May 08, 2007 at 04:33:58PM -0700, Andrew Morton wrote:
> >>> On Tue, 8 May 2007 12:33:48 +0200
> >>> Jarek Poplawski <[email protected]> wrote:
...
> >>> > static inline unsigned int tbase_get_deferrable(tvec_base_t *base)
> >>> > {
> >>> > - return ((unsigned int)(unsigned long)base &
> >>TBASE_DEFERRABLE_FLAG);
> >>> > + return (unsigned int)((unsigned long)base &
> >>TBASE_DEFERRABLE_FLAG);
> >>> > }
> >>...
> >>> The change makes sense, but does it actually "fix" anything?
> >>>
> >>
> >>Yes - this first place fixes logical error, so it's a sin
> >>- even if not punishable in practice. (It's also unnecessary
> >>test for long to int conversion.)
> >>
> >
> >I am sorry, I don't understand. What is the logical error in the first
> >one?
I am sorry, too - for my "logic". It seems it's all correct!
(Except, I don't know what's going here...)
> >
> >Actually, your change makes it different from what was originally
> >indended.
> >Original intention was to type convert base to a 32 bit value and
> >bitwise& with FLAG.
>
> But that is not what the original code is doing. If you wanted to
> typecast "base" to "a 32 bit value" then you should've used u32
> instead.
>
> Anyway, if you originally intended to actually typecast "base" to
> unsigned int, then you could do that directly without typecasting it
> first to unsigned long (unnecessarily) and then to unsigned int. Of
> course, if your system implements a pointer as something bigger than
> unsigned int (which is what you eventually convert "base" to), then
> you're screwed anyway and the intermediate typecast to unsigned long
> doesn't buy you anything at all.
>
> The other 3 changes in this patch were clearly meaningless, though.
>
((unsigned int)(unsigned long)base ...
((tvec_base_t *)((unsigned long)base ...
((tvec_base_t *)((unsigned long)(timer->base) ...
(tvec_base_t *)((unsigned long)(new_base) ...
Yes, if you don't count reading this one close each other, they are
clearly meaningles.
Regards,
Jarek P.
On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
...
> On a 64 bit system, converting pointer to int causes unnecessary
> compiler
> warning, and intermediate long conversion was to avoid that. I will have
> to rephrase my comment to remove 32 bit value and use int, as that is
> what
> the function returns.
Sorry!!! So, it's perfectly right and logical.
It's a pity, you couldn't NACK this patch a little sooner.
I hope there is no problem to remove this patch now.
It seems this type of conversion isn't popular enough, yet.
Thanks for explanation & regards,
Jarek P.
On Thu, 10 May 2007 09:39:04 +0200 Jarek Poplawski <[email protected]> wrote:
> On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
> ...
> > On a 64 bit system, converting pointer to int causes unnecessary
> > compiler
> > warning, and intermediate long conversion was to avoid that. I will have
> > to rephrase my comment to remove 32 bit value and use int, as that is
> > what
> > the function returns.
>
> Sorry!!! So, it's perfectly right and logical.
>
> It's a pity, you couldn't NACK this patch a little sooner.
> I hope there is no problem to remove this patch now.
Please send a new patch. That way we get a nice explanation for the
reversion, and we have well-oiled processes for applying patches, but
crappy processes for reverting them.
> >> >> > static inline unsigned int
> >tbase_get_deferrable(tvec_base_t *base)
> >> >> > {
> >> >> > - return ((unsigned int)(unsigned long)base &
> >> >TBASE_DEFERRABLE_FLAG);
> >> >> > + return (unsigned int)((unsigned long)base &
> >> >TBASE_DEFERRABLE_FLAG);
> >> >> > }
> >> >...
> >> >> The change makes sense, but does it actually "fix" anything?
> >> >>
> >> >
> >> >Yes - this first place fixes logical error, so it's a sin
> >> >- even if not punishable in practice. (It's also unnecessary
> >> >test for long to int conversion.)
> >> >
> >>
> >> I am sorry, I don't understand. What is the logical error in
> >the first
> >> one?
> >>
> >> Actually, your change makes it different from what was originally
> >> indended.
> >> Original intention was to type convert base to a 32 bit value and
> >> bitwise& with FLAG.
> >
> >But that is not what the original code is doing. If you wanted to
> >typecast "base" to "a 32 bit value" then you should've used u32
> >instead.
> >
> >Anyway, if you originally intended to actually typecast "base" to
> >unsigned int, then you could do that directly without typecasting it
> >first to unsigned long (unnecessarily) and then to unsigned int. Of
> >course, if your system implements a pointer as something bigger than
> >unsigned int (which is what you eventually convert "base" to), then
> >you're screwed anyway and the intermediate typecast to unsigned long
> >doesn't buy you anything at all.
>
> On a 64 bit system, converting pointer to int causes unnecessary
> compiler
> warning, and intermediate long conversion was to avoid that. I will have
Whoa! Hello, hold on, just wait a second there. Do you _really_ want
an unsigned int return out of tbase_get_deferrable() or will an
unsigned long do? If the rest of your code is fine with unsigned long,
then I'd suggest something like:
static inline unsigned long tbase_get_deferrable(tvec_base_t *base)
{
return ((unsigned long)base & TBASE_DEFERRABLE_FLAG);
}
I don't really know your code (so I could be horribly incorrect here),
but personally I would prefer *heeding* to that warning than _hiding_
it -- it's not unnecessary, it's telling you that you're *losing* data
by converting a pointer (which is always unsigned long) to unsigned
int for 64-bit platforms where sizeof(void *) == sizeof(unsigned long)
== 8 bytes, but sizeof(unsigned int) == 4.
On Thu, May 10, 2007 at 12:57:38AM -0700, Andrew Morton wrote:
> On Thu, 10 May 2007 09:39:04 +0200 Jarek Poplawski <[email protected]> wrote:
>
> > On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
> > ...
> > > On a 64 bit system, converting pointer to int causes unnecessary
> > > compiler
> > > warning, and intermediate long conversion was to avoid that. I will have
> > > to rephrase my comment to remove 32 bit value and use int, as that is
> > > what
> > > the function returns.
> >
> > Sorry!!! So, it's perfectly right and logical.
> >
> > It's a pity, you couldn't NACK this patch a little sooner.
> > I hope there is no problem to remove this patch now.
>
> Please send a new patch. That way we get a nice explanation for the
> reversion, and we have well-oiled processes for applying patches, but
> crappy processes for reverting them.
>
--->
[PATCH] timer: revert parenthesis fix in tbase_get_deferrable() etc.
> > On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
> > ...
> > > On a 64 bit system, converting pointer to int causes unnecessary
> > > compiler
> > > warning, and intermediate long conversion was to avoid that. I will have
> > > to rephrase my comment to remove 32 bit value and use int, as that is
> > > what
> > > the function returns.
So, this patch reverts all changes done by my previous patch.
I appologize for my wrong comment about "logical error" here.
Cc: "Pallipadi, Venkatesh" <[email protected]>
Cc: Satyam Sharma <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Signed-off-by: Jarek Poplawski <[email protected]>
---
diff -Nurp 2.6.21-git12-/kernel/timer.c 2.6.21-git12/kernel/timer.c
--- 2.6.21-git12-/kernel/timer.c 2007-05-10 10:55:34.000000000 +0200
+++ 2.6.21-git12/kernel/timer.c 2007-05-10 11:02:36.000000000 +0200
@@ -92,24 +92,24 @@ static DEFINE_PER_CPU(tvec_base_t *, tve
/* Functions below help us manage 'deferrable' flag */
static inline unsigned int tbase_get_deferrable(tvec_base_t *base)
{
- return (unsigned int)((unsigned long)base & TBASE_DEFERRABLE_FLAG);
+ return ((unsigned int)(unsigned long)base & TBASE_DEFERRABLE_FLAG);
}
static inline tvec_base_t *tbase_get_base(tvec_base_t *base)
{
- return (tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG);
+ return ((tvec_base_t *)((unsigned long)base & ~TBASE_DEFERRABLE_FLAG));
}
static inline void timer_set_deferrable(struct timer_list *timer)
{
- timer->base = (tvec_base_t *)((unsigned long)timer->base |
- TBASE_DEFERRABLE_FLAG);
+ timer->base = ((tvec_base_t *)((unsigned long)(timer->base) |
+ TBASE_DEFERRABLE_FLAG));
}
static inline void
timer_set_base(struct timer_list *timer, tvec_base_t *new_base)
{
- timer->base = (tvec_base_t *)((unsigned long)new_base |
+ timer->base = (tvec_base_t *)((unsigned long)(new_base) |
tbase_get_deferrable(timer->base));
}
On Thu, May 10, 2007 at 11:29:47AM +0200, Jarek Poplawski wrote:
...
> [PATCH] timer: revert parenthesis fix in tbase_get_deferrable() etc.
>
> > > On 09-05-2007 21:10, Pallipadi, Venkatesh wrote:
> > > ...
> > > > On a 64 bit system, converting pointer to int causes unnecessary
> > > > compiler
> > > > warning, and intermediate long conversion was to avoid that. I will have
> > > > to rephrase my comment to remove 32 bit value and use int, as that is
> > > > what
> > > > the function returns.
>
> So, this patch reverts all changes done by my previous patch.
>
> I appologize for my wrong comment about "logical error" here.
Should be:
I apologize for my wrong comment about "logical error" here.
Jarek P.
PS: Why I couldn't be sorry, as usual...
On 5/10/07, Satyam Sharma <[email protected]> wrote:
> > [...]
> > On a 64 bit system, converting pointer to int causes unnecessary
> > compiler
> > warning, and intermediate long conversion was to avoid that. I will have
>
> Whoa! Hello, hold on, just wait a second there. Do you _really_ want
> an unsigned int return out of tbase_get_deferrable() or will an
> unsigned long do? If the rest of your code is fine with unsigned long,
> then I'd suggest something like:
>
> static inline unsigned long tbase_get_deferrable(tvec_base_t *base)
> {
> return ((unsigned long)base & TBASE_DEFERRABLE_FLAG);
> }
>
> I don't really know your code (so I could be horribly incorrect here),
> but personally I would prefer *heeding* to that warning than _hiding_
> it -- it's not unnecessary, it's telling you that you're *losing* data
> by converting a pointer (which is always unsigned long) to unsigned
> int for 64-bit platforms where sizeof(void *) == sizeof(unsigned long)
> == 8 bytes, but sizeof(unsigned int) == 4.
Oh well, pardon my obtuseness (I _really_ ought to be at least looking
around in the code before jumping in like this :-) ...
TBASE_DEFERRABLE_FLAG only {ab}uses the lowest bit of tvec_base_t *,
so clearly no pointer truncation issues here (you could still prefer
the unsigned long version for simplicity & style, though).
*embarrassed, goes for a cup of coffee*