Re: [PATCH] SMP SNMP

On 29/01/11 07:04, Tsantilas Christos wrote:
> I am attaching the new version.
>
>
> On 01/27/2011 05:10 PM, Amos Jeffries wrote:
>> On 28/01/11 00:07, Tsantilas Christos wrote:
>>> The attached patch implements aggregation of SNMP responses, similar to
>>> how we aggregate some cache manager stats.
>>>
>>> The code contains changes that allow us to share some of the classes
>>> between Cache Manager and SNMP code:
>>>
>>> * implement the following base classes under the ipc directory/module:
>>> - Ipc::Forwarder (ipc/Forwarder{.cc,.h} files)
>>> - Ipc::Inquirer (ipc/Inquirer{.cc,.h} files)
>>> - Ipc::Request (ipc/Request{.cc,.h} files)
>>> - Ipc::Response (ipc/Response{.cc,.h} files)
>>>
>>> * fix the Mgr::Forwarder, Mgr::Inquirer, Mgr::Request and Mgr::Response
>>> classes to be implemented as kid classes of the equivalent ICP::*
>>> classes.
>>>
>>> Also implements for the SNMP the same mechanism used for cache manager:
>>> The SNMP requests forwarder to coordinator which collects the statistics
>>> from kids and aggregate them.
>>>
>>> This is a Measurement Factory project.
>>
>> Yay!
>>
>> minor niggle:
>> please update the documentation blurb to say Ipc::* instead of ICP::*
>>
>>
>> include/Range.h:
>> * is this change able to be applied separately? it looks like a bug fix
>> unrelated to the actual SMP changes.
>
> I think it is part of this patch. This change required because the
> "Range" class used in Snmp::Var and Snmp::Pdu classes
>

The new code depending on correct behaviour of Range is not relevant to
it's being an unrelated bug.
  Please commit this immediately along with the required casting changes
the existing code needs.

>> configure.ac:
>> * any particular reason for the "x" on src/snmpx/ ?
>> (see makefile changes before answering)
>> if not please rename. The SourceLayout plans call for all Squid SNMP
>> support ending up in src/snmp/
>
> I let it as is. We have already an libsnmp.a (the lowlevel snmp library)
> under the (topdir)/snmplib directory. It is not bad idea to call the new
> library libsnmpx.a
>

See my reply to Alex. If the removal of x does turn out to clash the
names we should fix the snmplib naming and location before this patch
goes in.

>> * please also AC_DEFINE() a wrapper macro USE_SNMP and use it around all
>> the new SNMP specific code.
>
> I used the already defined SQUID_SNMP macro which is used in many other
> places .

Okay fine for now.

>
>>
>> * the Makefile.am common syntax for internal library variables seems to
>> be name_LIBS. This will help avoid clashes with the third-party library
>> definition $(SNMPLIB) when the SNMPX->SNMP conversion requested above is
>> done.
>
> I am not fully understand what you mean here.
> I just rename the SNMPXLIB to SNMPX_LIBS
>
> Also please see my comment about renaming snmpx to snmp.
>
>>
>> * please combine the SNMP_LIBS setup and SNMP_ALL_SOURCE/SNMP_SOURCE
>> logics into one conditional block.
>
> Are we sure we want it?
> They are in different Makefile.am sections: Source files definitions and
> SUBIRS definition....
>

I'm sure. The makefile appears to be largely broken into feature
sections rather than file type.

>>
>> src/snmp_core.cc:
>> * CONF_ST_SWHIWM is a % and should not be combined. It is either
>> identical or a set of distinct config values. We might get away with the
>> min() value here, it looses some info though.
>
> Grr... I do not know which is the best way to handle this one.
>
> I put the min() value
>>
>> * same for CONF_ST_SWLOWM. we might get away with the min() value here
>> as well. also with a loss of info.
> And here I put the max() value.
>
> I am expecting that in most cases the cache_swap_low/cache_swap_high
> will have the same value for all kids so using min/max here will return
> the correct value.

Most times they will, there is the potential for individual worker cache
tuning though.

I suggest min() is the best for both. Since they mark the threshold % at
which the features begin to effect behaviour. In both cases the
behaviour of at least one worker will change at min().

I have plans to update these values to doubles. At which point the OID
may become a mesh of worker_ID:value.
That is delayed until sometime after rockstore etc have merged and wont
clash.

>
>>
>> * PERF_SYS_CURLRUEXP again is tricky to combine. best match would be a
>> min() for oldest timestamp.
>
> Currently it is just return "0".
>

Oh I see.

The meaning of LRU is "oldest timestamped object in cache, if LRU
algorithm is used". The fact that the workers are not presenting the
timestamp can be left for future fix. There is a bug open for that.

What this SMP support needs to do is aggregate via a special filter
equivalent to min() to retain the semantic oldest-object meaning. A
special one is needed that works as unsigned and ignores '0' values.

As I understand it, if there are no values available from any worker we
should return the SNMP no-data message instead of '0'.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.10
   Beta testers wanted for 3.2.0.4