On 07/06/2012 07:11 PM, Amos Jeffries wrote:
> 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).
The point of r11969 was to trim unswappable objects. The bug in that
patch was that we also started trimming objects that should have been
preserved for the memory cache. This patch fixes that bug.
>>> 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.
Since we are not changing the lower-level MemObject methods that do the
actual trimming, I will not rename them. I will add a "maybe" prefix to
StoreEntry::trimMemory(). I hope this is a reasonable compromise.
>>> 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
yes, everything.
> 1) local memory cache (non-shared cache_mem)
yes, by default (one can enable shared memory cache in non-SMP mode)
> 2) shared memory cache (cache_mem with workers)
yes, by default (one can disable to use local memory caches in SMP)
> (0) and (1) being overlapping right now due to the incomplete transition
> away from global object index.
Yes, they use the same storage and the same index for now.
> 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.
Yes, for _possibly_ moving into a memory cache 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.
The keep*() method checks whether the memory cache may accept the object
now or in the future. To avoid code duplication, the check is split into
general memory cachability (in any memory cache) and the admission
policy of the memory cache (a specific cache). The general checks are in
StoreEntry::memoryCachable().
Hope this clarifies,
Alex.
Received on Sat Jul 07 2012 - 04:08:40 MDT
This archive was generated by hypermail 2.2.0 : Fri Jul 13 2012 - 12:00:03 MDT