Re: [RFC] QoS params patch

[Valdis . Kletnieks]

On Wed, 26 Sep 2007 17:40:20 PDT, Mark Gross said:
(others here are probably better at spotting leaks and races than I am,
so I'm skipping those and picking other nits. ;)

> --- linux-2.6.23-rc8/kernel/Makefile	2007-09-26 13:54:54.000000000 -0700
> +++ linux-2.6.23-rc8-qos/kernel/Makefile	2007-09-26 14:06:38.000000000 -
0700
> @@ -9,7 +9,7 @@
>  	    rcupdate.o extable.o params.o posix-timers.o \
>  	    kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \
>  	    hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o \
> -	    utsname.o
> +	    utsname.o qos_params.o

So I don't get a choice in the matter if I will be dragging this thing
around in my kernel, even if I have no intention of using the functionality?

> + * This QoS design is best effort based.  Dependents register their QoS needs.
> + * Watchers register to keep track of the current QoS needs of the system.
> + *
> + * There are 3 basic classes of QoS parameter: latency, timeout, throughput
> + * each have defined units:
> + * latency: usec
> + * timeout: usec <-- currently not used.
> + * throughput: kbs (kilo byte / sec)

It's unclear whether these are registering a differing QoS request for each
process/container/whatever that asks for one, or if they're global across the
system.  Also, even though it's "best effort", it would be good to document
what the failure mode is if we get conflicting requests, or an overcommit
situation - do new requests get refused, or allowed and ignored, or allowed
and only sometimes fulfilled.  For instance, assume a gigabit ethernet,
and 3 processes ask for 400 mbits/sec each - who wins, who gets part of what
they asked for, and who loses and gets starved?

> + * User mode requirements on a QOS parameter register themselves to the
> + * subsystem by opening the device node /dev/... and writing there request to
> + * the node.  As long as the process holds a file handle open to the node the

/dev?  What /dev entry do you use for a network interface?  Should this
be a configfs creature instead, or maybe something else?

> +/* static helper functions */
> +static s32 max_compare(s32 v1, s32 v2)

Blech.  Is it time for the yearly stamp-out-reinvention-of-max() already?
The use of pointer functions is interesting, but I have to wonder if there's
not a better way...

> +#define PID_NAME_LEN sizeof("process_1234567890")
> +static char name[PID_NAME_LEN];

And then you just pass a pointer to this and kstrdup() it.  Why not kmalloc()
the space initially and just 'dep->name = name;' and be done with it?

General nit - why qos_power_*, when none of the supported QoS parameters
seem to be power-related?

Attachment: pgp00000.pgp
Description: PGP signature