[PATCH 00/11] Task watchers: Introduction

[PATCH 00/11] Task watchers: Introduction

[Matt Helsley]

Task watchers is a notifier chain that sends notifications to registered
callers whenever a task forks, execs, changes its [re][ug]id, or exits.
The goal is to keep the these paths comparatively simple while
enabling the addition of per-task intialization, monitoring, and tear-down
functions by existing and proposed kernel features.

The first patch adds a global atomic notifier chain, registration
functions, and a function to invoke the callers on the chain.

Later patches:

Register a task watcher for process events, shuffle bits of process events
functions around to reduce the code, and turn it into a module. 

Switch task watchers from an atomic to a blocking notifier chain

Register task watchers for:
	Audit
	Per Task Delay Accounting (note: not the taskstats calls)
	Profile

Add a per-task raw notifier chain

Add a task watcher for semundo

Switch the semundo task watcher to a per-task watcher

I've broken up the patches this way for clarity, to allow cherry-picking, and
to help focus the discussion of any potentially controversial details.

Testing:

Each patch applies to 2.6.17-rc6-mm2, compiles, boots, and runs with
audit=1 profile=2 and auditctl -e 0 or 1 during a run of random system calls
with all config values set to y.

I've tested a variety of combinations with respect to the CONNECTOR
and PROC_EVENTS config values:
CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=y
CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=m
CONFIG_CONNECTOR=m, CONFIG_PROC_EVENTS=m
CONFIG_CONNECTOR=n, CONFIG_PROC_EVENTS=n

while all the rest were =y|n:
CONFIG_TASKSTATS=y
CONFIG_TASK_DELAY_ACCT=y
CONFIG_AUDIT=y
CONFIG_AUDITSYSCALL=y
CONFIG_PROFILING=y
CONFIG_OPROFILE=y

I also did some touch testing of process events with the modular configuration.

While I haven't done every combination of configuration possible I
believe this covers a wide number of common configurations.

Andrew, please consider these patches for inclusion in -mm. Assuming
there are no objections I'd like to aim the task watchers and process events
patches for 2.6.18.

I've Cc'd most of the folks who responded with comments last time and added
folks listed in MAINTAINERS for the audit and profile patches. If anyone
would like to be removed from future Ccs or if there's anyone I ought to add
please let me know.

Cheers,
	-Matt Helsley

-------------------------------------------------------------------------------
Here are some performance numbers for those interested in getting a
rough idea of how applying each patch affects performance. I ran the
benchmarks with two other patches I've posted recently that aren't in this
series:
	profile_make_notifier_blocks_read_mostly
	semundo_simplify

Kernbench
NUMAQ - 16 700MHz PIII processors, Debian Sarge

+Patch 0 - None
Elapsed: 100.99s User: 1163.63s System: 224.24s CPU: 1373.67%
1163.68user 224.49system 1:41.35elapsed 1369%CPU (0avgtext+0avgdata 0maxresident)k
1164.11user 222.03system 1:40.41elapsed 1380%CPU (0avgtext+0avgdata 0maxresident)k
1163.10user 226.21system 1:41.20elapsed 1372%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 0 - None (second run)
Elapsed: 100.57s User: 1163.10s System: 224.01s CPU: 1378.33%
1163.48user 223.40system 1:40.06elapsed 1385%CPU (0avgtext+0avgdata 0maxresident)k
1161.64user 224.99system 1:40.63elapsed 1377%CPU (0avgtext+0avgdata 0maxresident)k
1164.18user 223.64system 1:41.01elapsed 1373%CPU (0avgtext+0avgdata 0maxresident)k

+Patch - profile_make_notifier_blocks_read_mostly (Posted outside of this series)
Elapsed: 100.74s User: 1163.92s System: 223.10s CPU: 1376.66%
1164.58user 222.78system 1:41.33elapsed 1369%CPU (0avgtext+0avgdata 0maxresident)k
1163.26user 224.21system 1:40.17elapsed 1385%CPU (0avgtext+0avgdata 0maxresident)k
1163.93user 222.32system 1:40.73elapsed 1376%CPU (0avgtext+0avgdata 0maxresident)k

