[PATCH] dix: Better document the functions Dispatch(), WaitForSomething() and alike.

[Fernando Carrijo]

Tiago Vignatti <tiago.vignatti at Nokia.com> wrote:

> On Wed, Jul 28, 2010 at 10:26:34PM +0200, ext Fernando Carrijo wrote:
> > +/* clientReady:
> > + *   array of clients ready for input and/or output. Actually, the
> > + *   array is not composed of clients, but of the indexes they occupy
> > + *   in the global variable clients.
> > + *
> > + * nread:
> > + *   number of items in the array
> > + */
> 
> not sure if people really use that but we may want to write with doxygen tags.
> You can take a look at dix/events.c for references.

Okay.

> >  static int
> >  SmartScheduleClient (int *clientReady, int nready)
> >  {
> > @@ -251,7 +259,7 @@ SmartScheduleClient (int *clientReady, int nready)
> >      long       now = SmartScheduleTime;
> >      long       idle;
> > 
> > -    bestPrio = -0x7fffffff;
> > +    bestPrio = -0x7fffffff; /* INT_MIN */
> 
> maybe put a "FIXME: change by INT_MIN" would be better?

Shall be fixed by the next version of the patch.

> > @@ -361,10 +379,20 @@ Dispatch(void)
> >      {
> >          if (*icheck[0] != *icheck[1])
> >         {
> > +           /* While dispatching, only two things can be more important than client
> > +            * scheduling: processing input events already present in the event queue,
> > +            * and flushing to clients any critically-deemed pending data. That's why
> > +            * we do this before going into WaitForSomething().
> > +            */
> 
> last phrase seems superfluous.

I'll remove it.

> > -       /*****************
> > -       *  Handle events in round robin fashion, doing input between
> > -       *  each round
> > -       *****************/
> > 
> > +       /* According to the Smart Scheduler's Dynamic Priorities Policy each
>                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                               is this on Keith's paper? Add a citation doesn't
>                               hurt then.

All words are there, except for "Policy", although it clearly represents one.

Either way, I'll rephrase and cite the paper.

> >                 FlushIfCriticalOutputPending();
> > +
> > +               /* If the client is still running at the end of the interval,
> > +                * the client's priority is reduced by one (but no less than a
> > +                * minimum value). If the client hasn't been ready to run for
> > +                * some time, priority is increased by one (but no more than the
> > +                * initial base value).
> > +                *
> > +                * These priority adjustments penalize busy applications and
> > +                * praise idle ones. This is a simplification of discovering
> > +                * precisely how much time a client has used; that would require
> > +                * a system call.
> 
> I understood everything until "required a system call". Can you be more
> verbose here?

Sure.

> > --- a/os/WaitFor.c
> > +++ b/os/WaitFor.c
> > @@ -130,15 +130,15 @@ static OsTimerPtr timers = NULL;
> >   * WaitForSomething:
> >   *     Make the server suspend until there is
> >   *     1. data from clients or
> > - *     2. input events available or
> > - *     3. ddx notices something of interest (graphics
> > - *        queue ready, etc.) or
> > + *     2. input events available
> 
> you could leave the "or"...

Yes.

> Thanks for doing this, Fernando!

Thank you for reviewing.