2005-03-31 07:50:31

by Yum Rayan

Subject: [PATCH] Reduce stack usage in time.c

Attempt to reduce stack usage in time.c (linux-2.6.12-rc1-mm3). Stack
usage was noted using checkstack.pl. Specifically:

Before patch
------------
sys_adjtimex - 128

After patch
-----------
sys_adjtimex - none (register usage only)

Signed-off-by: Yum Rayan <[email protected]>

--- a/kernel/time.c 2005-03-25 22:11:06.000000000 -0800
+++ b/kernel/time.c 2005-03-30 16:59:51.000000000 -0800
@@ -413,17 +413,27 @@

asmlinkage long sys_adjtimex(struct timex __user *txc_p)
{
- struct timex txc; /* Local copy of parameter */
- int ret;
+ struct timex *txc; /* Local copy of parameter */
+ int retval;
+
+ txc = kmalloc(sizeof(struct timex), GFP_KERNEL);
+ if (!txc)
+ return -ENOMEM;

/* Copy the user data space into the kernel copy
* structure. But bear in mind that the structures
* may change
*/
- if(copy_from_user(&txc, txc_p, sizeof(struct timex)))
- return -EFAULT;
- ret = do_adjtimex(&txc);
- return copy_to_user(txc_p, &txc, sizeof(struct timex)) ? -EFAULT : ret;
+ if(copy_from_user(txc, txc_p, sizeof(struct timex))) {
+ retval = -EFAULT;
+ goto free_txc;
+ }
+ retval = do_adjtimex(txc);
+ if (copy_to_user(txc_p, txc, sizeof(struct timex)))
+ retval = -EFAULT;
+free_txc:
+ kfree(txc);
+ return retval;
}

inline struct timespec current_kernel_time(void)

2005-03-31 08:27:25

by Jeff Garzik

Subject: Re: [PATCH] Reduce stack usage in time.c

Yum Rayan wrote:
> Attempt to reduce stack usage in time.c (linux-2.6.12-rc1-mm3). Stack
> usage was noted using checkstack.pl. Specifically:
>
> Before patch
> ------------
> sys_adjtimex - 128
>
> After patch
> -----------
> sys_adjtimex - none (register usage only)
>
> Signed-off-by: Yum Rayan <[email protected]>
>
> --- a/kernel/time.c 2005-03-25 22:11:06.000000000 -0800
> +++ b/kernel/time.c 2005-03-30 16:59:51.000000000 -0800
> @@ -413,17 +413,27 @@
>
> asmlinkage long sys_adjtimex(struct timex __user *txc_p)
> {
> - struct timex txc; /* Local copy of parameter */
> - int ret;
> + struct timex *txc; /* Local copy of parameter */
> + int retval;
> +
> + txc = kmalloc(sizeof(struct timex), GFP_KERNEL);
> + if (!txc)
> + return -ENOMEM;
>
> /* Copy the user data space into the kernel copy
> * structure. But bear in mind that the structures
> * may change
> */
> - if(copy_from_user(&txc, txc_p, sizeof(struct timex)))
> - return -EFAULT;
> - ret = do_adjtimex(&txc);
> - return copy_to_user(txc_p, &txc, sizeof(struct timex)) ? -EFAULT : ret;
> + if(copy_from_user(txc, txc_p, sizeof(struct timex))) {
> + retval = -EFAULT;
> + goto free_txc;
> + }
> + retval = do_adjtimex(txc);
> + if (copy_to_user(txc_p, txc, sizeof(struct timex)))
> + retval = -EFAULT;
> +free_txc:

It seems quite unhealthy to add a kmalloc(,GFP_KERNEL) allocation into a
time-sensitive function.

Jeff

2005-03-31 09:33:04

by Denis Vlasenko

Subject: Re: [PATCH] Reduce stack usage in time.c

On Thursday 31 March 2005 11:26, Jeff Garzik wrote:
> Yum Rayan wrote:
> > Attempt to reduce stack usage in time.c (linux-2.6.12-rc1-mm3). Stack
> > usage was noted using checkstack.pl. Specifically:
> >
> > Before patch
> > ------------
> > sys_adjtimex - 128
> >
> > After patch
> > -----------
> > sys_adjtimex - none (register usage only)
> >
> > Signed-off-by: Yum Rayan <[email protected]>
> >
> > --- a/kernel/time.c 2005-03-25 22:11:06.000000000 -0800
> > +++ b/kernel/time.c 2005-03-30 16:59:51.000000000 -0800
> > @@ -413,17 +413,27 @@
> >
> > asmlinkage long sys_adjtimex(struct timex __user *txc_p)
> > {
> > - struct timex txc; /* Local copy of parameter */
> > - int ret;
> > + struct timex *txc; /* Local copy of parameter */
> > + int retval;
> > +
> > + txc = kmalloc(sizeof(struct timex), GFP_KERNEL);
> > + if (!txc)
> > + return -ENOMEM;
> >
> > /* Copy the user data space into the kernel copy
> > * structure. But bear in mind that the structures
> > * may change
> > */
> > - if(copy_from_user(&txc, txc_p, sizeof(struct timex)))
> > - return -EFAULT;
> > - ret = do_adjtimex(&txc);
> > - return copy_to_user(txc_p, &txc, sizeof(struct timex)) ? -EFAULT : ret;
> > + if(copy_from_user(txc, txc_p, sizeof(struct timex))) {
> > + retval = -EFAULT;
> > + goto free_txc;
> > + }
> > + retval = do_adjtimex(txc);
> > + if (copy_to_user(txc_p, txc, sizeof(struct timex)))
> > + retval = -EFAULT;
> > +free_txc:

Is this a syscall?
Is it ever called from some deeply nested kernel function?
--
vda

2005-03-31 13:00:57

by Jörn Engel

Subject: Re: [PATCH] Reduce stack usage in time.c

On Thu, 31 March 2005 12:26:58 +0300, Denis Vlasenko wrote:
>
> Is this a syscall?
> Is it ever called from some deeply nested kernel function?

Never showed up in any of my callchain-tests. I'd leave it as is.

J?rn

--
When I am working on a problem I never think about beauty. I think
only how to solve the problem. But when I have finished, if the
solution is not beautiful, I know it is wrong.
-- R. Buckminster Fuller