You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Bojan Smojver <bo...@rexursive.com> on 2007/05/11 05:39:15 UTC
Regarding apr_file_writev() and locking...
At present, we have in this function:
if (thefile->buffered) {
flush
adjust various buffer offsets
}
If two threads do this for the same file (i.e. apr_file_t*), offsets may
end up screwed. Shouldn't we surround this with lock/unlock, like we do
in apr_file_write()?
PS. Also, I think we can do this only for HAVE_WRITEV as in the other
case apr_file_write() will take care of the whole thing (i.e. that
function already has a buffered path).
--
Bojan
Re: Regarding apr_file_writev() and locking...
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2007-05-12 at 09:32 -0300, Davi Arnaut wrote:
> No, but this:
>
> +#define file_lock(f) { .. }
> +#define file_unlock(f) { .. }
>
> should be:
>
> #define file_lock(f) do { .. } while (0)
> #define file_unlock(f) do { .. } while (0)
>
> or else it will break for expressions like:
>
> if (file->buffered || whatever)
> file_lock(f); /* semi-colon after the block */
> else
> whatever++;
OOPS! Sorry, will fix now.
--
Bojan
Re: Regarding apr_file_writev() and locking...
Posted by Davi Arnaut <da...@haxent.com.br>.
Bojan Smojver wrote:
> On Fri, 2007-05-11 at 22:38 -0300, Davi Arnaut wrote:
>
>> The patches improves the locking interaction with buffering.
>
> Looking at the inline functions file_lock/unlock, wouldn't it be a bit
> more appropriate to do them as macros? I'm thinking on platforms that
> have a C compiler that doesn't understand inlining, we may be a bit
> better off.
>
> Do you mind if I do macros instead?
>
No, but this:
+#define file_lock(f) { .. }
+#define file_unlock(f) { .. }
should be:
#define file_lock(f) do { .. } while (0)
#define file_unlock(f) do { .. } while (0)
or else it will break for expressions like:
if (file->buffered || whatever)
file_lock(f); /* semi-colon after the block */
else
whatever++;
--
Davi Arnaut
Re: Regarding apr_file_writev() and locking...
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2007-05-11 at 22:38 -0300, Davi Arnaut wrote:
> The patches improves the locking interaction with buffering.
Looking at the inline functions file_lock/unlock, wouldn't it be a bit
more appropriate to do them as macros? I'm thinking on platforms that
have a C compiler that doesn't understand inlining, we may be a bit
better off.
Do you mind if I do macros instead?
--
Bojan
Re: Regarding apr_file_writev() and locking...
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2007-05-12 at 02:22 -0300, Davi Arnaut wrote:
> On my machine (Pentium D CPU 2.66GHz, dual core) testlockperf shows (in
> some runs) that the NESTED mutex is actually faster then the UNNESTED.
Cool. I can revert that function then.
--
Bojan
Re: Regarding apr_file_writev() and locking...
Posted by Davi Arnaut <da...@haxent.com.br>.
Bojan Smojver wrote:
> On Fri, 2007-05-11 at 22:38 -0300, Davi Arnaut wrote:
>
>> I've just posted a few initial patches to bugzilla, PR 42400. (passes
>> testfile)
>
> Thanks. I had a brief look through and they seem nice. I will test to
> confirm.
>
> I see you switched the mutex to nested. I think I can then revert the
> code for file_read_buffered() - the nested mutex would take care of that
> now.
>
> The alternative approach would be to do apr_file_flush() in two parts -
> public (with locking) and private (no locking). However, I have no idea
> if nested mutex is greater overhead than another function call.
On my machine (Pentium D CPU 2.66GHz, dual core) testlockperf shows (in
some runs) that the NESTED mutex is actually faster then the UNNESTED.
apr_thread_mutex_t Tests
Initializing the apr_thread_mutex_t (UNNESTED) OK
Starting 1 threads OK
microseconds: 91388 usec
apr_thread_mutex_t Tests
Initializing the apr_thread_mutex_t (NESTED) OK
Starting 1 threads OK
microseconds: 87852 usec
apr_thread_mutex_t Tests
Initializing the apr_thread_mutex_t (UNNESTED) OK
Starting 2 threads OK
microseconds: 786738 usec
apr_thread_mutex_t Tests
Initializing the apr_thread_mutex_t (NESTED) OK
Starting 2 threads OK
microseconds: 784970 usec
apr_thread_mutex_t Tests
Initializing the apr_thread_mutex_t (UNNESTED) OK
Starting 3 threads OK
microseconds: 1186499 usec
apr_thread_mutex_t Tests
Initializing the apr_thread_mutex_t (NESTED) OK
Starting 3 threads OK
microseconds: 884517 usec
>> I am in the middle of writing a patch that converts all file_io to
>> standard i/o functions (stdio/streams) so we can remove all code
>> involving buffering, file positioning and even locking at some cases. It
>> saves a huge amount of code lines, and I hope, gives fewer bugs.
>>
>> Comments?
>
> I would venture I guess that this would be trunk stuff only. But maybe
> others feel something like that can go into 1.2.x/0.9.x.
trunk stuff.
> Anyhow, I'm sure someone will point out the history of buffered code and
> why a rewrite of stdio stuff happened in the fist place.
>
Yes, please. I saw in the svn log history, but I couldn't find the exact
commit.
--
Davi Arnaut
Re: Regarding apr_file_writev() and locking...
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2007-05-11 at 22:38 -0300, Davi Arnaut wrote:
> I've just posted a few initial patches to bugzilla, PR 42400. (passes
> testfile)
Thanks. I had a brief look through and they seem nice. I will test to
confirm.
I see you switched the mutex to nested. I think I can then revert the
code for file_read_buffered() - the nested mutex would take care of that
now.
The alternative approach would be to do apr_file_flush() in two parts -
public (with locking) and private (no locking). However, I have no idea
if nested mutex is greater overhead than another function call.
> I am in the middle of writing a patch that converts all file_io to
> standard i/o functions (stdio/streams) so we can remove all code
> involving buffering, file positioning and even locking at some cases. It
> saves a huge amount of code lines, and I hope, gives fewer bugs.
>
> Comments?
I would venture I guess that this would be trunk stuff only. But maybe
others feel something like that can go into 1.2.x/0.9.x.
Anyhow, I'm sure someone will point out the history of buffered code and
why a rewrite of stdio stuff happened in the fist place.
--
Bojan
Re: Regarding apr_file_writev() and locking...
Posted by Davi Arnaut <da...@haxent.com.br>.
Joe Orton wrote:
> On Mon, May 14, 2007 at 09:54:10AM -0300, Davi Arnaut wrote:
>> Joe Orton wrote:
>>> On Fri, May 11, 2007 at 10:38:01PM -0300, Davi Arnaut wrote:
>>>> The patches improves the locking interaction with buffering. But, quite
>>>> frankly, the unix file_io locking and buffering is a bit messed. There
>>>> is no clear indication which functions are protected by APR_XTHREAD,
>>>> checks for partial writes on flushing are missing, buffering and file
>>>> positioning don't play well together, et cetera.
>>> I don't disagree that the existing buffering/mutex stuff is a mess, but
>>> nested mutexes are not available on all platforms, so they can't be
>>> relied upon.
>> Pools also use nested mutexes.
>
> Only in the APR_POOL_DEBUG code.
I missed that, sorry.
>
>> So, I guess those platforms need to be fixed. BTW, which platforms
>> that use unix/file_io don't support nested mutexes?
>
> Anything which doesn't support PTHREAD_MUTEX_RECURSIVE: older Linuxes,
> probably some BSDs; not sure about any of the more exotic Unixes.
OK.
--
Davi Arnaut
Re: Regarding apr_file_writev() and locking...
Posted by Joe Orton <jo...@redhat.com>.
On Mon, May 14, 2007 at 09:54:10AM -0300, Davi Arnaut wrote:
> Joe Orton wrote:
> > On Fri, May 11, 2007 at 10:38:01PM -0300, Davi Arnaut wrote:
> >> The patches improves the locking interaction with buffering. But, quite
> >> frankly, the unix file_io locking and buffering is a bit messed. There
> >> is no clear indication which functions are protected by APR_XTHREAD,
> >> checks for partial writes on flushing are missing, buffering and file
> >> positioning don't play well together, et cetera.
> >
> > I don't disagree that the existing buffering/mutex stuff is a mess, but
> > nested mutexes are not available on all platforms, so they can't be
> > relied upon.
>
> Pools also use nested mutexes.
Only in the APR_POOL_DEBUG code.
> So, I guess those platforms need to be fixed. BTW, which platforms
> that use unix/file_io don't support nested mutexes?
Anything which doesn't support PTHREAD_MUTEX_RECURSIVE: older Linuxes,
probably some BSDs; not sure about any of the more exotic Unixes.
joe
Re: Regarding apr_file_writev() and locking...
Posted by Davi Arnaut <da...@haxent.com.br>.
Joe Orton wrote:
> On Fri, May 11, 2007 at 10:38:01PM -0300, Davi Arnaut wrote:
>> The patches improves the locking interaction with buffering. But, quite
>> frankly, the unix file_io locking and buffering is a bit messed. There
>> is no clear indication which functions are protected by APR_XTHREAD,
>> checks for partial writes on flushing are missing, buffering and file
>> positioning don't play well together, et cetera.
>
> I don't disagree that the existing buffering/mutex stuff is a mess, but
> nested mutexes are not available on all platforms, so they can't be
> relied upon.
Pools also use nested mutexes. So, I guess those platforms need to be
fixed. BTW, which platforms that use unix/file_io don't support nested
mutexes?
For now I think we can create a file_flush_locked function.
>> I am in the middle of writing a patch that converts all file_io to
>> standard i/o functions (stdio/streams) so we can remove all code
>> involving buffering, file positioning and even locking at some cases. It
>> saves a huge amount of code lines, and I hope, gives fewer bugs.
>
> A notable Unix platform has a crippled stdio implementation (32-bit
> Solaris), so this probably doesn't really fly either.
Another option is to split locking, buffering and i/o into separate
layers. Locking and buffering would be platform-agnostic.
A similar (yet much simpler) mechanism like the apache filters. This
also can save us a lot of trouble, since each port (unix/win32/os2) of
file_io has it's own implementation of buffering and locking, but yet
they are the same.
--
Davi Arnaut
Re: Regarding apr_file_writev() and locking...
Posted by Joe Orton <jo...@redhat.com>.
On Fri, May 11, 2007 at 10:38:01PM -0300, Davi Arnaut wrote:
> The patches improves the locking interaction with buffering. But, quite
> frankly, the unix file_io locking and buffering is a bit messed. There
> is no clear indication which functions are protected by APR_XTHREAD,
> checks for partial writes on flushing are missing, buffering and file
> positioning don't play well together, et cetera.
I don't disagree that the existing buffering/mutex stuff is a mess, but
nested mutexes are not available on all platforms, so they can't be
relied upon.
> I am in the middle of writing a patch that converts all file_io to
> standard i/o functions (stdio/streams) so we can remove all code
> involving buffering, file positioning and even locking at some cases. It
> saves a huge amount of code lines, and I hope, gives fewer bugs.
A notable Unix platform has a crippled stdio implementation (32-bit
Solaris), so this probably doesn't really fly either.
joe
Re: Regarding apr_file_writev() and locking...
Posted by Davi Arnaut <da...@haxent.com.br>.
Bojan Smojver wrote:
> On Fri, 2007-05-11 at 15:58 +0100, Joe Orton wrote:
>
>> Yes, that looks necessary.
>
> Thanks, cool. Will implement. Honestly, even apr_file_flush() looks a
> bit suspect to me.
>
I've just posted a few initial patches to bugzilla, PR 42400. (passes
testfile)
The patches improves the locking interaction with buffering. But, quite
frankly, the unix file_io locking and buffering is a bit messed. There
is no clear indication which functions are protected by APR_XTHREAD,
checks for partial writes on flushing are missing, buffering and file
positioning don't play well together, et cetera.
I am in the middle of writing a patch that converts all file_io to
standard i/o functions (stdio/streams) so we can remove all code
involving buffering, file positioning and even locking at some cases. It
saves a huge amount of code lines, and I hope, gives fewer bugs.
Comments?
--
Davi Arnaut
Re: Regarding apr_file_writev() and locking...
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2007-05-11 at 15:58 +0100, Joe Orton wrote:
> Yes, that looks necessary.
Thanks, cool. Will implement. Honestly, even apr_file_flush() looks a
bit suspect to me.
> Yup. BTW can you add CHANGES entries (on both 1.2.x and 0.9.x branches)
> listing the PR 40963 fix?
Will do, together with the above. Thanks for pointing it out.
--
Bojan
Re: Regarding apr_file_writev() and locking...
Posted by Joe Orton <jo...@redhat.com>.
On Fri, May 11, 2007 at 01:39:15PM +1000, Bojan Smojver wrote:
> At present, we have in this function:
>
> if (thefile->buffered) {
> flush
> adjust various buffer offsets
> }
>
> If two threads do this for the same file (i.e. apr_file_t*), offsets may
> end up screwed. Shouldn't we surround this with lock/unlock, like we do
> in apr_file_write()?
Yes, that looks necessary.
> PS. Also, I think we can do this only for HAVE_WRITEV as in the other
> case apr_file_write() will take care of the whole thing (i.e. that
> function already has a buffered path).
Yup. BTW can you add CHANGES entries (on both 1.2.x and 0.9.x branches)
listing the PR 40963 fix?
joe