+Patch - semundo_simplify (Posted outside of this series)
Elapsed: 100.92s User: 1162.45s System: 224.52s CPU: 1373.67%
1163.16user 224.83system 1:40.85elapsed 1376%CPU (0avgtext+0avgdata 0maxresident)k
1162.95user 223.99system 1:41.33elapsed 1368%CPU (0avgtext+0avgdata 0maxresident)k
1161.23user 224.74system 1:40.58elapsed 1377%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 1 - task_watchers
Elapsed: 100.99s User: 1163.45s System: 224.78s CPU: 1374%
1163.45user 224.74system 1:40.30elapsed 1384%CPU (0avgtext+0avgdata 0maxresident)k
1164.55user 223.82system 1:41.12elapsed 1372%CPU (0avgtext+0avgdata 0maxresident)k
1162.34user 225.78system 1:41.56elapsed 1366%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 2 - add a process events task watcher
Elapsed: 100.87s User: 1163.36s System: 225.11s CPU: 1375.67%
1164.12user 225.32system 1:41.13elapsed 1373%CPU (0avgtext+0avgdata 0maxresident)k
1163.05user 226.86system 1:40.87elapsed 1377%CPU (0avgtext+0avgdata 0maxresident)k
1162.92user 223.16system 1:40.61elapsed 1377%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 3 - refactor process events
Elapsed: 100.66s User: 1162.81s System: 227.08s CPU: 1380.33%
1162.62user 226.87system 1:40.69elapsed 1379%CPU (0avgtext+0avgdata 0maxresident)k
1163.26user 226.93system 1:40.56elapsed 1382%CPU (0avgtext+0avgdata 0maxresident)k
1162.54user 227.45system 1:40.72elapsed 1380%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 4 - process events module
Elapsed: 101.06s User: 1162.57s System: 225.67s CPU: 1373%
1164.08user 224.63system 1:40.57elapsed 1380%CPU (0avgtext+0avgdata 0maxresident)k
1160.70user 226.88system 1:40.79elapsed 1376%CPU (0avgtext+0avgdata 0maxresident)k
1162.94user 225.50system 1:41.82elapsed 1363%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 5 - switch to use a blocking notifier chain
Elapsed: 102.52s User: 1162.54s System: 224.73s CPU: 1353.67%
1162.57user 224.95system 1:40.50elapsed 1380%CPU (0avgtext+0avgdata 0maxresident)k
1162.77user 224.85system 1:46.22elapsed 1306%CPU (0avgtext+0avgdata 0maxresident)k
1162.28user 224.39system 1:40.84elapsed 1375%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 6 - add an audit task watcher
Elapsed: 101.07s User: 1161.91s System: 224.90s CPU: 1371.33%
1162.64user 224.15system 1:40.79elapsed 1375%CPU (0avgtext+0avgdata 0maxresident)k
1162.21user 225.85system 1:41.50elapsed 1367%CPU (0avgtext+0avgdata 0maxresident)k
1160.88user 224.69system 1:40.92elapsed 1372%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 7 - add a delayacct task watcher
Elapsed: 101.00s User: 1163.59s System: 224.08s CPU: 1373.33%
1163.09user 225.15system 1:41.04elapsed 1373%CPU (0avgtext+0avgdata 0maxresident)k
1164.51user 221.55system 1:40.89elapsed 1373%CPU (0avgtext+0avgdata 0maxresident)k
1163.17user 225.55system 1:41.06elapsed 1374%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 8 - add a profile task watcher
Elapsed: 100.61s User: 1162.95s System: 224.36s CPU: 1378.33%
1163.38user 224.60system 1:40.93elapsed 1375%CPU (0avgtext+0avgdata 0maxresident)k
1162.59user 224.10system 1:41.00elapsed 1372%CPU (0avgtext+0avgdata 0maxresident)k
1162.88user 224.38system 1:39.89elapsed 1388%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 8 - add a profile task watcher (second run)
Elapsed: 100.95s User: 1164.07s System: 224.58s CPU: 1375%
1164.13user 224.69system 1:40.90elapsed 1376%CPU (0avgtext+0avgdata 0maxresident)k
1164.74user 224.09system 1:40.96elapsed 1375%CPU (0avgtext+0avgdata 0maxresident)k
1163.34user 224.96system 1:40.99elapsed 1374%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 9 - introduce per-task watchers
Elapsed: 100.76s User: 1162.49s System: 225.37s CPU: 1377%
1162.65user 225.80system 1:40.89elapsed 1376%CPU (0avgtext+0avgdata 0maxresident)k
1162.09user 225.44system 1:40.53elapsed 1380%CPU (0avgtext+0avgdata 0maxresident)k
1162.72user 224.86system 1:40.85elapsed 1375%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 10 - add a semundo task watcher
Elapsed: 101.27s User: 1163.71s System: 224.82s CPU: 1370.67%
1163.84user 224.10system 1:41.55elapsed 1366%CPU (0avgtext+0avgdata 0maxresident)k
1163.43user 224.76system 1:41.16elapsed 1372%CPU (0avgtext+0avgdata 0maxresident)k
1163.86user 225.59system 1:41.10elapsed 1374%CPU (0avgtext+0avgdata 0maxresident)k

+Patch 11 - switch the semundo task watcher to a per-task watcher
Elapsed: 100.96s User: 1162.93s System: 224.76s CPU: 1374%
1163.14user 223.37system 1:40.61elapsed 1378%CPU (0avgtext+0avgdata 0maxresident)k
1162.92user 226.02system 1:41.34elapsed 1370%CPU (0avgtext+0avgdata 0maxresident)k
1162.73user 224.88system 1:40.94elapsed 1374%CPU (0avgtext+0avgdata 0maxresident)k

--

Re: [PATCH 00/11] Task watchers: Introduction

[Andrew Morton]

On Tue, 13 Jun 2006 16:52:01 -0700
Matt Helsley <matthltc@us.ibm.com> wrote:

> Task watchers is a notifier chain that sends notifications to registered
> callers whenever a task forks, execs, changes its [re][ug]id, or exits.

Seems a reasonable objective - it'll certainly curtail (indeed, reverse)
the ongoing proliferation of little subsystem-specific hooks all over the
core code, will allow us to remove some #includes from core code and should
permit some more things to be loaded as modules.

But I do wonder if it would have been better to have separate chains for
each of WATCH_TASK_INIT, WATCH_TASK_EXEC, WATCH_TASK_UID, WATCH_TASK_GID,
WATCH_TASK_EXIT.  That would reduce the number of elements which need to be
traversed at each event and would eliminate the need for demultiplexing at
each handler.

