On Mon, 2008-07-07 at 13:00 +0200, John Kacur wrote: > On Sat, Jul 5, 2008 at 5:18 PM, Darren Hart <dvhltc@xxxxxxxxxx> wrote: > > On Fri, 2008-07-04 at 15:08 +0200, John Kacur wrote: > >> Since we're always being admonished to do code-review, and I needed to > >> waste some time, here are a few comments, to what looks largely like a > >> nice patch to me. > > > > Hi John, thanks for the review. Note that this is a backport of Peter'z > > git patch, so I'll try to address some of your rationale questions > > below, but the short answer is "I tried to match Peter's implementation > > as closely as possible." Regarding the patch applying, it worked for > > me... see below. > > > >> > >> On Fri, Jul 4, 2008 at 12:41 AM, Darren Hart <dvhltc@xxxxxxxxxx> wrote: > >> > Enqueue deprioritized RT tasks to head of prio array > >> > > >> > This patch backports Peter Z's enqueue to head of prio array on > >> > de-prioritization to 2.6.24.7-rt14 which doesn't have the > >> > enqueue_rt_entity and associated changes. > >> > > >> > I've run several long running real-time java benchmarks and it's > >> > holding so far. Steven, please consider this patch for inclusion > >> > in the next 2.6.24.7-rtX release. > >> > > >> > Peter, I didn't include your Signed-off-by as only about half your > >> > original patch applied to 2.6.24.7-r14. If you're happy with this > >> > version, would you also sign off? > >> > > >> > Signed-off-by: Darren Hart <dvhltc@xxxxxxxxxx> > >> > > >> > > >> > --- > >> > Index: linux-2.6.24.7-ibmrt2.6-view/include/linux/sched.h > >> > =================================================================== > >> > --- linux-2.6.24.7-ibmrt2.6-view.orig/include/linux/sched.h > >> > +++ linux-2.6.24.7-ibmrt2.6-view/include/linux/sched.h > >> > @@ -897,11 +897,16 @@ struct uts_namespace; > >> > struct rq; > >> > struct sched_domain; > >> > > >> > +#define ENQUEUE_WAKEUP 0x01 > >> > +#define ENQUEUE_HEAD 0x02 > >> > + > >> > +#define DEQUEUE_SLEEP 0x01 > >> > + > >> > >> Question: is ENQUEUE_WAKEUP equal to DEQUEUE_SLEEP by design or > >> coincidence? > > > > Coincidence. The ENQUEUE_* flags are only to be used with the > > enqueue_task* methods, while the DEQUEUE_* flags are for deqeue_task*. > > Note that the conversion of sleep to the DEQUEUE_SLEEP flag isn't really > > necessary as there is only the one flag, but it makes the calls > > parallel, which I suspect was Peter's intention (but I speculate here). > > > >> The renaming of wakeup and sleep to flags makes it at > >> least superficially seem like they overlap. Since a large part of the > >> patch is renaming, it might be easier to understand if the renaming > >> was done as a separate patch, but on the other hand, that is probably > >> just a PITA. :) > > > > Seems a small enough patch to be all in one to me. If others object > > I'll split it out, but again, I tried to keep the backport as close to > > Peter's original patch as possible. > ----SNIP----- > I'm not sure the renaming is necessary at all, since "wakeup" and > "sleep" seem more descriptive than "flags". If you skip the renaming > then the size of the patch is reduced to half it's current size. The renaming isn't strictly necessary, but the name "flags" denotes that there are multiple value that can be passed whereas "wakeup" is binary - either on or off. Since ENQUEUE_HEAD has nothing to do with "wakeup" it doesn't make since to reuse the old name. Changing the dequeue* routines makes the related calls parallel in signature and allows for similar changes in the future. Also, as this is a backport, I would rather not try and change it here only to have it revert back to Peter's original in the upstream -rt tree. Thanks, -- Darren Hart Real-Time Linux Team Lead IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
