Re: SMP Rock Store and shared memory cache patches

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.

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

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

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.12
   Beta testers wanted for 3.2.0.7 and 3.1.12.1