Re: [PATCH] libpager/data-request.c: missing call to _pager_allow_termination

[Neal H. Walfield]

At Tue, 28 Sep 2004 18:33:57 +0300,
Ognyan Kulev wrote:
> 
> Neal H. Walfield wrote:
> > This looks correct to me.  Normally, I would say that this sort of
> > micro-optimization does not matter as we don't care how slow the error
> > path is, however, the other jump to allow_term_out (on the fast path)
> > also has a gratuitous unlock relock sequence:
> > 
> >   /* Let someone else in.  */
> >   _pager_release_seqno (p, seqno);
> >   mutex_unlock (&p->interlock);
> > 
> >   if (!doread)
> >     goto allow_term_out;
> > 
> > So, if wold be useful to also move the if above the unlock and moved
> > the mutex_lock in allow_term_out to error_read and adjusted callers.
> 
> Does this apply to goto error_read too?

No.  The error_read assumes that the interlock is held and that is the
correct assumption for all of the callers (and therefore no fix up is
required).

>   Actually, I haven't though that 
> unlock-relock is a problem here.

An unlock followed by a relock is just gratuitous.  In the fast path
we want to avoid them.

> >>    err = _pager_pagemap_resize (p, offset + length);
> >>    if (err)
> >>-    goto release_out;              /* Can't do much about the actual 
> >>error.  */
> >>+    {
> >>+      _pager_allow_termination (p);
> >>+      goto release_out;      /* Can't do much about the actual error.  */
> >>+    }
> > 
> > This only solves half the problem.  You also have to call
> > _pager_release_seqno as well.
> 
> It's called in release_out.

You're right, sorry.  (It is not called in allow_term_out which is
what I now remembering looking at.)

> > 2004-09-28  Neal H. Walfield  <neal@cs.uml.edu>
> >     Ognyan Kulev  <ogi@fmi.uni-sofia.bg>
> 
> I'm not sure this style is correct.

Something similar to this has been used in glibc for collaboration:

    2004-01-02  Jakub Jelinek  <jakub@redhat.com>
                Paolo Bonzini  <bonzini@gnu.org>
    
            * posix/regex_internal.h (re_const_bitset_ptr_t): New type.
            ...

Thanks,
Neal