Re: SMP Rock Store and shared memory cache patches

On 05/12/2011 11:05 PM, Amos Jeffries wrote:
> On 13/05/11 08:52, Alex Rousskov wrote:
>> Hello,
>>
>> We are getting ready to post changes to support SMP Rock Store
>> cache_dir module and SMP shared memory support. If all the changes are
>> combined, the 13K-line patch is probably too big to be reviewed
>> properly. To improve chances of finding problems during review, I plan
>> to split the submission into three related but distinct parts:
>>
>> 1. Squid core changes. Store API changes. Shared memory cache.
>> 2. Rock Store addition.
>> 3. IPC additions: Shared memory and atomic operations primitives.
>>
>> It would be difficult to commit #1 and #3 separately without breaking
>> the build. Doing so would require separating the new shared memory cache
>> code and related Squid core changes from #1. Assuming we can address all
>> the issues during review, our commit options are:
>>
>> A. Commit #1 and #3 together. A large commit message would cover both.
>> B. Commit #1 and #3 separately. Build will fail between the commits.
>> C. Massage #1 and #3 so that they can be committed separately without
>> breaking the build.
>>
>> I think #1 and #3 can (and should) be reviewed separately, but I would
>> prefer not to spend time on making them truly independent as far as code
>> building is concerned. In other words, I would prefer to do A or B but
>> not C. What do you think is the best way forward here?
>
> I'm used to reviewing the release bundle patches (20-150K lines), so any
> way you want to split or not is fine by me for the things I check.
>
> As for commit, I don't see 1+3 then 2 as a problem. This is precisely
> the reason I've kept the minor fluff/polish going to 3.2, so trunk->3.2
> diff will not be an issue here.

Sounds good. After manually splitting the merge bundle into three parts
(posted to squid-dev), I know that the boundary between p2 and other
parts is fuzzy in some places. Our policy is that every commit should
not break trunk build, right? We will try to follow that policy when
finalizing the partial patches.

>> Another big question is portability and applicability to all supported
>> Squid environments. Currently, the new code will only build on platforms
>> with support for atomic operations such as __sync_fetch_and_add().
>> Moreover, the new code may not be suitable for some high-performance
>> environments because of the optimizations and assumptions that are
>> currently hard-coded (e.g., a 32KB max-size). That is, the code may not
>> build and some features may be "slower" than the current ones, depending
>> on the environment.
>>
>> On the other hand, the changes will build in the majority of cases (this
>> speculation is based on the assumed percentage of folks building using
>> recent GCC versions on Linux). The changes will also be an improvement
>> and sometimes a critical improvement for at least some high-performance
>> environments (we develop with real deployment examples in mind).
>
> FYI: I'm currently considering a trunk feature stable-enough for 3.2 if
> it passes the full test-builds scan on:
> CentOS (ICC compatibility).
> Natty (GCC 4.6.1 - most strict GCC we have)
> FreeBSD-7 (relatively strict GCC with non-Linux environment)
>
> The other have a large backlog of things to fix, with low community
> interest in getting them fixed, and no way for us to test them anyway.
> So caring where possible is great but not critical.
>
>
> Please run the ALPHA tests for those slaves on these patches before
> audit submission. Particularly since as you point out the fine details
> will be hard to uncover early.
> The slaves will need to test *one* patch at a time in the same
> configuration as planned to be committed. Bug in Jenkins; scheduling two
> patch builds for the one slave looses one of the patch upload files :(.

OK, we will make sure that the patched Squid builds on the above three
platforms using Hudson.

>> My current plan is to do the following:
>>
>> i) Try to make sure the code builds if the right shared memory and
>> atomic operation (IPC) primitives are not available. If those IPC
>> primitives are not available on the build platform, disable SMP Rock
>> Store and shared memory cache support.
>>
>> ii) Add a squid.conf option to disable SMP shared memory cache even if
>> it builds fine. The user will get the current non-shared memory cache
>> then. SMP Rock Store is already optional, as any other store module.
>>
>> iii) Leave it to others to discover proper IPC primitives on their
>> favorite platforms and to optimize/enhance Rock Store and shared memory
>> code for their environments. If the features are useful to many and
>> optional to all, we should not hold them hostage to 100% portability,
>> IMO.
>>
>> Any objections or better ideas? Do you think anything else is required
>> for the features to be accepted as far as build and performance
>> portability are concerned?
>
> Sounds good. You may want to test 1 and 3 as a single patch on the
> slaves. Then get them audited and committed and one cycle.
> Then repeat for 2 as a separate audit cycle.

Will try.

Thank you,

Alex.
Received on Sun May 15 2011 - 21:24:36 MDT