Re: [PATCH v2] Do not release entries that may be kept in local memory cache.

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.
Received on Sat Jul 07 2012 - 01:11:27 MDT