[Mesa-dev] [PATCH] HUD: Add support for block I/O, network I/O and lmsensor stats

[Emil Velikov]

Hi Steven,

On 28 September 2016 at 19:58, Steven Toth <stoth at kernellabs.com> wrote:
> V8: Feedback based on peer review
>     convert if block into a switch
>     Constify some func args
>
> V7: Increase precision when measuring lmsensors volts
>     Flatten patch series.
>
> V6: Feedback based on peer review
>     Simplify sensor initialization (arg passing).
>     Constify some func args
>
> V5: Feedback based on peer review
>     Convert sprintf to snprintf
>     Convert char * to const char *
>     int arg converted to bool
>     Func changes to take a filename vs a larger struct.
>     Omit the space between '*' and the param name.
>
> V4: Merged with master as of 2016/9/27 6pm
>
> V3: Flatten the entire patchset ready for the ML
>
> V2: Additional seperate patches based on feedback
> a) configure.ac: Add a comment related to libsensors
>
> b) HUD: Disable Block/NIC I/O stats by default.
> Implement configuration option --enable-gallium-extra-hud=yes
> and enable both statistics when this option is enabled.
>
> c) Configure.ac: Minor cleanup to user visible configuration settings
>
> d) Configure.ac: HUD stats - build system improvements
> Move the -lsensors out of a deeper Makefile, bring it into the configure.ac.
> Also, rename a compiler directive to more closely follow the standard.
>
> V1: Initial release to the ML
> Three new features:
> 1. Disk/block I/O device read/write stats MB/ps.
> 2. Network Interface RX/TX transfer statistics as a percentage
>    of the overall NIC speed.
> 3. lmsensor power, voltage and temperature sensors.
>
While I'm in favour of keeping a brief changelog within the summary
the actual commit message is getting lost in here :-\

> The lmsensor changes makes a dependency on libsensors so support
> for the change is opt out by default.
>
> Signed-off-by: Steven Toth <stoth at kernellabs.com>
> ---
>  configure.ac                                 |  42 +++
>  src/gallium/auxiliary/Makefile.am            |   2 +
>  src/gallium/auxiliary/Makefile.sources       |   3 +
>  src/gallium/auxiliary/hud/hud_context.c      |  73 +++++
>  src/gallium/auxiliary/hud/hud_diskstat.c     | 337 ++++++++++++++++++++
>  src/gallium/auxiliary/hud/hud_nic.c          | 446 +++++++++++++++++++++++++++
>  src/gallium/auxiliary/hud/hud_private.h      |  25 ++
>  src/gallium/auxiliary/hud/hud_sensors_temp.c | 374 ++++++++++++++++++++++
>  src/gallium/include/pipe/p_defines.h         |   4 +
>  9 files changed, 1306 insertions(+)
>  create mode 100644 src/gallium/auxiliary/hud/hud_diskstat.c
>  create mode 100644 src/gallium/auxiliary/hud/hud_nic.c
>  create mode 100644 src/gallium/auxiliary/hud/hud_sensors_temp.c
>
> diff --git a/configure.ac b/configure.ac
> index c702b53..1bfac3b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -91,6 +91,7 @@ XCBGLX_REQUIRED=1.8.1
>  XSHMFENCE_REQUIRED=1.1
>  XVMC_REQUIRED=1.0.6
>  PYTHON_MAKO_REQUIRED=0.8.0
> +LIBSENSORS_REQUIRED=4.0.0
>
>  dnl Check for progs
>  AC_PROG_CPP
> @@ -871,6 +872,32 @@ AC_ARG_ENABLE([dri],
>      [enable_dri="$enableval"],
>      [enable_dri=yes])
>
> +AC_ARG_ENABLE([gallium-extra-hud],
> +    [AS_HELP_STRING([--enable-gallium-extra-hud],
> +        [enable HUD block/NIC I/O HUD stats support @<:@default=disabled@:>@])],
> +    [enable_gallium_extra_hud="$enableval"],
> +    [enable_gallium_extra_hud=no])
Looking at some of your other patches makes me wonder. Is it worth
having this as a list - (maybe?) rename it to s/enable/with/ and have
the user provide a list of required parts - sensors, cpu freq, etc.

I'm thinking of cases where extra deps come to play and having a
toggle for each one will be insane :-\

If having X separate bits in the extra-hud sounds like and overkill,
how do you feel if we do
--with/enable-gallium-extra-hud={all,without-sensors}

> +AM_CONDITIONAL(HAVE_GALLIUM_EXTRA_HUD, test "x$enable_gallium_extra_hud" = xyes)
> +if test "x$enable_gallium_extra_hud" = xyes ; then
> +    DEFINES="${DEFINES} -DHAVE_GALLIUM_EXTRA_HUD=1"
> +fi
> +
> +#TODO: no pkgconfig .pc available for libsensors.
> +#PKG_CHECK_MODULES([LIBSENSORS], [libsensors >= $LIBSENSORS_REQUIRED], [enable_lmsensors=yes], [enable_lmsensors=no])
> +AC_ARG_ENABLE([lmsensors],
> +    [AS_HELP_STRING([--enable-lmsensors],
> +        [enable HUD lmsensor support @<:@default=disabled@:>@])],
> +    [enable_lmsensors="$enableval"],
> +    [enable_lmsensors=no])
> +AM_CONDITIONAL(HAVE_LIBSENSORS, test "x$enable_lmsensors" = xyes)
> +if test "x$enable_lmsensors" = xyes ; then
> +    DEFINES="${DEFINES} -DHAVE_LIBSENSORS=1"
> +    LIBSENSORS_LDFLAGS="-lsensors"
> +else
> +    LIBSENSORS_LDFLAGS=""
> +fi
> +AC_SUBST(LIBSENSORS_LDFLAGS)
You want the LIBS variable, LDFLAGS is something else.


>  endif
>
> +libgallium_la_LDFLAGS = $(LIBSENSORS_LDFLAGS)
> +
Please add the new variable to GALLIUM_COMMON_LIB_DEPS (in
src/gallium/Automake.inc)


> +
> +#define LOCAL_DEBUG 0
> +
This and the related debugging code does not seem too useful past the
development stage. Can we nuke it, please ?


Thanks
Emil