Re: [Lse-tech] [PATCH 00/11] Task watchers: Introduction

From: Peter Williams
Date: Thu Jun 22 2006 - 02:28:31 EST


Matt Helsley wrote:
On Thu, 2006-06-22 at 14:26 +1000, Peter Williams wrote:

<snip>

BTW as a former user of PAGG, I think there are ideas in the PAGG implementation that you should look at. In particular:

1. The use of an array of function pointers (one for each hook) can cut down on the overhead. The notifier_block only needs to contain a pointer to the array so there's no increase in the size of that structure. Within the array a null pointer would mean "don't bother calling". Only one real array needs to exist even for per task as they're all using the same functions (just separate data). It removes the need for a switch statement in the client's function as well as saving on unnecessary function calls.
I don't think having an explicit array of function pointers is likely
to be as fast as a switch statement (or a simple branch) generated by
the compiler.
With the array there's no need for any switch or branching. You know exactly which function in the array to use in each hook.
I don't forsee enough of a difference to make this worth arguing about.
You're welcome to benchmark and compare arrays vs. switches/branches on
a variety of archs, SMP boxen, NUMA boxen, etc. and post the results.
I'm going to focus on other issues for now.

It doesn't save unecessary function calls unless I modify the core
notifier block structure. Otherwise I still need to stuff a generic
function into .notifier_call and from there get the pointer to the array
to make the next call. So it adds more pointer indirection but does not
reduce the number of intermediate function calls.
There comes a point when trying to reuse existing code is less cost effective than starting over.
Write my own notifier chains just to avoid a function call? I don't
think that's sufficient justification for implementing my own.
Can't help thinking why the easier option of adding setuid and setgid hooks to PAGG and then including PAGG wasn't adopted.

Task watchers is not intended to group tasks. It's intended to factor a
common pattern out of these paths in a way useful to existing parts of
the kernel, proposed changes, and modules.

Your goal of grouping tasks seems like it could use task watchers. That
does not mean that every task watcher needs to manage groups of tasks.

The same is true of PAGG (in spite of the implication in its name). It's really just a general task tracking and call back mechanism (with an ability to store per task data in a way that is easy to find from a call back -- just like per task watchers) and grouping is just one of things it can be used for. A lot of work went into making it safe and relatively easy to use from modules. It's a pity to see all that work go to waste.

Admittedly, it didn't have hooks for setuid and setgid but that would have been easy to fix. Easier than getting task watchers to the same level of maturity and ease of use. Of course, the ease of use issues won't bite until somebody tries to do something substantial with task watchers from a loadable module as (once you get the hang of them) task watchers are quite easy to use from inside the kernel.

A lot of work went to make the call back mechanisms in PAGG efficient as well but (like you) I don't think that's a very big issue as the hooks aren't on a busy path.

Peter
PS A year or so ago the CKRM folks promised to have a look at using PAGG instead of inventing their own but I doubt that they ever did.
--
Peter Williams pwil3058@xxxxxxxxxxxxxx

"Learning, n. The kind of ignorance distinguishing the studious."
-- Ambrose Bierce
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/