linux-kernel - Re: [PATCH v5]: Improve performance of LZO hibernation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 28 Sep 2011 10:22:27 +0300
From:	Pekka Enberg <penberg@...helsinki.fi>
To:	Bojan Smojver <bojan@...ursive.com>
Cc:	linux-kernel@...r.kernel.org, "Rafael J. Wysocki" <rjw@...k.pl>
Subject: Re: [PATCH v5]: Improve performance of LZO hibernation

On Wed, Sep 28, 2011 at 7:20 AM, Bojan Smojver <bojan@...ursive.com> wrote:
> +static int lzo_compress_threadfn(void *data)
> +{
> +       struct cmp_data *d = data;
> +
> +       while(1) {
> +               wait_event(d->go, atomic_read(&d->ready) ||
> +                                 kthread_should_stop());
> +               if (kthread_should_stop())
> +                       break;

So... what happens to the hibernation process when 'kthread_should_stop()'
returns true?

> +               atomic_set(&d->ready, 0);
> +
> +               d->ret = lzo1x_1_compress(d->unc, d->unc_len,
> +                                         d->cmp + LZO_HEADER, &d->cmp_len,
> +                                         d->wrk);
> +               atomic_set(&d->stop, 1);
> +               wake_up(&d->done);
> +       }
> +
> +       return 0;
> +}
>
>  /**
>  * save_image_lzo - Save the suspend image data compressed with LZO.
> @@ -434,45 +494,74 @@ static int save_image_lzo(struct swap_map_handle *handle,
>        int ret = 0;
>        int nr_pages;
>        int err2;
> +       int cpu;
>        struct bio *bio;
>        struct timeval start;
>        struct timeval stop;
> -       size_t off, unc_len, cmp_len;
> -       unsigned char *unc, *cmp, *wrk, *page;
> +       size_t off, thr, cthr, nthr;
> +       unsigned char *page = NULL;
> +       struct cmp_data *data = NULL;
> +
> +       /*
> +        * We'll limit the number of threads for compression to limit memory
> +        * footprint.
> +        */
> +       nthr = num_online_cpus() - 1;
> +       nthr = nthr > LZO_THREADS ? LZO_THREADS : (nthr < 1 ? 1 : nthr);

That's probably one of the most unreadable uses of the ternary
operator I've ever seen!

What's going on here anyway? Why "num_online_cpus() - 1"? What's wrong with

  nr_threads = num_online_cpus();
  if (nr_threads > LZO_THREADS)
    nr_threads = LZO_THREADS;

[ And yes, please use less cryptic variable names. ]

Overall, I really like your patch!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/