Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core
From: Andrew Morton
Date: Tue Jun 22 2004 - 03:56:17 EST
Mikael Pettersson <mikpe@xxxxxxxxx> wrote:
>
> perfctr-2.7.3 for 2.6.7-rc1-mm1, part 1/6:
>
OK, I spent a couple hours on the perfctr code. It looks sane, although a
bit hard to follow. The above changelog (which is _all_ you gave us) makes
this large and complex patch hard to review, hard to understand, hard for
others to support.
One can look at the code, sort-of-see what it's doing, query micro-issues,
but it is hard (and a bad use of one's time) to try and reverse-engineer
the big-picture "how it all hangs together" from the implementation.
We need (especially at this stage in the kernel cycle) at least a couple of
pages of design documentation which describes
- the major data structures
- the relationships between them
- their lifetime rules.
- a general overview of how the whole thing operates (what's
PERFCTR_INTERRUPT_SUPPORT do? What interrupts are generated? Describe
the backpatching design, etc). What is "forbidden" on p4 siblings, and
why and what is the implication of this? etcetera.
- the thread/process/data structure inheritance/sharing/creation models
- any known shortcomings, rejected design decisions, etc.
- the core<->arch-specific interface
Also there should be a document or a manpage or something which describes,
in detail:
- the user/kernel API (separate document, probably)
- what's in that filesystem, and how the objects in it are used.
- the sysfs interface
- the relationship with ptrace
Random points:
- In __vperfctr_set_cpus_allowed(), is it possible for a process to
generate that printk deliberately, thus spamming the logs?
- perfctr_set_cpus_allowed() does task_lock(). Should that be
vperfctr_task_lock() instead?
Please update the locking comment over task_lock() to represent this
new usage.
Note that printk() can call wake_up(), which takes scheduler locks. So
you've introduced a lock ranking here. Looks to be OK though.
- (What is the thread/process/child inheritance model for perfctr
anyway?)
- Why does sys_vperfctr_open() call ptrace_check_attach()? (I suspect
I'd know that if there was API documentation?)
- cpus_copy_to_user() has the arguments in the wrong order, and should
have sparse annotation added, please.
- <canofworms>cannot cpus_copy_to_user() share code with
sys_sched_getaffinity()?</canofworms>
- Sometime all the new syscalls need sparse annotation added, propagated
into callees.
- Can perfctr_init() and perfctr_exit() have static scope?
- This
cache = &get_cpu_cache();
looks cumbersome. It'd be nice to add the & to get_cpu_cache() itself.
- In perfctr_cpu_init():
perfctr_cpu_init_one(NULL);
smp_call_function(perfctr_cpu_init_one, NULL, 1, 1);
use on_each_cpu() here. Ditto perfctr_cpu_exit(), other places.
- Why does perfctr_cpu_init() do preempt_disable()? Needs a comment.
Ditto perfctr_cpu_exit().
- Why does vperfctr_alloc() allocate an entire page? (Needs comment)
- Why is that page reserved?
- The top-level Kconfig help should perhaps contain some handy URLs.
(Except there don't seem to be any :()
- stack space.
struct vperfctr_control is 348 bytes, do_vperfctr_read() uses 500 bytes
of stack and does copy_to_user(), which can cause tremendously deep
callchains (think: it can call into XFS and then into the qlogic driver ;))
These big structures should be dynamically allocated.
sizeof(struct perfctr_cpu_state) is 708.
- ooh, it has a filesystem, and something can be mmapped. API documentation? ;)
- why is the filesystem kern_mount()ed?
- why are the inodes initialised to state I_DIRTY?
- In sys_vperfctr_open(), can the `if (!vperfctr_fs_init_done())' test
actually return true?
- Can the presence of thread_struct.perfctr be dependent upon
CONFIG_PERFCTR?
- If task A creates itself a node in that new filesystem (what's the
naming schema there? What facilities does the filesystem offer? Why was
an fs interface chosen?) and task B opens that node, then task A exits,
what keeps the task_struct at the node's file->f_private_data->owner
valid?
What happens in this situation? Is it valid usage? Should the read fail?
- Are functions like p5_write_control() preempt-protected?
- Is there much point in making CONFIG_PERFCTR_VIRTUAL optional?
-
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/