Re: [PATCH] SMP SSL session cache implementation

On 2014-01-10 04:50, Tsantilas Christos wrote:
> On 01/09/2014 12:07 AM, Amos Jeffries wrote:
>> On 2014-01-09 07:30, Tsantilas Christos wrote:
>>> This patch implement SSL session cache sharing across SMP workers
>>> using
>>> shared memory. The following new squid configuration options added:
>>>
>>> - The "sslproxy_session_cache_size" option which sets the cache size
>>> to
>>> use for ssl session. Example usage:
>>> sslproxy_session_cache_size 4 MB
>>>
>>> - The "sslproxy_session_ttl" option which defines the time in
>>> seconds
>>> the ssl session is valid. Example usage:
>>> sslproxy_session_ttl 600
>>>
>>> This patch also investigates the Ipc::MemMap class to help squid
>>> developers implement shared caches for squid processes.
>>>
>>> This is a Measurement Factory project
>
> Some notes before proceed to any change in the patch. I need squid
> developers opinions.
>
> If you compare the MemMap and StoreMap code you will see that they are
> similar. The original goal was to keep the code similar in order to
> merge these two classes to one in the future.
>
> This is why some of the variable documentations does not make sense
> for
> this class, and this is why the sfileno is used instead of an unsigned
> integer type.

Thank you for explaining sfileno. Its seems weird but is okay. A code
comment to explain would not go amiss.
As for the code comments describing weirdness, please make them correct
for the class as of *this* submission. They can (and should) be changed
during that future merge to be appropriate for design decisions made
during those changes which likely turn out to be different from what we
currently assume. Making them correct for the now helps avoid people
breaking the current API.

>
> This is something I should mention when I posted the patch but this
> patch implemented a long time ago, and I had forgot it. I just re-read
> internal Measurement Factory mails about this. Sorry for this.
>
> I am suggesting to keep MemMap as is (with only the FlexibleArrays
> fixes) just to have it similar to StoreMap class.
>
> In the future we should merge the Ipc::StoreMap and Ipc::MemMap to a
> Ipc::SharedCache class. Then we can make more fixes here.
>
> Opinions?
>

 From the earlier description I was thinking you were intending to make
this MemMap a generic memory cache class in the sense that we could use
it for the non-Store caching Squid does (ie auth credentials, helper
results, DNS results) not just StoreMap HTTP objects. That is a base
class we could really use to fix the outstanding non-SMP components.

With that assumption it makes more sense to make StoreMap inherit from
this class and add the Store-specific bits on top. This class just doing
generic cache management details (key/ttl/add/remove/update) in
SMP-aware operations.

Given your plans, the request to separate SSL changes from MemMap
addition is even more important to be done so that future Store updates
do not result in someone inexperienced attempting to it themselves when
back-porting a future Store change.

Most of the requests are styling and documentation fixes. They do not
change with this extra detail, except the one about use of size_t can be
ignored.

>
>>
>> general issues:
>>
>> * Please do not add HERE macro in new code.
>>
>> * MemMap seems to be combining the concepts and operations of an
>> indexed
>> cache with timeouts. "map"'s do not have the same semantics. Perhaps
>> calling it Ipc::SharedCache would be better.
>>
>> * Please finalize and merge the MemMap feature separately so it is not
>> tied to the SSL changes.
>>
>>
>> in src/ipc/MemMap.cc:
>>
>> * why is sfileno being used to references slots? It is a type
>> specifically designed for operations matching inode filesystem
>> semantics. It is dragging in size limitations which memory caches are
>> perhaps better off without.
>>
>> * please implement the XXX notes about FlexibleArray
>> - XXX also in .h
>>
>> * +Ipc::MemMap::openForWriting()
>> - "XXX: the caller should do that" already seems to be implemented by
>> commenting out the setKey(). Please remove the whole line if it is
>> working as-is right now.
>> -
>>
>> * Ipc::MemMap::abortIo() comment about caller is irrelevant and wrong
>> (caller could be anything). The only relevant thing is what state the
>> *slot* is in and the code explains that well enough.
>> - Please remove that comment.
>>
>> * entryCount() / entryLimit() and possibly valid() should be unsigned
>> type (size_t probably).
>>
>>
>> in src/ipc/MemMap.h:
>>
>> * Please update class members order/layout to match Squid-3 coding
>> guidelines.
>> - see
>> http://wiki.squid-cache.org/Squid3CodingGuidelines#Class_declaration_layout
>>
>> * If MemMap is supposed to be used outside SSL why is it depending on
>> macros named SSL_SESSION_* ?
>> - please rename those to something more generic.
>>
>> * comment explaining class MemMap appears to have multiple sentences.
>> Please use upper case and full stops. The doxygen output will run
>> those
>> lines into a single sentence if you dont write clearly.
>>
>> * openForWriting() comment "finds, reservers space for " is not clear.
>> - what exactly is is saying anyway?
>> - there is also a missing empty line between this function
>> declaration
>> and the next ones comment.
>>
>> * comment "notified before a readable entry is freed" for cleaner.
>> Please clarify what cleaner is in relation to MemMap and how "notify"
>> happens (callback? require class method?).
>>
>> * please use SBuf instead of String for new variables (eg SBuf path)
>>
>> * Ipc::MemMapSlot::set( ... expireOn)
>> - s/expireOn/expireAt/ - English weird grammar. time_t is treated
>> as
>> absolute value so 'at' (as opposed to offset in a scale which would be
>> 'on').
>>
>> * How about moving waitingToBeFreed above the lock. That way the
>> comments are more consistent with what the lock controls.
>>
>> * please update documentation of State::Writeable. "transitions from
>> Empty to Readable" does not explain anything unless one already knows
>> what the state means.
>> - perhapse Writeable should be described "in process of being written
>> to, not Empty and not yet Readable". If so then it should also
>> probably
>> be renamed "Writing" to match the active semantic.
>>
>>
>> in src/main.cc:
>>
>> * the #if USE_SSL macro does not need adding twice in a row. Please
>> combine the USE_SSL operations under one wrapper #if...#endif
>>
>>
>> in src/ssl/support.cc:
>>
>> * store_session_cb() and remove_session_cb() should be static yes?
>>
>> * why is store_session_cb() always returning 0?
>> - surely the absence of a session cache when the callback occurs
>> should
>> be an error.
>> - also the comment question should be marked with a TODO
>>
>> * get_session_cb()
>> - double-empty lines near the top of the function.
>>
>> * Ssl::initialize_session_cache()
>> - the for loops seem to be doing a lot of useless walking in the case
>> that "else SslSessionCache = NULL;". Please return immediately after
>> setting that =NULL value.
>>
>>
>> Amos
>>
>>
Received on Thu Jan 09 2014 - 23:04:52 MST