On 16.08.2005 [17:39:11 -0700], George Anzinger wrote:
> Nishanth Aravamudan wrote:
> >On 04.08.2005 [09:45:55 -0700], George Anzinger wrote:
> >
> >>Uh... PLEASE tell me you are NOT changing timespec_to_jiffies() (and
> >>timeval_to_jiffies() to add 1. This is NOT the right thing to do. For
> >>repeating times (see setitimer code) we need the actual time as we KNOW
> >>where the jiffies edge is in the repeating case. The +1 is needed ONLY
> >>for the initial time, not the repeating time.
> >>
> >>
> >>See:
> >>http://marc.theaimsgroup.com/?l=linux-kernel&m=112208357906156&w=2
> >
> >
> >I followed that thread, George, but I think it's a different case with
> >schedule_timeout() [maybe this indicates drivers/other users should
> >maybe be using itimers, but I'll get to that in a sec].
>
> I think I miss understood back then :).
Ok, no problem. I was just thinking today about all the issues that were
borught up... I really appreciate your feedback.
> >
> >With schedule_timeout(), we are just given a relative jiffies value. We
> >have no context as to which task is requesting the delay, per se,
> >meaning we don't (can't) know from the interface whether this is the
> >first delay in a sequence, or a brand new one, without changing all
> >users to have some sort of control structure. The callers of
> >schedule_timeout() don't even get a pointer to the timer added
> >internally.
> >
> >So, adding 1 to all sleeps seems like it might be reasonable, as looping
> >sleeps probably need to use a different interface. I had worked a bit
> >ago on something like poll_event() with the kernel-janitors group, which
> >would abstract out the repeated sleeps. Basically wait_event() without
> >wait-queues... Maybe we could make such an interface just use itimers?
> >I've attached my old patch for poll_event(), just for reference.
>
> I think not. itimers is really pointed at a particular system call and
> has resources in the task structure to do it. These would be hard to
> share...
Ok, I wasn't sure about that -- I'm not too familiar with the itimer
code (maybe I'll take a look at it soon :) )
> >My point, I guess, is that in the schedule_timeout() case, we don't know
> >where the jiffies edge is, as we either expire or receive a wait-queue
> >event/signal, we never mod_timer() the internal timer... So we have to
> >assume that we need to sleep the request. But maybe Roman's idea of
> >sleeping a certain number of jiffy edges is sufficient. I am not yet
> >convinced driver authors want/need such an interface, though, still
> >thinking it over.
>
> IMNSHO we should not get too parental with kernel only interfaces.
> Adding 1 is easy enough for the caller and even easier to explain in the
> instructions (i.e. this call sleeps for X jiffies edges). This allows
> the caller to do more if needed and, should he ever just want to sync to
> the next jiffie he does not have to deal with backing out that +1.
I don't want to be too parental either, but I also am trying to avoid
code duplication. Lots of drivers basically do something like
poll_event() does (or could do with some changes), i.e. looping a
constant amount multiple times, checking something every so often. The
patch was just a thought, though. I will keep evaluating drivers and see
if it's a useful interface to have eventually.
I guess I'm just concerned with making an unintuitive interface. As was
brought up at OLS, drivers are a major source of bugs/buggy code. The
simpler, more useful we can make interfaces, the better, I think. I'm
not claiming you disagree, I just want to make my own motives clear.
While fixing up the schedule_timeout() comment would make it clear what
schedule_timeout() achieves, I'm not sure how useful such an interface
is, if every caller adds 1 :) I need to mull it over, though... Lots to
consider. I also, of course, want to stay flexible for the reasons you
mention (letting the driver adjust the timeout as they expect to).
Thanks,
Nish
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- References:
- Re: [PATCH] Add schedule_timeout_{interruptible,uninterruptible}{,_msecs}() interfaces
- Re: [PATCH] Add schedule_timeout_{interruptible,uninterruptible}{,_msecs}() interfaces
- Re: [PATCH] Add schedule_timeout_{interruptible,uninterruptible}{,_msecs}() interfaces
- [UPDATE PATCH] Add schedule_timeout_{intr,unintr}{,_msecs}() interfaces
- Re: [UPDATE PATCH] Add schedule_timeout_{intr,unintr}{,_msecs}() interfaces
- [PATCH] push rounding up of relative request to schedule_timeout()
- [UPDATE PATCH] push rounding up of relative request to schedule_timeout()
- Re: [UPDATE PATCH] push rounding up of relative request to schedule_timeout()
- Re: [UPDATE PATCH] push rounding up of relative request to schedule_timeout()
- Re: [UPDATE PATCH] push rounding up of relative request to schedule_timeout()
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |