Re: [PATCH 1/2] utrace core

[Alexey Dobriyan <adobriyan@gmail.com>]

On Wed, Sep 03, 2008 at 05:11:27AM -0700, Roland McGrath wrote:
> > Again, embed struct utrace directly into task_struct. task_struct
> > lifetime rules are way more tested than struct utrace ones.
> 
> The most consistent feedback I've seen to all new features is that they
> mustn't add overhead when they're not being used.

That's correct but shouldn't be taken to extreme as usual.

> So I never considered it an option to bloat task_struct by ~120 bytes.

By how many?

> Of course much more than that is entailed when a task is actually being
> traced somehow.

Engine part is irrelevant for deciding "struct utrace utrace;" vs
"struct utrace *utrace;"

> But the presumption is that most tasks most of the time aren't, and that's
> what not to bloat.

Let's actually measure something.

1) clean kernel (my usual config, x86_64, NR_CPUS=2, PREEMPT=y, SLUB_DEBUG=y)
2) struct utrace *utrace;
3) struct utrace utrace; (without rcu_head, without check_dead)

			taken from symlink
	cache		/object_size, bytes
----------------------------------------
1)	task_struct	1392/1384

2)	task_struct	1408/1400
	utrace		72/72

3)	task_struct	1456/1456


I don't know why "object_size" differs from to where symlink points to
but it's irrelevant.

So, your approach gives +16(+72) bytes, mine gives +72 bytes.

So all this complexity is for ~56 bytes in untraced case. In traced
case, more memory will be used.

For 32-bit difference should be smaller because pointers are smaller.

> The revamp of the API after the first prototype made some of the internals
> much simpler to implement, that had been very sticky in the old prototype
> code.  But the allocation and freeing of struct utrace is an area I did not
> fully revisit.  Buggy is buggy, and sure it needs to be tested and fixed.
> I'm still inclined to look into making it right rather than punting.
> 
> > Add simple spinlock guarding all accesses (OK, I haven't looked very
> > closely if it's possible)
> 
> I can't tell what you mean here.  Do you mean something different 
> from struct utrace.lock?  If there were no pointer and its allocation
> to synchronize, then what other lock would you be adding?

I mean sticking to simple locking rules if you agree to "struct utrace utrace;":
1) utrace_flags go into struct utrace
2) utrace.lock guards _everything_ under it, no exceptions.

If I understand correctly ->utrace_flags are under utrace->lock, BUT
struct utrace is reattachable itself. Of course you protect against it,
but this doesn't help in convincing someone that there are no problems.

Again, this is just general observation and an example. Checking for
bugs will be much, much simpler.