Re: [PATCH] SMP SSL session cache implementation

On 11/01/2014 6:48 a.m., Tsantilas Christos wrote:
> 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.

Okay.

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

Kids I think is the best way to go there.

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

That is great.

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

Okay. Looks good now.

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

Okay. Thank you. That is fine then.

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

Hmm. It sounds like leaving it there could cause trouble later, but yes
is non-critical to the current transaction.

+1. Overall this one is looking much better. There is still an
double-empty line after the code for function Ipc::MemMap::Init(). But
that can be fixed on commit.

Thank you
Amos
Received on Sat Jan 11 2014 - 03:26:56 MST