All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andi Kleen <andi@firstfloor.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Ananth Mavinakayanahalli <ananth@in.ibm.com>,
	"Frank Ch. Eigler" <fche@redhat.com>, Ingo Molnar <mingo@elte.hu>,
	Peter Zijlstra <peterz@infradead.org>,
	Roland McGrath <roland@redhat.com>,
	linux-kernel@vger.kernel.org, utrace-devel@redhat.com
Subject: Re: [RFC,PATCH 14/14] utrace core
Date: Wed, 25 Nov 2009 09:46:17 +0100	[thread overview]
Message-ID: <20091125084617.GD29096@one.firstfloor.org> (raw)
In-Reply-To: <20091124214450.GA12828@redhat.com>

> This is subjective, but personally I disagree. Contrary, imho it
> is good that tracehook hides the (simple) details. I do not understand
> why the reader of, say, do_fork() should see the contents of
> tracehook_report_clone_complete(). This will complicate the understanding.

Someone who has to debug or review fork needs to know what's going on.

Yes they can find out by going through inlines, but that
just costs more time and distracts from the actual problem.

> Those people who want to understand/change fork() do not care about
> utrace/ptrace usually.
> 
> And please note that it is much, much easier to change this code
> when it lives in tracehooks.h instead of sched.c/signal.c/etc.

The problem is that when you have to trace this code when something
goes wrong the extra layer just holds you up. For debugging usually
abstraction is a bad idea.

My experience is also that in general such extra "abstraction layers"
are frowned upon in Linux kernel code style. For example when new
vendor drivers are submitted for hardware like NICs etc,
they frequently tend to have all kinds of "abstraction layers".
Typically the first step to linuxify them is to get rid of those. 

This makes the code more readable, shorter, better to debug and read.

Another classic example is: lock_foo() is frowned upon (Linus
tends to always complain about that), rather prefer spin_lock(&foo_lock)
or mutex_lock(&foo_lock) that makes it clear what's going on.

I don't see why this should be any different for utrace.

> > Less code obfuscation.
> >
> > When it's a utrace call, call it a utrace call, not something else.
> 
> Why do you think this is obfuscation? Well, we can rename these
> helpers, s/tracehook_/utrace_/, but I don't see how this can make
> the code more readable.

Because the inlines do not add anything to functionality and actually
hide what the code does, that is obfuscation. For you it might be obvious
because you've been hacking that code for quite some time, but for 
someone who is not in your position that's different.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

  reply	other threads:[~2009-11-25  8:46 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-11-24 20:02 [RFC,PATCH 14/14] utrace core Oleg Nesterov
2009-11-24 20:32 ` Andi Kleen
2009-11-24 20:41   ` Oleg Nesterov
2009-11-24 21:26     ` Andi Kleen
2009-11-24 21:31       ` Frank Ch. Eigler
2009-11-24 21:34         ` Andi Kleen
2009-11-24 21:44       ` Oleg Nesterov
2009-11-25  8:46         ` Andi Kleen [this message]
2009-11-25 14:55           ` Oleg Nesterov
2009-11-25 16:00             ` Ingo Molnar
2009-11-25 21:50   ` Christoph Hellwig
2009-12-01 23:47   ` Roland McGrath
2009-12-01 19:54 ` Peter Zijlstra
2009-12-01 22:08   ` Oleg Nesterov
2009-12-07 18:34     ` Peter Zijlstra
2009-12-08 15:04       ` Oleg Nesterov
2009-12-08 15:29         ` Peter Zijlstra
2009-12-08 16:31           ` Oleg Nesterov
2009-12-08 18:19             ` Peter Zijlstra
2009-12-08 18:37               ` Oleg Nesterov
2009-12-13 20:48               ` Roland McGrath
2009-12-08 15:35         ` Peter Zijlstra
2009-12-08 17:51           ` Oleg Nesterov
2009-12-02  5:44   ` Roland McGrath
2009-12-02 18:34   ` Oleg Nesterov
2009-12-02 18:49   ` Oleg Nesterov
2009-12-05 19:14     ` Roland McGrath
2009-12-14  0:25   ` Roland McGrath
2009-12-14 13:51     ` Peter Zijlstra
2009-12-14 17:41       ` Oleg Nesterov
2009-12-14 19:31         ` Oleg Nesterov
2009-12-14 19:42           ` Roland McGrath
2009-12-16 11:18       ` Roland McGrath
2009-12-14 17:03     ` Oleg Nesterov
2009-12-14 19:44       ` Roland McGrath
2009-12-14 20:24         ` Oleg Nesterov
2009-12-15  2:59           ` Roland McGrath

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20091125084617.GD29096@one.firstfloor.org \
    --to=andi@firstfloor.org \
    --cc=adobriyan@gmail.com \
    --cc=ananth@in.ibm.com \
    --cc=fche@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=roland@redhat.com \
    --cc=utrace-devel@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.