Re: [PATCH 00/11] Task watchers: Introduction

[Peter Williams]

Matt Helsley wrote:
> Task watchers is a notifier chain that sends notifications to registered
> callers whenever a task forks, execs, changes its [re][ug]id, or exits.
> The goal is to keep the these paths comparatively simple while
> enabling the addition of per-task intialization, monitoring, and tear-down
> functions by existing and proposed kernel features.
> 
> The first patch adds a global atomic notifier chain, registration
> functions, and a function to invoke the callers on the chain.
> 
> Later patches:
> 
> Register a task watcher for process events, shuffle bits of process events
> functions around to reduce the code, and turn it into a module. 
> 
> Switch task watchers from an atomic to a blocking notifier chain
> 
> Register task watchers for:
> 	Audit
> 	Per Task Delay Accounting (note: not the taskstats calls)
> 	Profile
> 
> Add a per-task raw notifier chain

This feature is less useful than it could be in that it only allows a 
per-task raw notifier to be added to the current task.  For the per 
process CPU capping client that I'm writing, I'd like to be able to 
attach one of these to a task that's being forked (from the forking 
task).  Not being able to do this will force me to go to the expense of 
maintaining my own hash tables for locating my per task data.

On a related note, I can't see where the new task's notify field gets 
initialized during fork.

> 
> Add a task watcher for semundo
> 
> Switch the semundo task watcher to a per-task watcher
> 
> I've broken up the patches this way for clarity, to allow cherry-picking, and
> to help focus the discussion of any potentially controversial details.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH 00/11] Task watchers: Introduction

[Matt Helsley]

On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
> Matt Helsley wrote:
> > Task watchers is a notifier chain that sends notifications to registered
> > callers whenever a task forks, execs, changes its [re][ug]id, or exits.
> > The goal is to keep the these paths comparatively simple while
> > enabling the addition of per-task intialization, monitoring, and tear-down
> > functions by existing and proposed kernel features.
> > 
> > The first patch adds a global atomic notifier chain, registration
> > functions, and a function to invoke the callers on the chain.
> > 
> > Later patches:
> > 
> > Register a task watcher for process events, shuffle bits of process events
> > functions around to reduce the code, and turn it into a module. 
> > 
> > Switch task watchers from an atomic to a blocking notifier chain
> > 
> > Register task watchers for:
> > 	Audit
> > 	Per Task Delay Accounting (note: not the taskstats calls)
> > 	Profile
> > 
> > Add a per-task raw notifier chain
> 
> This feature is less useful than it could be in that it only allows a 
> per-task raw notifier to be added to the current task.  For the per 
> process CPU capping client that I'm writing, I'd like to be able to 
> attach one of these to a task that's being forked (from the forking 
> task).  Not being able to do this will force me to go to the expense of 
> maintaining my own hash tables for locating my per task data.

	You're right. The patches are missing the bit that allows the per-task
watcher to be copied on fork. I've got an patch which I'll post and CC
you on shortly. What it does is allow register_per_task_watcher() to
register for the child task.

> On a related note, I can't see where the new task's notify field gets 
> initialized during fork.

It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.

Cheers,
	-Matt Helsley

Re: [PATCH 00/11] Task watchers: Introduction

[Matt Helsley]

On Mon, 2006-06-19 at 03:24 -0700, Andrew Morton wrote:
> On Tue, 13 Jun 2006 16:52:01 -0700
> Matt Helsley <matthltc@us.ibm.com> wrote:
> 
> > Task watchers is a notifier chain that sends notifications to registered
> > callers whenever a task forks, execs, changes its [re][ug]id, or exits.
> 
> Seems a reasonable objective - it'll certainly curtail (indeed, reverse)
> the ongoing proliferation of little subsystem-specific hooks all over the
> core code, will allow us to remove some #includes from core code and should
> permit some more things to be loaded as modules.
> 
> But I do wonder if it would have been better to have separate chains for
> each of WATCH_TASK_INIT, WATCH_TASK_EXEC, WATCH_TASK_UID, WATCH_TASK_GID,
> WATCH_TASK_EXIT.  That would reduce the number of elements which need to be
> traversed at each event and would eliminate the need for demultiplexing at
> each handler.

	It's a good idea, and should have the advantages you cited. My only
concern is that each task watcher would have to (un)register multiple
notifier blocks. I expect that in most cases there would only be two.
Also, if we apply this to per-task notifiers it would mean that we'd
have a 6 raw notifier heads per-task.

	Would you like me to redo the patches as multiple chains? Alternately,
I could produce patches that apply on top of the current set.

Cheers,
	-Matt Helsley

PS: I've already picked up your warning fix.

Re: [PATCH 00/11] Task watchers: Introduction

[Andrew Morton]

On Wed, 21 Jun 2006 01:35:29 -0700
Matt Helsley <matthltc@us.ibm.com> wrote:

