[PATCH] Unify sys_tkill() and sys_tgkill()

[PATCH] Unify sys_tkill() and sys_tgkill()

[Vadim Lobanov]

Hi,

The majority of the sys_tkill() and sys_tgkill() function code is
duplicated between the two of them. This patch pulls the duplication out
into a separate function -- do_tkill() -- and lets sys_tkill() and
sys_tgkill() be simple wrappers around it. This should make it easier to
maintain in light of future changes.

Diffed against 2.6.14-rc2.

Signed-off-by: Vadim Lobanov <vlobanov@speakeasy.net>

diff -Npru a/kernel/signal.c b/kernel/signal.c
--- a/kernel/signal.c	2005-09-22 18:32:48.000000000 -0700
+++ b/kernel/signal.c	2005-09-23 18:51:58.000000000 -0700
@@ -2262,26 +2262,13 @@ sys_kill(int pid, int sig)
 	return kill_something_info(sig, &info, pid);
 }

-/**
- *  sys_tgkill - send signal to one specific thread
- *  @tgid: the thread group ID of the thread
- *  @pid: the PID of the thread
- *  @sig: signal to be sent
- *
- *  This syscall also checks the tgid and returns -ESRCH even if the PID
- *  exists but it's not belonging to the target process anymore. This
- *  method solves the problem of threads exiting and PIDs getting reused.
- */
-asmlinkage long sys_tgkill(int tgid, int pid, int sig)
+static int do_tkill(int tgid, int pid, int sig)
 {
-	struct siginfo info;
 	int error;
+	struct siginfo info;
 	struct task_struct *p;

-	/* This is only valid for single tasks */
-	if (pid <= 0 || tgid <= 0)
-		return -EINVAL;
-
+	error = -ESRCH;
 	info.si_signo = sig;
 	info.si_errno = 0;
 	info.si_code = SI_TKILL;
@@ -2290,8 +2277,7 @@ asmlinkage long sys_tgkill(int tgid, int

 	read_lock(&tasklist_lock);
 	p = find_task_by_pid(pid);
-	error = -ESRCH;
-	if (p && (p->tgid == tgid)) {
+	if (p && ((tgid <= 0) || (p->tgid == tgid))) {
 		error = check_kill_permission(sig, &info, p);
 		/*
 		 * The null signal is a permissions and process existence
@@ -2305,47 +2291,40 @@ asmlinkage long sys_tgkill(int tgid, int
 		}
 	}
 	read_unlock(&tasklist_lock);
+
 	return error;
 }

+/**
+ *  sys_tgkill - send signal to one specific thread
+ *  @tgid: the thread group ID of the thread
+ *  @pid: the PID of the thread
+ *  @sig: signal to be sent
+ *
+ *  This syscall also checks the tgid and returns -ESRCH even if the PID
+ *  exists but it's not belonging to the target process anymore. This
+ *  method solves the problem of threads exiting and PIDs getting reused.
+ */
+asmlinkage long sys_tgkill(int tgid, int pid, int sig)
+{
+	/* This is only valid for single tasks */
+	if (pid <= 0 || tgid <= 0)
+		return -EINVAL;
+
+	return (do_tkill(tgid, pid, sig));
+}
+
 /*
  *  Send a signal to only one task, even if it's a CLONE_THREAD task.
  */
 asmlinkage long
 sys_tkill(int pid, int sig)
 {
-	struct siginfo info;
-	int error;
-	struct task_struct *p;
-
 	/* This is only valid for single tasks */
 	if (pid <= 0)
 		return -EINVAL;

-	info.si_signo = sig;
-	info.si_errno = 0;
-	info.si_code = SI_TKILL;
-	info.si_pid = current->tgid;
-	info.si_uid = current->uid;
-
-	read_lock(&tasklist_lock);
-	p = find_task_by_pid(pid);
-	error = -ESRCH;
-	if (p) {
-		error = check_kill_permission(sig, &info, p);
-		/*
-		 * The null signal is a permissions and process existence
-		 * probe.  No signal is actually delivered.
-		 */
-		if (!error && sig && p->sighand) {
-			spin_lock_irq(&p->sighand->siglock);
-			handle_stop_signal(sig, p);
-			error = specific_send_sig_info(sig, &info, p);
-			spin_unlock_irq(&p->sighand->siglock);
-		}
-	}
-	read_unlock(&tasklist_lock);
-	return error;
+	return (do_tkill(0, pid, sig));
 }

 asmlinkage long

Re: [PATCH] Unify sys_tkill() and sys_tgkill()

[Jesper Juhl]

On 9/24/05, Vadim Lobanov <vlobanov@speakeasy.net> wrote:
> Hi,
>
> The majority of the sys_tkill() and sys_tgkill() function code is
> duplicated between the two of them. This patch pulls the duplication out
> into a separate function -- do_tkill() -- and lets sys_tkill() and
> sys_tgkill() be simple wrappers around it. This should make it easier to
> maintain in light of future changes.
>

A few nitpicks ... :

[snip]
> +static int do_tkill(int tgid, int pid, int sig)

I would probably have made this

  static inline int do_tkill(int tgid, int pid, int sig)


[snip]
> +       if (p && ((tgid <= 0) || (p->tgid == tgid))) {

Why all the extra parenthesis?

   if (p && (tgid <= 0 || p->tgid == tgid)) {


[snip]
> +       return (do_tkill(tgid, pid, sig));

return is not a function

   return do_tkill(tgid, pid, sig);

[snip]
> +       return (do_tkill(0, pid, sig));

again, get rid of the pointless extra parens

   return do_tkill(0, pid, sig);


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: Serious time drift - clock running fast

[Howard Chu]

I'm having the same problem with 2.6.13, AMD64 X2, Asus A8V Deluxe 
motherboard. What's worse is that this is my local net's NTP server, so 
it's taking all my other machines' clocks along for the ride, and I'm 
losing my associations to the upper strata servers because the skew gets 
too great. (So ntpd needs to be restarted periodically.)

I've seen earlier reports on this list about the clock running twice 
normal speed. That's not what I'm seeing here; after several hours it's 
only ahead by 5 minutes at the moment. (The system has been up 20 days, 
but I restarted ntpd a few hours ago, and it resync'd via ntpdate at 
that point.) Maybe it would be running at 2X if I kill ntpd, I haven't 
checked that.

If it matters, I configured a 250Hz clock tick on this kernel.

-- 
  -- Howard Chu
  Chief Architect, Symas Corp.  http://www.symas.com
  Director, Highland Sun        http://highlandsun.com/hyc
  OpenLDAP Core Team            http://www.openldap.org/project/

Re: [PATCH] Unify sys_tkill() and sys_tgkill()

[Jörn Engel]

On Sat, 24 September 2005 16:52:28 +0200, Jesper Juhl wrote:
> 
> [snip]
> > +static int do_tkill(int tgid, int pid, int sig)
> 
> I would probably have made this
> 
>   static inline int do_tkill(int tgid, int pid, int sig)

Why?  It would only return the original duplication in binary form and
save a minimal amount of time for something already slow - a system
call.  With small caches, the code duplication could even waste more
performance than the missing function call would gain you.

Other nits were well-picked.

Jörn

-- 
You can't tell where a program is going to spend its time. Bottlenecks
occur in surprising places, so don't try to second guess and put in a
speed hack until you've proven that's where the bottleneck is.
-- Rob Pike

Re: [PATCH] Unify sys_tkill() and sys_tgkill()

[Jesper Juhl]

On 9/24/05, Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:
> On Sat, 24 September 2005 16:52:28 +0200, Jesper Juhl wrote:
> >
> > [snip]
> > > +static int do_tkill(int tgid, int pid, int sig)
> >
> > I would probably have made this
> >
> >   static inline int do_tkill(int tgid, int pid, int sig)
>
> Why?  It would only return the original duplication in binary form and
> save a minimal amount of time for something already slow - a system
> call.  With small caches, the code duplication could even waste more
> performance than the missing function call would gain you.
>
You are right, I didn't think that through properly.
Thank you for catching that.

> Other nits were well-picked.
>
Thanks.

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Unify sys_tkill() and sys_tgkill()

[Vadim Lobanov]

On Sat, 24 Sep 2005, Jesper Juhl wrote:

> On 9/24/05, Vadim Lobanov <vlobanov@speakeasy.net> wrote:
> > Hi,
> >
> > The majority of the sys_tkill() and sys_tgkill() function code is
> > duplicated between the two of them. This patch pulls the duplication out
> > into a separate function -- do_tkill() -- and lets sys_tkill() and
> > sys_tgkill() be simple wrappers around it. This should make it easier to
> > maintain in light of future changes.
> >
>
> A few nitpicks ... :
>
> [snip]
> > +static int do_tkill(int tgid, int pid, int sig)
>
> I would probably have made this
>
>   static inline int do_tkill(int tgid, int pid, int sig)

Probably not necessary to inline this, as per the previous comments.

> [snip]
> > +       if (p && ((tgid <= 0) || (p->tgid == tgid))) {
>
> Why all the extra parenthesis?
>
>    if (p && (tgid <= 0 || p->tgid == tgid)) {
>
>
> [snip]
> > +       return (do_tkill(tgid, pid, sig));
>
> return is not a function
>
>    return do_tkill(tgid, pid, sig);
>
> [snip]
> > +       return (do_tkill(0, pid, sig));
>
> again, get rid of the pointless extra parens
>
>    return do_tkill(0, pid, sig);

These are all no biggie. I'll remove the parens and resend.

> --
> Jesper Juhl <jesper.juhl@gmail.com>
> Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please      http://www.expita.com/nomime.html
>

Thanks for the comments.
-Vadim Lobanov