Re: [PATCH] SMP SSL session cache implementation

On 01/10/2014 01:04 AM, Amos Jeffries wrote:
> 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.

I tried to make most of the changes you are requests.

Also because StoreMap has many changes sinse this patch had developed, I
make some more changes to make StoreMap and MemMap similar.

>
>>
>> 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.

This is the goal.

>
> 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.

Yep.

>
> 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.

I am including the MemMap in this patch, just to fix it if required. I
will commit as separate patches.

>
> 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.

removed.

>>>
>>> * 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.

I let the name as is for now. In the future, if it is possible we should
merge MemMap and StoreMap to an Ipc::SharedCache class, or make them
kids of a Ipc::SharedCache class.

>>>
>>> * Please finalize and merge the MemMap feature separately so it is not
>>> tied to the SSL changes.

I will post it as separate patches

>>>
>>>
>>> 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.

This is becaus StoreMap uses sfileno. Just to have similar interfaces
for two classes.

>>>
>>> * please implement the XXX notes about FlexibleArray
>>> - XXX also in .h

done

>>>
>>> * +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.

done

>>> -
>>>
>>> * 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.

This method removed.

>>>
>>> * entryCount() / entryLimit() and possibly valid() should be unsigned
>>> type (size_t probably).

I did not change these two members, to have the same interface with
StoreMap class

>>>
>>>
>>> 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

Yep. fixed now.

>>>
>>>
>>> * 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.

We have a problem here. To convert MemMap to a general class we need to
make MemMapSlot a template class with template parameters the key size
and store data size.
This is also requires convert the MemMap class to a template class.

For now I did the following:
 - Renamed the SSL_SESSION_ID_SIZE and SSL_SESSION_MAX_SIZE to
MEMMAP_SLOT_KEY_SIZE and MEMMAP_SLOT_DATA_SIZE and I add some assertions
in support.cc related code to be sure that these defines are always
enough big to hold SSL shared session data.

I hope it is OK.

>>>
>>> * 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.

I fix it a litle. Now it is a small comment but I hope it is enough.

>>>
>>> * openForWriting() comment "finds, reservers space for " is not clear.
>>> - what exactly is is saying anyway?
fixed
>>> - there is also a missing empty line between this function declaration
>>> and the next ones comment.

fixed

>>>
>>> * 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?).

I fix the documentation here

>>>
>>> * please use SBuf instead of String for new variables (eg SBuf path)

ok.

>>>
>>> * 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').

fixed

>>>
>>> * How about moving waitingToBeFreed above the lock. That way the
>>> comments are more consistent with what the lock controls.

Do you mean on declaration members?
Now it is before lock.

>>>
>>> * 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.

I synced the MemMap class to StoreMap class. The states and State::*
removed now.

>>>
>>>
>>> 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
>>>

ok fixed

>>>
>>> in src/ssl/support.cc:
>>>
>>> * store_session_cb() and remove_session_cb() should be static yes?

They can 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.
If we return error in these cases will result to abort SSL connection.
This is wrong, we want to proceed with SSL negotiation even if we did
not store the session.

>>> - also the comment question should be marked with a TODO
I supose you mean the case we are failing to remove a session from
cache. I add a TODO on top of comment. However, looks that we do not
need to handle this case.

>>>
>>> * get_session_cb()
>>> - double-empty lines near the top of the function.

ok.

>>>
>>> * 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.

ok fixed.

>>>
>>>
>>> Amos
>>>
>>>
>