> On Mon, 2006-06-19 at 03:24 -0700, Andrew Morton wrote:
> > On Tue, 13 Jun 2006 16:52:01 -0700
> > Matt Helsley <matthltc@us.ibm.com> wrote:
> > 
> > > Task watchers is a notifier chain that sends notifications to registered
> > > callers whenever a task forks, execs, changes its [re][ug]id, or exits.
> > 
> > Seems a reasonable objective - it'll certainly curtail (indeed, reverse)
> > the ongoing proliferation of little subsystem-specific hooks all over the
> > core code, will allow us to remove some #includes from core code and should
> > permit some more things to be loaded as modules.
> > 
> > But I do wonder if it would have been better to have separate chains for
> > each of WATCH_TASK_INIT, WATCH_TASK_EXEC, WATCH_TASK_UID, WATCH_TASK_GID,
> > WATCH_TASK_EXIT.  That would reduce the number of elements which need to be
> > traversed at each event and would eliminate the need for demultiplexing at
> > each handler.
> 
> 	It's a good idea, and should have the advantages you cited. My only
> concern is that each task watcher would have to (un)register multiple
> notifier blocks. I expect that in most cases there would only be two.

OK.

> Also, if we apply this to per-task notifiers it would mean that we'd
> have a 6 raw notifier heads per-task.

hm, that's potentially a problem.

It's a lock and a pointer.  72 bytes in the task_struct.  I guess we can
live with that.

An alternatve would be to dynamically allocate it, but that'll hurt code
which uses the feature, and will be fiddly.

Perhaps six struct notifier_block *'s, which share a lock?  Dunno.

> 	Would you like me to redo the patches as multiple chains?

Well, how about you see how it looks, decide whether this is worth
pursuing.

It's hard to predict the eventual typical length of these chains.

> Alternately,
> I could produce patches that apply on top of the current set.

It depends on how many of the existing patches are affected.  If it's just
one or two then an increment would be fine.  If it's everything then a new
patchset I guess.

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

[Matt Helsley]

On Wed, 2006-06-21 at 02:07 -0700, Andrew Morton wrote:
> On Wed, 21 Jun 2006 01:35:29 -0700
> Matt Helsley <matthltc@us.ibm.com> wrote:
> 
> > On Mon, 2006-06-19 at 03:24 -0700, Andrew Morton wrote:
> > > On Tue, 13 Jun 2006 16:52:01 -0700
> > > Matt Helsley <matthltc@us.ibm.com> wrote:
> > > 
> > > > Task watchers is a notifier chain that sends notifications to registered
> > > > callers whenever a task forks, execs, changes its [re][ug]id, or exits.
> > > 
> > > Seems a reasonable objective - it'll certainly curtail (indeed, reverse)
> > > the ongoing proliferation of little subsystem-specific hooks all over the
> > > core code, will allow us to remove some #includes from core code and should
> > > permit some more things to be loaded as modules.
> > > 
> > > But I do wonder if it would have been better to have separate chains for
> > > each of WATCH_TASK_INIT, WATCH_TASK_EXEC, WATCH_TASK_UID, WATCH_TASK_GID,
> > > WATCH_TASK_EXIT.  That would reduce the number of elements which need to be
> > > traversed at each event and would eliminate the need for demultiplexing at
> > > each handler.
> > 
> > 	It's a good idea, and should have the advantages you cited. My only
> > concern is that each task watcher would have to (un)register multiple
> > notifier blocks. I expect that in most cases there would only be two.
> 
> OK.
> 
> > Also, if we apply this to per-task notifiers it would mean that we'd
> > have a 6 raw notifier heads per-task.
> 
> hm, that's potentially a problem.
> 
> It's a lock and a pointer.  72 bytes in the task_struct.  I guess we can
> live with that.

Happily the per-task chains are raw so each should be just a pointer
making the total 24 or 48 bytes (on 32 or 64-bit platforms
respectively).

> An alternatve would be to dynamically allocate it, but that'll hurt code
> which uses the feature, and will be fiddly.
> 
> Perhaps six struct notifier_block *'s, which share a lock?  Dunno.
> 
> > 	Would you like me to redo the patches as multiple chains?
> 
> Well, how about you see how it looks, decide whether this is worth
> pursuing.

OK. Should be interesing.

> It's hard to predict the eventual typical length of these chains.

That's understandable.

> > Alternately,
> > I could produce patches that apply on top of the current set.
> 
> It depends on how many of the existing patches are affected.  If it's just
> one or two then an increment would be fine.  If it's everything then a new
> patchset I guess.

It would affect most of them -- I'd need to change the bits that
register a notifier block. So I'll make a separate series.

Cheers,
	-Matt Helsley

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

[Peter Williams]

