On 7/07/2012 2:36 a.m., Alex Rousskov wrote:
On 07/05/2012 09:02 PM, Amos Jeffries wrote:
It sounds like another analysis of the places calling trimMemory() is
needed.
StoreEntry::trimMemory() is only called from StoreEntry::swapOut(),
before we are starting to swap data out (or skipping swap because the
entry is not swappable). Ideally, it should also be called from places
where we unlock entry's memory as well, but that is the way it was
written. Changing that is outside this bug scope and may be difficult.
I thought that was the point of r11969. To cause trimming of no longer
necessary non-cacheable/swappable object buffers. With this patch being
additional fixes for the unexpected side effect(s).
So that trimMemory is isolated to just trim like its name
suggests. The maybeTrim() decisions being outside functionality (in
StoreController?) and which is the only place calling e.trimMemory().
First of all, I would ignore the "maybe" aspect. The caller does not
rely on any trimming actually happening and some computations of how
much to trim (if anything!) happen deeper inside MemObject. We can add
"maybe" to a bunch of methods so that their names reflect the
possibility that nothing would be trimmed. Those old methods are present
in all Squid versions. I think renaming them just to polish the names
would cause more porting headaches than it would solve problems.
If I get your primary idea correctly, then instead of
StoreEntry::swapOut() calling e.trimMemory() it will call new
StoreController::trimMemory(e) and the controller would decide whether
to proceed with trimming and, if yes, will call e.trimMemory().
StoreEntry::trimMemory() will not have any new conditions added then.
If that is what you are suggesting, it sounds good to me, and I can do
that during commit of v3 patch (it would not change the functionality or
the order of the checks; just move a couple of checks to a different
class/method).
Yes that was what I meant. And yes it is a design change, the renamed
functions just make it abundantly clearer that portage is either not
possible or needs more than usual careful checking.
Looking at this v2 patch the Bug comment and IN_MEMORY test looks like
it should be inside the keepInLocalMemory() instead. Since things
already *in* local memory are obviously allowed there somehow. That is
probably wrong, but is clearly implied by the symbol names around it.
They were allowed into local memory because Squid cannot do I/O without
using local memory :-). Every byte read is placed in local memory first.
Um. Lets be clear on the three memory usages now. What I see you talking
about is:
0) transit memory - non-cacheables and unknown-cacheables, etc
1) local memory cache (non-shared cache_mem)
2) shared memory cache (cache_mem with workers)
(0) and (1) being overlapping right now due to the incomplete transition
away from global object index.
keepInLocalMemory() being the test to decide if an object is needed to
be kept whole for moving (0) -> (1) or (0)->(2) at some later point.
When its put that way the keep*() is more of a cacheability test. With
yoru focus being just on whether teh caches can accept the object, as
opposed to including whether the object is allowed to be stored by HTTP.
right?
The issue here is whether those local memory bytes can be forgotten
(i.e., the entry memory can be trimmed). The answer depends on the entry
state and on the memory caching module state/functionality. The latter
aspect was ignored for a while, until this patch.
HTH,
Alex.
