pcp
[Top] [All Lists]

Re: [PATCH] pmda/memory_bandwidth: Add a new pmda to monitor the maximum

To: Hemant Kumar <hemant@xxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH] pmda/memory_bandwidth: Add a new pmda to monitor the maximum memory bandwidth
From: Nathan Scott <nathans@xxxxxxxxxx>
Date: Mon, 9 May 2016 23:08:03 -0400 (EDT)
Cc: pcp@xxxxxxxxxxx
Delivered-to: pcp@xxxxxxxxxxx
In-reply-to: <1462545988-24166-1-git-send-email-hemant@xxxxxxxxxxxxxxxxxx>
References: <1462545988-24166-1-git-send-email-hemant@xxxxxxxxxxxxxxxxxx>
Reply-to: Nathan Scott <nathans@xxxxxxxxxx>
Thread-index: 1K/XZTXHowYn39KyDmZo/DvKt7MyEg==
Thread-topic: pmda/memory_bandwidth: Add a new pmda to monitor the maximum memory bandwidth
Hi Hemant,

----- Original Message -----
> [...]
> The agent first parses the sysfs nodes directory and creates a list of
> numa nodes. Then, it parses the config file and verifies whether
> the node name specified in the .conf file has a match in the sysfs nodes
> list. If not, it throws an error. The agent updates the bandwidth
> information in the node_info struct. And, whenever a client asks for the
> information, it gives the values for each node:

Now that I see the code it seems a bit like overkill to have this live in
a new PMDA.  Since this is so simple (just the one metric, and some quite
simple logic to implement it), I'm wondering if we should just add it to
pmdalinux instead.  Or, do you think it will need to gain significantly in
complexity over time?

>  # pminfo | grep memory_bandwidth
>   memory_bandwidth.max
> 

Since pmdalinux already supports a NUMA node indom, and has mem.numa as a
tree already, perhaps a mem.numa.bandwidth [.max?] metric would suit here.
If the config file is present, then the PMDA could use it, else just report
no values for that metric.

Similar to pmdaproc and its hotproc configuration file, I think it would
be a good idea to have some identification at the head of that file - so a
Version number at least, so we can easily add to the syntax over time.  See
src/pmdas/linux_proc/samplehotproc.conf for a sample - pretty much the same
approach should work here (ship a sample, not enabled by default).

Other than those things, it all looks good to me - with some added QA and
docs as usual.

cheers.

--
Nathan

<Prev in Thread] Current Thread [Next in Thread>