Matt Helsley wrote:
> On Wed, 2006-06-21 at 02:07 -0700, Andrew Morton wrote:
>> On Wed, 21 Jun 2006 01:35:29 -0700
>> Matt Helsley <matthltc@us.ibm.com> wrote:
>>
>>> On Mon, 2006-06-19 at 03:24 -0700, Andrew Morton wrote:
>>>> On Tue, 13 Jun 2006 16:52:01 -0700
>>>> Matt Helsley <matthltc@us.ibm.com> wrote:
>>>>
>>>>> Task watchers is a notifier chain that sends notifications to registered
>>>>> callers whenever a task forks, execs, changes its [re][ug]id, or exits.
>>>> Seems a reasonable objective - it'll certainly curtail (indeed, reverse)
>>>> the ongoing proliferation of little subsystem-specific hooks all over the
>>>> core code, will allow us to remove some #includes from core code and should
>>>> permit some more things to be loaded as modules.
>>>>
>>>> But I do wonder if it would have been better to have separate chains for
>>>> each of WATCH_TASK_INIT, WATCH_TASK_EXEC, WATCH_TASK_UID, WATCH_TASK_GID,
>>>> WATCH_TASK_EXIT.  That would reduce the number of elements which need to be
>>>> traversed at each event and would eliminate the need for demultiplexing at
>>>> each handler.
>>> 	It's a good idea, and should have the advantages you cited. My only
>>> concern is that each task watcher would have to (un)register multiple
>>> notifier blocks. I expect that in most cases there would only be two.
>> OK.
>>
>>> Also, if we apply this to per-task notifiers it would mean that we'd
>>> have a 6 raw notifier heads per-task.
>> hm, that's potentially a problem.
>>
>> It's a lock and a pointer.  72 bytes in the task_struct.  I guess we can
>> live with that.
> 
> Happily the per-task chains are raw so each should be just a pointer
> making the total 24 or 48 bytes (on 32 or 64-bit platforms
> respectively).
> 
>> An alternatve would be to dynamically allocate it, but that'll hurt code
>> which uses the feature, and will be fiddly.
>>
>> Perhaps six struct notifier_block *'s, which share a lock?  Dunno.
>>
>>> 	Would you like me to redo the patches as multiple chains?
>> Well, how about you see how it looks, decide whether this is worth
>> pursuing.
> 
> OK. Should be interesing.
> 
>> It's hard to predict the eventual typical length of these chains.
> 
> That's understandable.
> 
>>> Alternately,
>>> I could produce patches that apply on top of the current set.
>> It depends on how many of the existing patches are affected.  If it's just
>> one or two then an increment would be fine.  If it's everything then a new
>> patchset I guess.
> 
> It would affect most of them -- I'd need to change the bits that
> register a notifier block. So I'll make a separate series.

How about making WATCH_TASK_INIT and friends flags so that clients can 
then pass a mask (probably part of the notifier_block) that specifies 
which ones they wish to be notified of.  This would save unnecessary 
function calls.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH 00/11] Task watchers: Introduction

[Peter Williams]

Matt Helsley wrote:
> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
>> On a related note, I can't see where the new task's notify field gets 
>> initialized during fork.
> 
> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.

I think that's too late.  It needs to be done at the start of 
notify_watchers() before any other watchers are called for the new task.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH 00/11] Task watchers: Introduction

[Peter Williams]

Peter Williams wrote:
> Matt Helsley wrote:
>> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
>>> On a related note, I can't see where the new task's notify field gets 
>>> initialized during fork.
>>
>> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
>> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.
> 
> I think that's too late.  It needs to be done at the start of 
> notify_watchers() before any other watchers are called for the new task.

On second thoughts, it would simpler just before the WATCH_TASK_INIT 
call in copy_process() in fork.c.  It can be done unconditionally there.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH 00/11] Task watchers: Introduction

[Matt Helsley]

On Wed, 2006-06-21 at 21:41 +1000, Peter Williams wrote:
> Peter Williams wrote:
> > Matt Helsley wrote:
> >> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
> >>> On a related note, I can't see where the new task's notify field gets 
> >>> initialized during fork.
> >>
> >> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
> >> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.
> > 
> > I think that's too late.  It needs to be done at the start of 
> > notify_watchers() before any other watchers are called for the new task.

	I don't see why you think it's too late. It needs to be initialized
before it's used. Waiting until notify_per_task_watchers() is called
with WATCH_TASK_INIT does this.

> On second thoughts, it would simpler just before the WATCH_TASK_INIT 
> call in copy_process() in fork.c.  It can be done unconditionally there.
> 
> Peter

	That would work. It would not simplify the control flow of the code.
The branch for WATCH_TASK_INIT in notify_per_task_watchers() is
unavoidable; we need to call the parent task's chain in that case since
we know the child task's is empty.

	It is also counter to one goal of the patches -- reducing the "clutter"
in these paths. Arguably task watchers is the same kind of clutter that
existed before. However, it is a means of factoring such clutter into
fewer instances (ideally one) of the pattern.

Cheers,
	-Matt Helsley

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

[Matt Helsley]

On Wed, 2006-06-21 at 20:40 +1000, Peter Williams wrote:
> Matt Helsley wrote:
> > On Wed, 2006-06-21 at 02:07 -0700, Andrew Morton wrote:
> >> On Wed, 21 Jun 2006 01:35:29 -0700
> >> Matt Helsley <matthltc@us.ibm.com> wrote:

<snip>

> >>> Alternately,
> >>> I could produce patches that apply on top of the current set.
> >> It depends on how many of the existing patches are affected.  If it's just
> >> one or two then an increment would be fine.  If it's everything then a new
> >> patchset I guess.
> > 
> > It would affect most of them -- I'd need to change the bits that
> > register a notifier block. So I'll make a separate series.
> 
> How about making WATCH_TASK_INIT and friends flags so that clients can 
> then pass a mask (probably part of the notifier_block) that specifies 
> which ones they wish to be notified of.  This would save unnecessary 
> function calls.
> 
> Peter

	Yes, I was considering that. However, I realized that it still would
involve either multiple notifier blocks or significant, non-intuitive
changes in the notifier chain code so that one notifier block could be
registered on multiple chains.

	I'll keep this suggestion in mind.

Cheers,
	-Matt Helsley

Re: [PATCH 00/11] Task watchers: Introduction

[Peter Williams]

