[KJ] Fwd: [Patch] Add HZ values and time_after() macros in iphase.c

[KJ] Fwd: [Patch] Add HZ values and time_after() macros in iphase.c

[Darren Jenkins]

Forgot to reply to all!

---------- Forwarded message ----------
From: Darren Jenkins <darrenrjenkins@gmail.com>
Date: Aug 30, 2006 2:42 AM
Subject: Re: [KJ] [Patch] Add HZ values and time_after() macros in iphase.c
To: Nishanth Aravamudan <nacc@us.ibm.com>


G'day Nish

On 8/30/06, Nishanth Aravamudan <nacc@us.ibm.com> wrote:
> On 29.08.2006 [12:48:22 +1000], Darren Jenkins" wrote:
> > G'day list
> >
> > After grepping for schedual_timeout() -HZ, I found one in iphase.c.
> > After looking at it I also thought I would add some time_after() macro's
> > for clarification, and remove a variable in one case, that wasn't really
> > needed.
> >
> > Add HZ values and time_after() macro's in iphase.c
>
> Should be split in two patches.

Fair enough.

> > Compile tested with allyesconfig
> >
> >
> > Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
> >
> > --- drivers/atm/iphase.c.orig 2006-08-28 23:29:06.000000000 +1000
> > +++ drivers/atm/iphase.c      2006-08-29 01:07:30.000000000 +1000
> > @@ -184,12 +184,11 @@ static u16 get_desc (IADEV *dev, struct
> >    u_short            desc_num, i;
> >    struct sk_buff        *skb;
> >    struct ia_vcc         *iavcc_r = NULL;
> > -  unsigned long delta;
> >    static unsigned long timer = 0;
> >    int ltimeout;
> >
> >    ia_hack_tcq (dev);
> > -  if(((jiffies - timer)>50)||((dev->ffL.tcq_rd=dev->host_tcq_wr))){
> > +  if(time_after(jiffies, (timer+HZ/2))||((dev->ffL.tcq_rd=dev->host_tcq_wr))){
>
> You're changing the functionality here. It's best to keep the same
> semantics as much as possible, please. And I'd use msecs_to_jiffies().

sounds good.

> BTW, it's not a bug, per se, to only want to sleep for a fixed number of
> jiffies, even if I don't completely approve of it, either.

No not a bug per se. However if we arn't using a single jiffie(for a
min timeout), not making the call HZ relative will cause a bug with a
large enough change in HZ.
That is if the delay really was needed in the first place. :)

>
> >       timer = jiffies;
> >       i=0;
> >       while (i < dev->num_tx_desc) {
> > @@ -198,9 +197,10 @@ static u16 get_desc (IADEV *dev, struct
> >             continue;
> >          }
> >          ltimeout = dev->desc_tbl[i].iavcc->ltimeout;
> > -        delta = jiffies - dev->desc_tbl[i].timestamp;
> > -        if (delta >= ltimeout) {
> > -           IF_ABR(printk("RECOVER run!! desc_tbl %d = %d  delta = %ld, time = %ld\n", i,dev->desc_tbl[i].timestamp, delta, jiffies);)
> > +        if (time_after_eq(jiffies,
> > +             ((unsigned long)dev->desc_tbl[i].timestamp + ltimeout))) {
> > +           IF_ABR(printk("RECOVER run!! desc_tbl %d = %d, time = %ld\n",
> > +             i,dev->desc_tbl[i].timestamp, jiffies);)
> >             if (dev->ffL.tcq_rd = dev->ffL.tcq_st)
> >                dev->ffL.tcq_rd =  dev->ffL.tcq_ed;
> >             else
> > @@ -1223,7 +1223,7 @@ static void rx_intr(struct atm_dev *dev)
> >          iadev->rx_tmp_jif = jiffies;
> >          iadev->rxing = 0;
> >       }
> > -     else if (((jiffies - iadev->rx_tmp_jif) > 50) &&
> > +     else if (time_after(jiffies, ((unsigned long)iadev->rx_tmp_jif + HZ/2)) &&
> >                 ((iadev->rx_pkt_cnt - iadev->rx_tmp_cnt) = 0)) {
>
> Changes functionality.

Just wondering, do you mean it is changed because it is HZ/2 and not HZ/20 ?
(just remembered HZ in 2.6 is/was 1000 not 100 anymore)
or do you mean the adding of time_after() code change ?


>
> >          for (i = 1; i <= iadev->num_rx_desc; i++)
> >                 free_desc(dev, i);
> > @@ -2610,7 +2610,7 @@ static void ia_close(struct atm_vcc *vcc
> >          if (vcc->qos.txtp.traffic_class != ATM_NONE) {
> >             iadev->close_pending++;
> >          prepare_to_wait(&iadev->timeout_wait, &wait, TASK_UNINTERRUPTIBLE);
> > -        schedule_timeout(50);
> > +        schedule_timeout(HZ/2);
>
> Changes functionality.
>
> Thanks,
> Nish


Darren J.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Fwd: [Patch] Add HZ values and time_after() macros in

[Nishanth Aravamudan]

On 30.08.2006 [02:44:02 +1000], Darren Jenkins wrote:
> Forgot to reply to all!
> 
> ---------- Forwarded message ----------
> From: Darren Jenkins <darrenrjenkins@gmail.com>
> Date: Aug 30, 2006 2:42 AM
> Subject: Re: [KJ] [Patch] Add HZ values and time_after() macros in iphase.c
> To: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> 
> G'day Nish

<snip>

> > BTW, it's not a bug, per se, to only want to sleep for a fixed number of
> > jiffies, even if I don't completely approve of it, either.
> 
> No not a bug per se. However if we arn't using a single jiffie(for a
> min timeout), not making the call HZ relative will cause a bug with a
> large enough change in HZ.

Yes, that is true. I support your changes in general, but given that
HZ\x1000 in original 2.6, I would suggest that 50 jiffies is not the same
as HZ/2. It should either be HZ/20 or, preferably, msecs_to_jiffies(50).
If the maintainer then replies saying they had never changed the code
from 2.4's HZ to 2.6, then we can proceed accordingly and your current
change is more appropriate, but I wouldn't assume that to be the case.

> That is if the delay really was needed in the first place. :)

Always tough to tell :)

<snip>

> > > -     else if (((jiffies - iadev->rx_tmp_jif) > 50) &&
> > > +     else if (time_after(jiffies, ((unsigned long)iadev->rx_tmp_jif + HZ/2)) &&
> > >                 ((iadev->rx_pkt_cnt - iadev->rx_tmp_cnt) = 0)) {
> >
> > Changes functionality.
> 
> Just wondering, do you mean it is changed because it is HZ/2 and not HZ/20 ?
> (just remembered HZ in 2.6 is/was 1000 not 100 anymore)

Yes, exactly. You went from 50 jiffies (50 ms with the previous default
HZ) to 500 jiffies (500 ms with the previous default HZ), which is a
significant difference in time and may even introduce undesired behavior.

> or do you mean the adding of time_after() code change ?

No, that is fine. Direct subtraction from jiffies for comparision is
frowned upon.

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Fwd: [Patch] Add HZ values and time_after() macros in

[Matthew Wilcox]

On Tue, Aug 29, 2006 at 10:00:08AM -0700, Nishanth Aravamudan wrote:
> Yes, that is true. I support your changes in general, but given that
> HZ\x1000 in original 2.6, I would suggest that 50 jiffies is not the same
> as HZ/2. It should either be HZ/20 or, preferably, msecs_to_jiffies(50).
> If the maintainer then replies saying they had never changed the code
> from 2.4's HZ to 2.6, then we can proceed accordingly and your current
> change is more appropriate, but I wouldn't assume that to be the case.

It's easy enough to look at 2.4:

  if(((jiffies - timer)>50)||((dev->ffL.tcq_rd=dev->host_tcq_wr))){      

So the patch is a bugfix.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Fwd: [Patch] Add HZ values and time_after() macros in

[Nishanth Aravamudan]

On 29.08.2006 [11:30:58 -0600], Matthew Wilcox wrote:
> On Tue, Aug 29, 2006 at 10:00:08AM -0700, Nishanth Aravamudan wrote:
> > Yes, that is true. I support your changes in general, but given that
> > HZ\x1000 in original 2.6, I would suggest that 50 jiffies is not the same
> > as HZ/2. It should either be HZ/20 or, preferably, msecs_to_jiffies(50).
> > If the maintainer then replies saying they had never changed the code
> > from 2.4's HZ to 2.6, then we can proceed accordingly and your current
> > change is more appropriate, but I wouldn't assume that to be the case.
> 
> It's easy enough to look at 2.4:
> 
>   if(((jiffies - timer)>50)||((dev->ffL.tcq_rd=dev->host_tcq_wr))){      
> 
> So the patch is a bugfix.

Hrm, true.

So, Darren, if you want to update my patch given that information, feel
free.

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors