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