Matt Helsley wrote:
> On Wed, 2006-06-21 at 21:41 +1000, Peter Williams wrote:
>> Peter Williams wrote:
>>> Matt Helsley wrote:
>>>> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
>>>>> On a related note, I can't see where the new task's notify field gets 
>>>>> initialized during fork.
>>>> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
>>>> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.
>>> I think that's too late.  It needs to be done at the start of 
>>> notify_watchers() before any other watchers are called for the new task.
> 
> 	I don't see why you think it's too late. It needs to be initialized
> before it's used. Waiting until notify_per_task_watchers() is called
> with WATCH_TASK_INIT does this.

I probably didn't understand the code well enough.  I'm still learning 
how it all hangs together :-).

> 
>> On second thoughts, it would simpler just before the WATCH_TASK_INIT 
>> call in copy_process() in fork.c.  It can be done unconditionally there.
>>
>> Peter
> 
> 	That would work. It would not simplify the control flow of the code.
> The branch for WATCH_TASK_INIT in notify_per_task_watchers() is
> unavoidable; we need to call the parent task's chain in that case since
> we know the child task's is empty.
> 
> 	It is also counter to one goal of the patches -- reducing the "clutter"
> in these paths. Arguably task watchers is the same kind of clutter that
> existed before. However, it is a means of factoring such clutter into
> fewer instances (ideally one) of the pattern.

Maybe a few comments in the code to help reviewers such as me learn how 
it works more quickly would be worthwhile.

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.

2. A helper mechanism to allow a client that's being loaded as a module 
to visit all existing tasks and do whatever initialization it needs to 
do.  Without this every client would have to implement such a mechanism 
themselves (and it's not pretty).

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH 00/11] Task watchers: Introduction

[Matt Helsley]

On Thu, 2006-06-22 at 09:04 +1000, Peter Williams wrote:
> Matt Helsley wrote:
> > On Wed, 2006-06-21 at 21:41 +1000, Peter Williams wrote:
> >> Peter Williams wrote:
> >>> Matt Helsley wrote:
> >>>> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
> >>>>> On a related note, I can't see where the new task's notify field gets 
> >>>>> initialized during fork.
> >>>> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
> >>>> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.
> >>> I think that's too late.  It needs to be done at the start of 
> >>> notify_watchers() before any other watchers are called for the new task.
> > 
> > 	I don't see why you think it's too late. It needs to be initialized
> > before it's used. Waiting until notify_per_task_watchers() is called
> > with WATCH_TASK_INIT does this.
> 
> I probably didn't understand the code well enough.  I'm still learning 
> how it all hangs together :-).
> 
> > 
> >> On second thoughts, it would simpler just before the WATCH_TASK_INIT 
> >> call in copy_process() in fork.c.  It can be done unconditionally there.
> >>
> >> Peter
> > 
> > 	That would work. It would not simplify the control flow of the code.
> > The branch for WATCH_TASK_INIT in notify_per_task_watchers() is
> > unavoidable; we need to call the parent task's chain in that case since
> > we know the child task's is empty.
> > 
> > 	It is also counter to one goal of the patches -- reducing the "clutter"
> > in these paths. Arguably task watchers is the same kind of clutter that
> > existed before. However, it is a means of factoring such clutter into
> > fewer instances (ideally one) of the pattern.
> 
> Maybe a few comments in the code to help reviewers such as me learn how 
> it works more quickly would be worthwhile.

Good point. I'll keep this in mind as I consider the multi-chain
approach suggested by Andrew -- I suspect improvments in my commenting
will be even more critical there.

> 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.

	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.

	As far as the multi-chain approach is concerned, I'm still leaning
towards registering a single function with a mask describing what it
wants to be notified of.

> 2. A helper mechanism to allow a client that's being loaded as a module 
> to visit all existing tasks and do whatever initialization it needs to 
> do.  Without this every client would have to implement such a mechanism 
> themselves (and it's not pretty).

	Interesting idea. It should resemble existing macros. Something like:
	register_task_watcher(&my_nb, &unnoticed);
	for_each_unnoticed_task(unnoticed)
		...

> Peter

Re: [PATCH 00/11] Task watchers: Introduction

[Peter Williams]

Matt Helsley wrote:
> On Thu, 2006-06-22 at 09:04 +1000, Peter Williams wrote:
>> Matt Helsley wrote:
>>> On Wed, 2006-06-21 at 21:41 +1000, Peter Williams wrote:
>>>> Peter Williams wrote:
>>>>> Matt Helsley wrote:
>>>>>> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
>>>>>>> On a related note, I can't see where the new task's notify field gets 
>>>>>>> initialized during fork.
>>>>>> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
>>>>>> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.
>>>>> I think that's too late.  It needs to be done at the start of 
>>>>> notify_watchers() before any other watchers are called for the new task.
>>> 	I don't see why you think it's too late. It needs to be initialized
>>> before it's used. Waiting until notify_per_task_watchers() is called
>>> with WATCH_TASK_INIT does this.
>> I probably didn't understand the code well enough.  I'm still learning 
>> how it all hangs together :-).
>>
>>>> On second thoughts, it would simpler just before the WATCH_TASK_INIT 
>>>> call in copy_process() in fork.c.  It can be done unconditionally there.
>>>>
>>>> Peter
>>> 	That would work. It would not simplify the control flow of the code.
>>> The branch for WATCH_TASK_INIT in notify_per_task_watchers() is
>>> unavoidable; we need to call the parent task's chain in that case since
>>> we know the child task's is empty.
>>>
>>> 	It is also counter to one goal of the patches -- reducing the "clutter"
>>> in these paths. Arguably task watchers is the same kind of clutter that
>>> existed before. However, it is a means of factoring such clutter into
>>> fewer instances (ideally one) of the pattern.
>> Maybe a few comments in the code to help reviewers such as me learn how 
>> it works more quickly would be worthwhile.
> 
> Good point. I'll keep this in mind as I consider the multi-chain
> approach suggested by Andrew -- I suspect improvments in my commenting
> will be even more critical there.
> 
>> 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.

> 
> 	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.

> 
> 	As far as the multi-chain approach is concerned, I'm still leaning
> towards registering a single function with a mask describing what it
> wants to be notified of.

I think that will be less efficient than the function array.

> 
>> 2. A helper mechanism to allow a client that's being loaded as a module 
>> to visit all existing tasks and do whatever initialization it needs to 
>> do.  Without this every client would have to implement such a mechanism 
>> themselves (and it's not pretty).
> 
> 	Interesting idea. It should resemble existing macros. Something like:
> 	register_task_watcher(&my_nb, &unnoticed);
> 	for_each_unnoticed_task(unnoticed)
> 		...

Something like that.  It involved some tricky locking issues and was 
reasonably complex (which made providing it a good option when compared 
to each client implementing its own version).  Rather than trying to do 
this from scratch I'd advise getting a copy of the most recent PAGG 
patches and using that as a model as a fair bit of effort was spent 
ironing out all the problems involved.  It's not as easy as it sounds.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH 00/11] Task watchers: Introduction

[Matt Helsley]

On Thu, 2006-06-22 at 11:11 +1000, Peter Williams wrote:
> Matt Helsley wrote:
> > On Thu, 2006-06-22 at 09:04 +1000, Peter Williams wrote:
> >> Matt Helsley wrote:
> >>> On Wed, 2006-06-21 at 21:41 +1000, Peter Williams wrote:
> >>>> Peter Williams wrote:
> >>>>> Matt Helsley wrote:
> >>>>>> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
> >>>>>>> On a related note, I can't see where the new task's notify field gets 
> >>>>>>> initialized during fork.
> >>>>>> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
> >>>>>> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.
> >>>>> I think that's too late.  It needs to be done at the start of 
> >>>>> notify_watchers() before any other watchers are called for the new task.
> >>> 	I don't see why you think it's too late. It needs to be initialized
> >>> before it's used. Waiting until notify_per_task_watchers() is called
> >>> with WATCH_TASK_INIT does this.
> >> I probably didn't understand the code well enough.  I'm still learning 
> >> how it all hangs together :-).
> >>
> >>>> On second thoughts, it would simpler just before the WATCH_TASK_INIT 
> >>>> call in copy_process() in fork.c.  It can be done unconditionally there.
> >>>>
> >>>> Peter
> >>> 	That would work. It would not simplify the control flow of the code.
> >>> The branch for WATCH_TASK_INIT in notify_per_task_watchers() is
> >>> unavoidable; we need to call the parent task's chain in that case since
> >>> we know the child task's is empty.
> >>>
> >>> 	It is also counter to one goal of the patches -- reducing the "clutter"
> >>> in these paths. Arguably task watchers is the same kind of clutter that
> >>> existed before. However, it is a means of factoring such clutter into
> >>> fewer instances (ideally one) of the pattern.
> >> Maybe a few comments in the code to help reviewers such as me learn how 
> >> it works more quickly would be worthwhile.
> > 
> > Good point. I'll keep this in mind as I consider the multi-chain
> > approach suggested by Andrew -- I suspect improvments in my commenting
> > will be even more critical there.
> > 
> >> 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.

> > 
> > 	As far as the multi-chain approach is concerned, I'm still leaning
> > towards registering a single function with a mask describing what it
> > wants to be notified of.
> 
> I think that will be less efficient than the function array.

Well if I don't register with the mask there are other approaches that
wouldn't require the switch or the array.

> > 
> >> 2. A helper mechanism to allow a client that's being loaded as a module 
> >> to visit all existing tasks and do whatever initialization it needs to 
> >> do.  Without this every client would have to implement such a mechanism 
> >> themselves (and it's not pretty).
> > 
> > 	Interesting idea. It should resemble existing macros. Something like:
> > 	register_task_watcher(&my_nb, &unnoticed);
> > 	for_each_unnoticed_task(unnoticed)
> > 		...
> 
> Something like that.  It involved some tricky locking issues and was
> reasonably complex (which made providing it a good option when compared 
> to each client implementing its own version).  Rather than trying to do 
> this from scratch I'd advise getting a copy of the most recent PAGG
> patches and using that as a model as a fair bit of effort was spent 
> ironing out all the problems involved.  It's not as easy as it sounds.

	Yes, it does sound quite hairy.

	My feeling is that such code should be largely independent of task
watchers and I'd like to stay focused. So I have no plans to work on
something like "for_each_unnoticed_task()" for now.

Cheers,
	-Matt Helsley

Re: [PATCH 00/11] Task watchers: Introduction

[Peter Williams]

Matt Helsley wrote:
> On Thu, 2006-06-22 at 11:11 +1000, Peter Williams wrote:
>> Matt Helsley wrote:
>>> On Thu, 2006-06-22 at 09:04 +1000, Peter Williams wrote:
>>>> Matt Helsley wrote:
>>>>> On Wed, 2006-06-21 at 21:41 +1000, Peter Williams wrote:
>>>>>> Peter Williams wrote:
>>>>>>> Matt Helsley wrote:
>>>>>>>> On Wed, 2006-06-21 at 15:41 +1000, Peter Williams wrote:
>>>>>>>>> On a related note, I can't see where the new task's notify field gets 
>>>>>>>>> initialized during fork.
>>>>>>>> It's initialized in kernel/sys.c:notify_per_task_watchers(), which calls
>>>>>>>> RAW_INIT_NOTIFIER_HEAD(&task->notify) in response to WATCH_TASK_INIT.
>>>>>>> I think that's too late.  It needs to be done at the start of 
>>>>>>> notify_watchers() before any other watchers are called for the new task.
>>>>> 	I don't see why you think it's too late. It needs to be initialized
>>>>> before it's used. Waiting until notify_per_task_watchers() is called
>>>>> with WATCH_TASK_INIT does this.
>>>> I probably didn't understand the code well enough.  I'm still learning 
>>>> how it all hangs together :-).
>>>>
>>>>>> On second thoughts, it would simpler just before the WATCH_TASK_INIT 
>>>>>> call in copy_process() in fork.c.  It can be done unconditionally there.
>>>>>>
>>>>>> Peter
>>>>> 	That would work. It would not simplify the control flow of the code.
>>>>> The branch for WATCH_TASK_INIT in notify_per_task_watchers() is
>>>>> unavoidable; we need to call the parent task's chain in that case since
>>>>> we know the child task's is empty.
>>>>>
>>>>> 	It is also counter to one goal of the patches -- reducing the "clutter"
>>>>> in these paths. Arguably task watchers is the same kind of clutter that
>>>>> existed before. However, it is a means of factoring such clutter into
>>>>> fewer instances (ideally one) of the pattern.
>>>> Maybe a few comments in the code to help reviewers such as me learn how 
>>>> it works more quickly would be worthwhile.
>>> Good point. I'll keep this in mind as I consider the multi-chain
>>> approach suggested by Andrew -- I suspect improvments in my commenting
>>> will be even more critical there.
>>>
>>>> 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.

> 
>>> 	As far as the multi-chain approach is concerned, I'm still leaning
>>> towards registering a single function with a mask describing what it
>>> wants to be notified of.
>> I think that will be less efficient than the function array.
> 
> Well if I don't register with the mask there are other approaches that
> wouldn't require the switch or the array.
> 
>>>> 2. A helper mechanism to allow a client that's being loaded as a module 
>>>> to visit all existing tasks and do whatever initialization it needs to 
>>>> do.  Without this every client would have to implement such a mechanism 
>>>> themselves (and it's not pretty).
>>> 	Interesting idea. It should resemble existing macros. Something like:
>>> 	register_task_watcher(&my_nb, &unnoticed);
>>> 	for_each_unnoticed_task(unnoticed)
>>> 		...
>> Something like that.  It involved some tricky locking issues and was
>> reasonably complex (which made providing it a good option when compared 
>> to each client implementing its own version).  Rather than trying to do 
>> this from scratch I'd advise getting a copy of the most recent PAGG
>> patches and using that as a model as a fair bit of effort was spent 
>> ironing out all the problems involved.  It's not as easy as it sounds.
> 
> 	Yes, it does sound quite hairy.
> 
> 	My feeling is that such code should be largely independent of task
> watchers and I'd like to stay focused. So I have no plans to work on
> something like "for_each_unnoticed_task()" for now.

Yes, this won't be an issue until there's a client in a loadable module.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

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

[Matt Helsley]

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.

Cheers,
	-Matt Helsley

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

[Peter Williams]

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@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

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

[Chandra Seetharaman]

On Thu, 2006-06-22 at 16:29 +1000, Peter Williams wrote:

> 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.

I think it was about 2 years back. We weren't going to re-invent after
that point, we had a full implementation at that time.

If i remember correct, we concluded that some design constraints and
additional overhead were the reasons for not proceeding in that
direction.

BTW, if i remember correct, PAGG folks also promised to look at CKRM to
see if they could use CKRM instead.

chandra
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

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

[Peter Williams]

Chandra Seetharaman wrote:
> On Thu, 2006-06-22 at 16:29 +1000, Peter Williams wrote:
> 
>> 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.
> 
> I think it was about 2 years back. We weren't going to re-invent after
> that point, we had a full implementation at that time.
> 
> If i remember correct, we concluded that some design constraints and
> additional overhead were the reasons for not proceeding in that
> direction.
> 
> BTW, if i remember correct, PAGG folks also promised to look at CKRM to
> see if they could use CKRM instead.

I don't recall them promising that but I (personally) looked at CKRM and 
decided that its equivalent functionality was unsuitable as it only 
allowed one client (unlike PAGG and task watchers).  CKRM at that stage 
was one big amorphous lump and trying to use sub components wasn't easy 
as they had been designed to meet CKRM's needs only rather than 
providing some generally useful functionality.

I'm pleased to say that (unless I'm mistaken) that last bit is no longer 
true and CKRM is moving towards providing low level functionality that 
may be generally useful rather than just meeting CKRM's needs.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce