You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Doug MacEachern <do...@covalent.net> on 2001/11/28 01:45:11 UTC
apr_buckets_file.c:file_read + XTHREAD
if file_read() cannot mmap the the file, it does this:
#if APR_HAS_THREADS && !APR_HAS_XTHREAD_FILES
if ((flags = apr_file_flags_get(f)) & APR_XTHREAD) {
/* this file descriptor is shared across multiple threads and
* this OS doesn't support that natively, so as a workaround
* we must reopen the file into a->readpool */
const char *fname;
apr_file_name_get(&fname, f);
rv = apr_file_open(&f, fname, (flags & ~APR_XTHREAD), 0, a->readpool);
if (rv != APR_SUCCESS)
return rv;
a->fd = f;
}
#endif
in the case above does the file get re-opened everytime file_read() is
called or just once per-request? if not the latter, any reason why it
can't be that way? looks like the XTHREAD thinger is only used by
mod_file_cache.c, shouldn't its handler take care of re-opening the file
instead of file_read()?
re-open and apr_file_seek() for every file_read() doesn't seem right.
and i don't see why apr_file_seek() should happen if the file isn't being
re-opened, but it currently happens regardless of the XTHREAD flag.
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Ryan Bloom" <rb...@covalent.net>
Sent: Tuesday, November 27, 2001 11:26 PM
> > What I'm suggesting is that as each bucket is consumed, it is un-memaped.
> > The buckets are only mmaped as they are read. This way, we get the
> > benefits of mmap, without the pain of page consumption.
> >
> > Idea?
>
> Not good. The problem is that you will take the real chance that multiple
> filters will be converting to mmap, and then unmapping large files without
> making any changes. Think of a large SSI file that does have any tags.
> You'll MMAP each piece, unmapping as you go, and then you'll do the
> same thing again in the next filter.
That would hurt, yes. But we need two things to make this happen without such
a nasty side effect. One, refcounting. Two, the simple fact that once consumed,
the other filters have no business touching any old bucket or data.
The only reasonable scenario, short of a really, really messed up filter, is
that something like a big SSI tag would span _two_ mmapped buckets. That's fine,
so we actually _drop_ the threshold for a single mmap (say 256kb or something, that
should actually become tunable.) In any case, any filter hanging on to several
meg at once is just asking for problems, if it might be expected to be invoked by
many requests concurrently.
Cliff has his hands around this concept better than I... I'll wait to grok his
patch :) Time to get back to work by this weekend, and actually start authoring
the three filters I had in mind ... so I can start stressing such things.
Bill
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Ryan Bloom" <rb...@covalent.net>
Sent: Tuesday, November 27, 2001 11:35 PM
> Bill's idea was to map and then unmap the individual sections of the file, so
> that no more than one section was mapped at a time. If you do that, and you
> really unmap each section as you map the next, you will be thrashing your
> MMAPs. If you are really going to leave each section as an MMAP, then I
> would agree completely, and say let's do it, but that wasn't what Bill suggested.
MMAPs consume memory. Yes, they may not start in-memory but await their first
pagefault. Yes, they may go away when they fall off the mru and weren't modified.
But why force the kernel to make these elections when we can help? If you set some
arbitrary mmap limit based on your available memory and anticipated consumption,
then cycle out those pages, you avoid forcing unneeded pagefaults for lack of memory
and the associated discarding of the mmaps themselves.
Now I'm speaking to Win32, I don't know the Unix mmap architecture, but I don't
imagine it's that different. The bottom line is that the read-from-file is as
optimized as you can _ever_ get with an mmaped section. It is far more efficient
than a read call which is soaking the kernel buffer, then copying to the user's
buffer space.
I imagine the cost of mru elections will outweigh any imagined benefit from keeping
a much-too-large file mapped in memory.
Bill
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Ryan Bloom <rb...@covalent.net>.
On Tuesday 27 November 2001 09:39 pm, Cliff Woolley wrote:
> On Tue, 27 Nov 2001, Ryan Bloom wrote:
> > Bill's idea was to map and then unmap the individual sections of the
> > file, so that no more than one section was mapped at a time. If you
> > do that, and you really unmap each section as you map the next, you
> > will be thrashing your MMAPs. If you are really going to leave each
> > section as an MMAP, then I would agree completely, and say let's do
> > it, but that wasn't what Bill suggested.
>
> It might not be exactly what he said, but I thought it's what he _meant_.
> Ahh, don't you love the expressiveness of email? :-) One at a time is
> typically what you get with the code I posted anyhow since most filters
> limit how much they'll buffer, but the one-at-a-time rule is just not
> enforced. If it were enforced, then oh yeah, that would be bad. :)
The problem is the pathological case for the content-length filter, where it
absolutely REQUIRES a content-length. If you put that filter after the
SSI filter, and we map and unmap, you will have problems. While I agree
that in the general case mapping and unmapping is fine, I am most concerned
about the pathological case that we only hit once every few years, because
that is the case that will bite us in the ass. :-)
I remember one lesson REALLY well from my time at IBM. I worked next
to one of the guys working on the IBM proxy server, and he told me a story
about one of the web server developers who had moved on years before.
The guy wrote a block of threaded code that had a tiny race condition. He
did all of the math, and he found out that in the pathological case, the race
condition was thousandths of a second. He theorized that they would never
actually see the race condition, so it was safe to leave it in. Wouldn't you
know it, the code never got out of QA. When you put any kind of real stress
on the server, the race condition was hit.
Always worry about the pathological case when you write the code, it will
happen at least once. :-)
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, William A. Rowe, Jr. wrote:
Guys, we're not communicating worth a damn. :) I think we're proabably
all saying the same things without realizing it.
> > > Bill's idea was to map and then unmap the individual sections of the
> > > file, so that no more than one section was mapped at a time.
>
> No, properly I implied that we have no more sections mapped than we are
> actually interested in, at any given time.
Right. And you can be interested in more than one at a time. As soon as
one filter becomes interested in a section of the file, the whole server
is interested until that section goes out onto the network or its bucket
gets deleted, in which case we don't care about it anymore by definition.
> > It might not be exactly what he said, but I thought it's what he _meant_.
> > Ahh, don't you love the expressiveness of email? :-) One at a time is
> > typically what you get with the code I posted anyhow since most filters
> > limit how much they'll buffer, but the one-at-a-time rule is just not
> > enforced. If it were enforced, then oh yeah, that would be bad. :)
>
> No, I really implied that we do unmap those that are consumed. If a really
> bad filter reads in all 200 buckets of 4MB each, then the system will start
> flailing. But no filter author would create such a design, no :-?
> If you read from the bucket (mmap) and pass it on, the memory footprint
> won't get out of hand.
That's what I meant! :) Yes there is a pathological case where you'll
try to mmap a whole huge 200MB file at once because some jackass (eg the
content-length filter) buffered the whole damned thing, but at some point,
the OS will say "you can't mmap that much stuff all at the same time", and
we'll gracefully fall back on the read-8KB-at-a-time method. (If we're
saying that we want to be more restrictive about how much crap we can mmap
at a time system-wide, I say that's up to APR to decide for us if it
doesn't already, in which case this is still the right approach from
apr-util's perspective.)
Are we on the same page yet? I hope? If so, does the patch I sent in
earlier implement that page? I think it does.
All it says is that if the file is bigger than APR_MMAP_LIMIT, then we try
to mmap a limit's worth of the file. If that succeeds, we split the file
bucket into two parts, the first part is APR_MMAP_LIMIT in size, and the
other part is the rest of the file. We turn the first of those two into
an mmap bucket, shoving in the mmap we just created. If the mmap fails,
we abort and fall back on the read-8KB method. If the file is less than
APR_MMAP_LIMIT in size, we do the same thing we did before, ie, MMAP the
whole file. When all references to a given mmap go away, we delete the
mmap. If we're the owners of the MMAP (which we will be in this case),
that means we munmap the region.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Cliff Woolley" <cl...@yahoo.com>
Sent: Tuesday, November 27, 2001 11:39 PM
> On Tue, 27 Nov 2001, Ryan Bloom wrote:
>
> > Bill's idea was to map and then unmap the individual sections of the
> > file, so that no more than one section was mapped at a time.
No, properly I implied that we have no more sections mapped than we are
actually interested in, at any given time.
> > If you
> > do that, and you really unmap each section as you map the next, you
> > will be thrashing your MMAPs.
How? See my other post on why unmapping/remapping is really painless
when weighed against reading, and pagefaulting.
> > If you are really going to leave each
> > section as an MMAP, then I would agree completely, and say let's do
> > it, but that wasn't what Bill suggested.
>
> It might not be exactly what he said, but I thought it's what he _meant_.
> Ahh, don't you love the expressiveness of email? :-) One at a time is
> typically what you get with the code I posted anyhow since most filters
> limit how much they'll buffer, but the one-at-a-time rule is just not
> enforced. If it were enforced, then oh yeah, that would be bad. :)
No, I really implied that we do unmap those that are consumed. If a really
bad filter reads in all 200 buckets of 4MB each, then the system will start
flailing. But no filter author would create such a design, no :-?
If you read from the bucket (mmap) and pass it on, the memory footprint
won't get out of hand.
Bill
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, Ryan Bloom wrote:
> Bill's idea was to map and then unmap the individual sections of the
> file, so that no more than one section was mapped at a time. If you
> do that, and you really unmap each section as you map the next, you
> will be thrashing your MMAPs. If you are really going to leave each
> section as an MMAP, then I would agree completely, and say let's do
> it, but that wasn't what Bill suggested.
It might not be exactly what he said, but I thought it's what he _meant_.
Ahh, don't you love the expressiveness of email? :-) One at a time is
typically what you get with the code I posted anyhow since most filters
limit how much they'll buffer, but the one-at-a-time rule is just not
enforced. If it were enforced, then oh yeah, that would be bad. :)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Ryan Bloom <rb...@covalent.net>.
On Tuesday 27 November 2001 09:26 pm, Cliff Woolley wrote:
> On Tue, 27 Nov 2001, Ryan Bloom wrote:
> > Not good. The problem is that you will take the real chance that
> > multiple filters will be converting to mmap, and then unmapping large
> > files without making any changes. Think of a large SSI file that does
> > have any tags. You'll MMAP each piece, unmapping as you go, and then
> > you'll do the same thing again in the next filter.
>
> Um, why? Once one filter mmap's the thing, it stays that way until the
> bucket is deleted or the bucket gets sent out onto the network. It's not
> like mod_include says oh, I ended up not doing anything to that bucket, so
> I can map it back down to a file, right? So mod_include leaves it as an
> mmap which it passes down the filter chain right away. Only once it goes
> out on the network do we munmap it. Granted, we might then have to mmap
> the next 4MB of the file when we return up the stack, but I don't see that
> as a problem.
>
> What am I missing?
Bill's idea was to map and then unmap the individual sections of the file, so
that no more than one section was mapped at a time. If you do that, and you
really unmap each section as you map the next, you will be thrashing your
MMAPs. If you are really going to leave each section as an MMAP, then I
would agree completely, and say let's do it, but that wasn't what Bill suggested.
:-)
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, Ryan Bloom wrote:
> Not good. The problem is that you will take the real chance that multiple
> filters will be converting to mmap, and then unmapping large files without
> making any changes. Think of a large SSI file that does have any tags.
> You'll MMAP each piece, unmapping as you go, and then you'll do the
> same thing again in the next filter.
Um, why? Once one filter mmap's the thing, it stays that way until the
bucket is deleted or the bucket gets sent out onto the network. It's not
like mod_include says oh, I ended up not doing anything to that bucket, so
I can map it back down to a file, right? So mod_include leaves it as an
mmap which it passes down the filter chain right away. Only once it goes
out on the network do we munmap it. Granted, we might then have to mmap
the next 4MB of the file when we return up the stack, but I don't see that
as a problem.
What am I missing?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Ryan Bloom <rb...@covalent.net>.
On Tuesday 27 November 2001 08:05 pm, William A. Rowe, Jr. wrote:
> From: "Cliff Woolley" <cl...@yahoo.com>
> Sent: Tuesday, November 27, 2001 9:35 PM
>
> > Well, here's another kludge for you: the problem is that
> > apr_bucket_read() for file buckets falls into an abyss of performance as
> > soon as the magic APR_MMAP_LIMIT is exceeded. That's because we give up
> > on mmap entirely and read 8kb at a time frome the whole bigass file, even
> > if all we wanted in the first place was the first 32 bytes or so (say for
> > something like mod_mime_magic) and we're never going to touch the other
> > 12MB. We still have to pay the penalty anyway. What if
> > apr_bucket_read() on a file bucket bigger than APR_MMAP_LIMIT would turn
> > the file bucket into an mmap bucket+file bucket pair, with the mmap
> > bucket APR_MMAP_LIMIT in size, and the file bucket gets the rest? I
> > think that would tackle this problem nicely. In a case like
> > mod_mime_magic where we're only reading the beginning of the file, we'd
> > actually still be able to use sendfile() with the first APR_MMAP_LIMIT's
> > worth as a header. :) Of course, you might run into a case where you're
> > os-limited as to how much you can have mmaped at one time per process if
> > we buffer a large number of buckets for some reason, but then the mmap
> > will fail and we'll fall back on read() [the code already does that].
>
> Better yet.
>
> If APR_MMAP_LIMIT is exceeded, break it into n APR_MMAP_LIMIT sized
> buckets, wherein only one bucket may be mapped at a time.
>
> What I'm suggesting is that as each bucket is consumed, it is un-memaped.
> The buckets are only mmaped as they are read. This way, we get the
> benefits of mmap, without the pain of page consumption.
>
> Idea?
Not good. The problem is that you will take the real chance that multiple
filters will be converting to mmap, and then unmapping large files without
making any changes. Think of a large SSI file that does have any tags.
You'll MMAP each piece, unmapping as you go, and then you'll do the
same thing again in the next filter.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, Justin Erenkrantz wrote:
> However, I wonder if the cost of repetitive mmap() and munmap()
> might be too prohibitive (how expensive is mmap and munmap?).
Hopefully less expensive than sending 4MB of data out over the wire, but
hey, you never know. :)
> But, we'll do this in APR_MMAP_LIMIT chunks (which I think is
> 4MB). So, it might balance out. -- justin
Hopefully.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Tue, Nov 27, 2001 at 10:05:27PM -0600, William A. Rowe, Jr. wrote:
> If APR_MMAP_LIMIT is exceeded, break it into n APR_MMAP_LIMIT sized buckets,
> wherein only one bucket may be mapped at a time.
>
> What I'm suggesting is that as each bucket is consumed, it is un-memaped.
> The buckets are only mmaped as they are read. This way, we get the benefits
> of mmap, without the pain of page consumption.
+1. I'm kind of shocked we aren't doing that now.
However, I wonder if the cost of repetitive mmap() and munmap()
might be too prohibitive (how expensive is mmap and munmap?).
But, we'll do this in APR_MMAP_LIMIT chunks (which I think is
4MB). So, it might balance out. -- justin
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 27 Nov 2001, William A. Rowe, Jr. wrote:
> Better yet.
>
> If APR_MMAP_LIMIT is exceeded, break it into n APR_MMAP_LIMIT sized buckets,
> wherein only one bucket may be mapped at a time.
> What I'm suggesting is that as each bucket is consumed, it is un-memaped.
> The buckets are only mmaped as they are read. This way, we get the benefits
> of mmap, without the pain of page consumption.
Absolutely.
That's actually what I had in mind. I forgot about the friggin pool
cleanup doing the apr_mmap_delete() for us rather than mmap_destroy()
doing it. Feh.
Maybe the newly-introduced mmap "ownership" concept can help us here. If
the mmap bucket holds the apr_mmap_t that "owns" the OS mmap, then the
buckets code's mmap_destroy() can safely munmap the thing. We should now
actually always be able to call apr_mmap_delete() from mmap_destroy()
safely, because apr_mmap_delete() knows about the owner concept and just
returns success if it's not the owner. That would be nice, actually,
because we wouldn't have to worry about cases like mmap_destroy()
munmaping an mmap owned by mod_file_cache, for example.
So I guess it would look something like this:
Index: apr_buckets_file.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_buckets_file.c,v
retrieving revision 1.61
diff -u -d -r1.61 apr_buckets_file.c
--- apr_buckets_file.c 2001/11/28 00:38:00 1.61
+++ apr_buckets_file.c 2001/11/28 04:26:13
@@ -84,15 +84,25 @@
apr_bucket_file *a = e->data;
apr_mmap_t *mm;
- if (APR_MMAP_CANDIDATE(filelength) &&
- (apr_mmap_create(&mm, a->fd, fileoffset, filelength,
- APR_MMAP_READ, p) == APR_SUCCESS))
+ if (filelength > APR_MMAP_LIMIT) {
+ if (apr_mmap_create(&mm, a->fd, fileoffset, APR_MMAP_LIMIT,
+ APR_MMAP_READ, p) != APR_SUCCESS) {
+ return 0;
+ }
+ else {
+ apr_bucket_split(e, APR_MMAP_LIMIT);
+ filelength = APR_MMAP_LIMIT;
+ }
+ }
+ else if ((filelength >= APR_MMAP_THRESHOLD) &&
+ (apr_mmap_create(&mm, a->fd, fileoffset, filelength,
+ APR_MMAP_READ, p) != APR_SUCCESS))
{
- apr_bucket_mmap_make(e, mm, 0, filelength);
- file_destroy(a);
- return 1;
+ return 0;
}
- return 0;
+ apr_bucket_mmap_make(e, mm, 0, filelength);
+ file_destroy(a);
+ return 1;
}
#endif
Index: apr_buckets_mmap.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_buckets_mmap.c,v
retrieving revision 1.44
diff -u -d -r1.44 apr_buckets_mmap.c
--- apr_buckets_mmap.c 2001/11/21 17:00:51 1.44
+++ apr_buckets_mmap.c 2001/11/28 04:26:13
@@ -80,8 +80,7 @@
apr_bucket_mmap *m = data;
if (apr_bucket_shared_destroy(m)) {
- /* no need to apr_mmap_delete(m->mmap) here... it will
- * get done automatically when the pool gets cleaned up. */
+ apr_mmap_delete(m->mmap);
free(m);
}
}
[Tested to compile but not to work ;]
Thoughts?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Cliff Woolley" <cl...@yahoo.com>
Sent: Tuesday, November 27, 2001 9:35 PM
> Well, here's another kludge for you: the problem is that apr_bucket_read()
> for file buckets falls into an abyss of performance as soon as the magic
> APR_MMAP_LIMIT is exceeded. That's because we give up on mmap entirely
> and read 8kb at a time frome the whole bigass file, even if all we wanted
> in the first place was the first 32 bytes or so (say for something like
> mod_mime_magic) and we're never going to touch the other 12MB. We still
> have to pay the penalty anyway. What if apr_bucket_read() on a file
> bucket bigger than APR_MMAP_LIMIT would turn the file bucket into an mmap
> bucket+file bucket pair, with the mmap bucket APR_MMAP_LIMIT in size, and
> the file bucket gets the rest? I think that would tackle this problem
> nicely. In a case like mod_mime_magic where we're only reading the
> beginning of the file, we'd actually still be able to use sendfile()
> with the first APR_MMAP_LIMIT's worth as a header. :) Of course, you
> might run into a case where you're os-limited as to how much you can have
> mmaped at one time per process if we buffer a large number of buckets for
> some reason, but then the mmap will fail and we'll fall back on read()
> [the code already does that].
Better yet.
If APR_MMAP_LIMIT is exceeded, break it into n APR_MMAP_LIMIT sized buckets,
wherein only one bucket may be mapped at a time.
What I'm suggesting is that as each bucket is consumed, it is un-memaped.
The buckets are only mmaped as they are read. This way, we get the benefits
of mmap, without the pain of page consumption.
Idea?
Bill
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, Cliff Woolley wrote:
> APR_MMAP_LIMIT is exceeded. That's because we give up on mmap entirely
> and read 8kb at a time frome the whole bigass file, even if all we wanted
> in the first place was the first 32 bytes or so (say for something like
> mod_mime_magic) and we're never going to touch the other 12MB.
Oops. That's totally wrong. That's what I get for typing faster than
I can think. ;-)
Each time we read 8 more KB, we have another heap+file combination. We
just keep pulling 8K off the front of the file bucket. If we only ever
actually wanted to look at the first 32 bytes, when we call
apr_bucket_read() we'll get an 8KB heap bucket followed by a file bucket
containing the rest of the file, and we can still use sendfile() on that.
So ignore this part of the argument, it was crap. :)
But the rest of the bogosity still stands, and I think OtherBill's and my
proposed change is still very worthwhile.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, William A. Rowe, Jr. wrote:
> Bzzt... wrong. There are plenty of non-zero copy operations in the
> world... this isn't the 'unique exception' ... it will be frequent
> enough to warrent redesign.
>
> > Perhaps a better solution would be to just create a read function
> > that accepts a pre-allocated buffer that we must copy into. Then,
> > filters that know they are going to throw the data away would incur
> > the penalty of the copy in lieu of the penalty of the malloc().
> > With the file bucket, this has the advantage that the read from the
> > disk can be put directly into the pre-allocated buffer, and the copy
> > doesn't actually exist.
>
> Bingo. That way you haven't tied it up with specific bucket types -
> but extended the api for all buckets.
Works for me.
> This is where I keep saying that the consumer often knows more about
> the mechanics of transfering or processing the data than the
> producer... until we add some graceful methods for handling those
> cases, kludges will abound.
Well, here's another kludge for you: the problem is that apr_bucket_read()
for file buckets falls into an abyss of performance as soon as the magic
APR_MMAP_LIMIT is exceeded. That's because we give up on mmap entirely
and read 8kb at a time frome the whole bigass file, even if all we wanted
in the first place was the first 32 bytes or so (say for something like
mod_mime_magic) and we're never going to touch the other 12MB. We still
have to pay the penalty anyway. What if apr_bucket_read() on a file
bucket bigger than APR_MMAP_LIMIT would turn the file bucket into an mmap
bucket+file bucket pair, with the mmap bucket APR_MMAP_LIMIT in size, and
the file bucket gets the rest? I think that would tackle this problem
nicely. In a case like mod_mime_magic where we're only reading the
beginning of the file, we'd actually still be able to use sendfile()
with the first APR_MMAP_LIMIT's worth as a header. :) Of course, you
might run into a case where you're os-limited as to how much you can have
mmaped at one time per process if we buffer a large number of buckets for
some reason, but then the mmap will fail and we'll fall back on read()
[the code already does that].
It actually never occurred to me before that what happens with the code
now on a file bigger than APR_MMAP_LIMIT is that we get a bunch of 8KB
heap buckets until there is <= APR_MMAP_LIMIT _left_ in the file, and that
last bit of the file is mmaped. So we have a bunch of 8KB heap buckets
followed by a 4MB-ish mmap. Man, that's weird. :)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Ryan Bloom" <rb...@covalent.net>
Sent: Tuesday, November 27, 2001 9:12 PM
> On Tuesday 27 November 2001 07:02 pm, Doug MacEachern wrote:
>
> This seems straightforward to me. The bucket read function doesn't
> make sense in this case, because we know that we are going to read
> the entire file, and throw all of it away without ever actually using the
> data once it has been encrypted.
>
> In this case, the SSL module should just read from the file directly. It is
> kind of hokey, but that is because SSL just doesn't fit our filter model. The
> filters are written to allow for zero-copy, but SSL isn't a zero-copy
> operation in this case.
Bzzt... wrong. There are plenty of non-zero copy operations in the world... this
isn't the 'unique exception' ... it will be frequent enough to warrent redesign.
> Perhaps a better solution would be to just create a read function that
> accepts a pre-allocated buffer that we must copy into. Then, filters that
> know they are going to throw the data away would incur the penalty of the
> copy in lieu of the penalty of the malloc(). With the file bucket, this has the
> advantage that the read from the disk can be put directly into the
> pre-allocated buffer, and the copy doesn't actually exist.
Bingo. That way you haven't tied it up with specific bucket types - but extended
the api for all buckets.
This is where I keep saying that the consumer often knows more about the mechanics
of transfering or processing the data than the producer... until we add some
graceful methods for handling those cases, kludges will abound.
Bill
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Ryan Bloom <rb...@covalent.net>.
On Tuesday 27 November 2001 07:02 pm, Doug MacEachern wrote:
This seems straightforward to me. The bucket read function doesn't
make sense in this case, because we know that we are going to read
the entire file, and throw all of it away without ever actually using the
data once it has been encrypted.
In this case, the SSL module should just read from the file directly. It is
kind of hokey, but that is because SSL just doesn't fit our filter model. The
filters are written to allow for zero-copy, but SSL isn't a zero-copy
operation in this case.
The SSL filter will get much better performance if it can allocate a buffer
once, and just continue to read into that buffer. As long as it is careful
how it uses the file buckets, this won't be a problem.
Perhaps a better solution would be to just create a read function that
accepts a pre-allocated buffer that we must copy into. Then, filters that
know they are going to throw the data away would incur the penalty of the
copy in lieu of the penalty of the malloc(). With the file bucket, this has the
advantage that the read from the disk can be put directly into the
pre-allocated buffer, and the copy doesn't actually exist.
Ryan
> On Tue, 27 Nov 2001, William A. Rowe, Jr. wrote:
> > If that's true, we have some serious work to do. Do you have a good
> > sense for how much overhead we are talking about here [bucketwise?]
>
> see message below. i said 'putting ssl aside', but this actually only
> happens in the case of a filter such as ssl where we modify the contents
> of a file.
>
> you can add to the list ~1200 calls each:
> - to file_make_mmap() function (which always fail)
> - malloc sizeof(apr_bucket)
>
> Date: Sun, 25 Nov 2001 22:51:39 -0800 (PST)
> From: Doug MacEachern <do...@covalent.net>
> To: dev@httpd.apache.org
> Subject: Re: [patch] major ssl problem
> Message-ID: <Pi...@localhost>
>
> On Sat, 24 Nov 2001, Cliff Woolley wrote:
> > Actually, that's not exactly true. If APR_HAS_MMAP, and the file bucket
> > is between MMAP_THRESHOLD and MMAP_LIMIT (MMAP_LIMIT is 4MB by default),
> > then yes, len will be up to 4MB. But if the file bucket is bigger than
> > 4MB or the system doesn't have MMAP, then len will *never* be bigger than
> > APR_BUCKET_BUFF_SIZE (8KB).
>
> hmm..putting ssl aside, lets say we have a 10Mb file, looking at
> apr_buckets_file.c:file_read() that's ~1200 calls each to:
> - malloc for the 8k buffer
> - seek() for the current offset
> - read() of 8k for the actual buffer
>
> not to mention the APR_BRIGADE_FOREACH loop and whatever else is in
> between. seems that could use some performance tuning of its own?
--
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Doug MacEachern <do...@covalent.net>.
On Tue, 27 Nov 2001, William A. Rowe, Jr. wrote:
> If that's true, we have some serious work to do. Do you have a good sense
> for how much overhead we are talking about here [bucketwise?]
see message below. i said 'putting ssl aside', but this actually only
happens in the case of a filter such as ssl where we modify the contents
of a file.
you can add to the list ~1200 calls each:
- to file_make_mmap() function (which always fail)
- malloc sizeof(apr_bucket)
Date: Sun, 25 Nov 2001 22:51:39 -0800 (PST)
From: Doug MacEachern <do...@covalent.net>
To: dev@httpd.apache.org
Subject: Re: [patch] major ssl problem
Message-ID: <Pi...@localhost>
On Sat, 24 Nov 2001, Cliff Woolley wrote:
> Actually, that's not exactly true. If APR_HAS_MMAP, and the file bucket
> is between MMAP_THRESHOLD and MMAP_LIMIT (MMAP_LIMIT is 4MB by default),
> then yes, len will be up to 4MB. But if the file bucket is bigger than
> 4MB or the system doesn't have MMAP, then len will *never* be bigger than
> APR_BUCKET_BUFF_SIZE (8KB).
hmm..putting ssl aside, lets say we have a 10Mb file, looking at
apr_buckets_file.c:file_read() that's ~1200 calls each to:
- malloc for the 8k buffer
- seek() for the current offset
- read() of 8k for the actual buffer
not to mention the APR_BRIGADE_FOREACH loop and whatever else is in
between. seems that could use some performance tuning of its own?
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Doug MacEachern" <do...@covalent.net>
Sent: Tuesday, November 27, 2001 8:09 PM
> the specific case i'm looking at now is mod_ssl.
> if the filelength is > 4Mb, performance really blows chunks. calling
> apr_file_read (or xread) directly instead of apr_bucket_read() would
> improve things a great deal.
If that's true, we have some serious work to do. Do you have a good sense
for how much overhead we are talking about here [bucketwise?]
Sounds like some code worth stepping.
Bill
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Doug MacEachern <do...@covalent.net>.
the specific case i'm looking at now is mod_ssl.
if the filelength is > 4Mb, performance really blows chunks. calling
apr_file_read (or xread) directly instead of apr_bucket_read() would
improve things a great deal.
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 27 Nov 2001, Doug MacEachern wrote:
> ok. then how about a new apr_file_xread() function and changing
> bucket_read to use it?
>
> apr_status_t apr_file_xread(apr_file_t **f, void *buf,
> apr_size_t *nbytes, apr_off_t *offset,
> apr_pool_t *pool)
> {
Hmmm... doesn't quite seem right. How would you know when it was
appropriate to call apr_file_xread() rather than apr_file_read()? When
the file might be XTHREAD. But that's any time. :) So we're really
talking about replacing apr_file_read(), no? Either this logic would go
at the top of the existing apr_file_read() [whose argument list would
obviously change], or the existing apr_file_read() would become some
function private to APR that we call [we might do it that way to avoid
having to duplicate this logic across all APR platforms, for example].
Still, does it really seem right for apr_file_read() to change the
apr_file_t that was passed to it, not really. But that doesn't answer the
original question of when you'd call apr_file_xread over apr_file_read...
Anyhow, I'm willing to be convinced that this should be pushed down into
APR from APR-util in some way. It definitely shouldn't go UP into Apache,
though.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Doug MacEachern <do...@covalent.net>.
ok. then how about a new apr_file_xread() function and changing
bucket_read to use it?
something like:
apr_status_t apr_file_xread(apr_file_t **f, void *buf,
apr_size_t *nbytes, apr_off_t *offset,
apr_pool_t *pool)
{
apr_status_t rv;
#if APR_HAS_THREADS && !APR_HAS_XTHREAD_FILES
apr_int32_t flags;
#endif
#if APR_HAS_THREADS && !APR_HAS_XTHREAD_FILES
if ((flags = apr_file_flags_get(f)) & APR_XTHREAD) {
/* this file descriptor is shared across multiple threads and
* this OS doesn't support that natively, so as a workaround
* we must reopen the file into pool */
const char *fname;
apr_file_name_get(&fname, *f);
rv = apr_file_open(*f, fname, (flags & ~APR_XTHREAD), 0, pool);
if (rv != APR_SUCCESS)
return rv;
}
#endif
/* Handle offset ... */
rv = apr_file_seek(*f, APR_SET, offset);
if (rv != APR_SUCCESS) {
return rv;
}
return apr_file_read(*f, buf, nbytes);
}
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, Doug MacEachern wrote:
> main issue for me is, if the file cannot be mmapped, i want to avoid
> apr_bucket_read()/file_read() and just call apr_file_read() directly for
> better performance. i imagine other modules might want to do this as
> well. wouldn't want to see copy-n-paste of the XTHREAD logic in these
> cases.
The point is that you can't thread-safely read an XTHREAD file because of
the file pointer. Unless you serialize access to the file, that is. If
you have an XTHREAD file, multiple threads are mucking about with the file
pointer, which is jumping all over the place. You have no choice but to
seek. But there's a race condition between the time you seek and the time
you call file_read. We don't want to serialize obviously, so the only
choice is to reopen the file so that the new apr_file_t and corresponding
os file handle is private to the thread.
> as for mod_file_cache taking care of this, it too would only reopen
> the file on every request if we have to, no?
No. How would it know? The handler just sticks a file in a file bucket
and passes it down the filter chain. It's what happens in the filter
chain that determines whether we have to reopen it or not. If we never
try to read from the file, we can skip the reopen and use the cached
XTHREAD file descriptor with sendfile(). If we do try to read from it, we
need a thread-private descriptor. The handler has no information about
whether that will happen or not, and the handler doesn't even have control
of the execution at that point... it's all up the filters/buckets code.
> after all, it is mod_file_cache that sets the XTHREAD flag to begin
> with.
Because mod_file_cache has a single file descriptor that it wishes to
share across threads. It's up to APR/APR-Util to deal with that reality
from then on.
> just doesn't seem like bucket_read() is the right place for this
> logic.
It is. I promise. :)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Doug MacEachern <do...@covalent.net>.
On Tue, 27 Nov 2001, Cliff Woolley wrote:
> Nope, scratch that, it won't even happen in that case. Duh. The
> split/copied file buckets point to the _same_ apr_bucket_file struct and
> therefore the _same_ apr_file_t. Only once will we ever see that
> apr_file_t as APR_XTHREAD, at which point we reopen it as ~APR_XTHREAD and
> replace the apr_file_t in the shared apr_bucket_file struct.
>
> You'll only ever possibly reopen the file once for one family of file
> buckets [which basically means once per request max].
ok, i see that now. as for mod_file_cache taking care of this, it too
would only reopen the file on every request if we have to, no?
after all, it is mod_file_cache that sets the XTHREAD flag to begin with.
just doesn't seem like bucket_read() is the right place for this logic.
main issue for me is, if the file cannot be mmapped, i want to avoid
apr_bucket_read()/file_read() and just call apr_file_read() directly for
better performance. i imagine other modules might want to do this as
well. wouldn't want to see copy-n-paste of the XTHREAD logic in these
cases.
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, Cliff Woolley wrote:
> It's theoretically possible for it to happen if you've split or copied
> your file bucket _before reading from it_ the first time.
Nope, scratch that, it won't even happen in that case. Duh. The
split/copied file buckets point to the _same_ apr_bucket_file struct and
therefore the _same_ apr_file_t. Only once will we ever see that
apr_file_t as APR_XTHREAD, at which point we reopen it as ~APR_XTHREAD and
replace the apr_file_t in the shared apr_bucket_file struct.
You'll only ever possibly reopen the file once for one family of file
buckets [which basically means once per request max].
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: apr_buckets_file.c:file_read + XTHREAD
Posted by Cliff Woolley <cl...@yahoo.com>.
On Tue, 27 Nov 2001, Doug MacEachern wrote:
> if file_read() cannot mmap the the file, it does this:
>
> #if APR_HAS_THREADS && !APR_HAS_XTHREAD_FILES
> if ((flags = apr_file_flags_get(f)) & APR_XTHREAD) {
> /* this file descriptor is shared across multiple threads and
> * this OS doesn't support that natively, so as a workaround
> * we must reopen the file into a->readpool */
> const char *fname;
> apr_file_name_get(&fname, f);
>
> rv = apr_file_open(&f, fname, (flags & ~APR_XTHREAD), 0, a->readpool);
> if (rv != APR_SUCCESS)
> return rv;
>
> a->fd = f;
> }
> #endif
>
> in the case above does the file get re-opened everytime file_read() is
> called
It's theoretically possible for it to happen if you've split or copied
your file bucket _before reading from it_ the first time.
> if not the latter, any reason why it can't be that way? looks like
> the XTHREAD thinger is only used by mod_file_cache.c, shouldn't its
> handler take care of re-opening the file instead of file_read()?
No! The whole point of mod_file_cache is that we don't _want_ to reopen
the file on every request if we don't have to. :)
> re-open
That it will happen once is the more likely scenario. If the bucket has
not been split or copied, then we reopen the file once, turn off the
APR_XTHREAD flag, and replace the file bucket's a->fd with the re-opened,
not shared across threads file descriptor. Any copies or splits we do
after that which then have apr_bucket_read() called on them will see
~APR_XTHREAD and will skip this block.
> and apr_file_seek() for every file_read() doesn't seem right. and i
> don't see why apr_file_seek() should happen if the file isn't being
> re-opened, but it currently happens regardless of the XTHREAD flag.
Because if the file bucket is ever split or copied, your file pointer is
useless. There's not a really easy way to keep track of when you need to
seek and when you don't, so we just do it all the time. You could come
up with some kind of logic to set a variable that says whether it's been
split/copied or not, and if not, then you can skip the seek, but I think I
came up with a case once where even that would break.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA