On Saturday, 4 August 2007 14:35, Alon Bar-Lev wrote:
> On Saturday 04 August 2007, Alon Bar-Lev wrote:
> >
> > Hello Rafael,
> >
> > This is *UNTESTED* suggestion for switching into lzo without the memcpy.
> > Please see if I missed something as I the code combination of global
> > variables,
> > local memory pool and unrelated sizes is difficult to understand/maintain.
> >
> > Basically, I've modify the init_swap_writer() so that it is the only
> > function
> > knowing the size of the swap_map_handle needs. When you call it with
> > NULL you get the number of bytes it needs.
> >
> > I also don't fully understand why the encryption and compression/write
> > blocks
> > are related in size.... And why you remove the encryption block from
> > mem_size.
> >
> > Alon.
>
> OK.
> I did got this wrong.
> Here is another attempt.
>
> Apart of making init_swap_writer(), init_swap_reader() be responsible of the
> memory
> size required, I've moved buffer_size, max_block_size from global scope to
> the handler structure
> as it is related and not used without it.
Actually, buffer_size determines the size of the memory pool allocated in the
beginning, so it should be global. max_block_size may be moved to 'strcut
swap_map_handle', but this can be done in a separate patch.
> Also I think that:
> buffer_size = BUFFER_PAGES * page_size;
> Should be:
> buffer_size = BUFFER_PAGES * max_block_size;
>
> As compression adds its header...
No.
max_block_size is only determined when we know that compression will be
used and buffer_size is needed earlier (it may be changed, but in a separate
patch, too).
> But I still don't understand the encryption stuff...
> It seems that key_data points to the same location
> as handle->encrypt_buffer.
That's correct, but key_data is no longer needed when handle->encrypt_buffer
is used.
> Did I get this wrong?
> Why mem_size is decremented?
In order to set key_data to the desired value without explicit computations.
Nothing essential (mem_size is a local variable, BTW). Moreover, key_data
might be set to another value just as well, as long as the total size of memory
allocated to it is sufficient.
> For now, I allocated it its own space.
???
> There is a big fact that I am missing... why use only one mem_pool?
Because of my personal preference.
> You can malloc as much as you wish at initialization directly into
> the pointers... I just don't understand what may go wrong if you do this...
Nothing, really. I prefer to have two global pointers and one contiguous
memory area than to have five global pointers and five different memory
areas.
The problem is, we need to allocate all memory during the initialization,
because allocating memory later on may easily fail. Now, it doesn't matter
too much if we take it in one piece or in several pieces.
> ---
>
> diff -urNp suspend.org/Makefile suspend.compress1/Makefile
> --- suspend.org/Makefile 2007-07-29 21:30:27.000000000 +0300
> +++ suspend.compress1/Makefile 2007-08-04 14:47:14.000000000 +0300
> @@ -41,7 +41,7 @@ endif
>
> ifdef CONFIG_COMPRESS
> CC_FLAGS += -DCONFIG_COMPRESS
> -SWSUSP_LD_FLAGS += -llzf
> +SWSUSP_LD_FLAGS += -llzo2
> endif
>
> ifdef CONFIG_ENCRYPT
> diff -urNp suspend.org/resume.c suspend.compress1/resume.c
> --- suspend.org/resume.c 2007-05-13 20:53:13.000000000 +0300
> +++ suspend.compress1/resume.c 2007-08-04 14:47:51.000000000 +0300
> @@ -26,9 +26,7 @@
> #include <string.h>
> #include <errno.h>
> #ifdef CONFIG_COMPRESS
> -#include <lzf.h>
> -#else
> -#define lzf_decompress(a, b, c, d) 0
> +#include <lzo/lzo1x.h>
> #endif
>
> #include "swsusp.h"
> @@ -133,7 +131,7 @@ static struct config_par parameters[PARA
> };
>
> static unsigned int page_size;
> -static unsigned int buffer_size;
> +static unsigned int workpool_size;
> static void *mem_pool;
>
> /**
> @@ -178,6 +176,7 @@ struct swap_map_handle {
> unsigned int k;
> int fd;
> struct md5_ctx ctx;
> + unsigned int buffer_size;
> #ifdef CONFIG_ENCRYPT
> char *decrypt_buffer;
> gcry_cipher_hd_t cipher_handle;
> @@ -196,7 +195,7 @@ static int fill_buffer(struct swap_map_h
> void *dst = handle->read_buffer;
> int error;
>
> - int read_ahead_areas = READ_AHEAD_SIZE/buffer_size;
> + int read_ahead_areas = READ_AHEAD_SIZE/handle->buffer_size;
> int advise_first, advise_last, i;
>
> if (!handle->k) {
> @@ -223,7 +222,7 @@ static int fill_buffer(struct swap_map_h
> }
>
> handle->area_size = handle->areas[handle->k].size;
> - if (handle->area_size > buffer_size)
> + if (handle->area_size > handle->buffer_size)
> return -ENOMEM;
> #ifdef CONFIG_ENCRYPT
> if (decrypt)
> @@ -243,19 +242,48 @@ static int fill_buffer(struct swap_map_h
>
> static int init_swap_reader(struct swap_map_handle *handle, int fd, loff_t
> start, void *buf)
> {
> + unsigned int buffer_size;
> + unsigned int max_block_size;
> + char *p = (char *)buf;
> int error;
>
> - if (!start || !buf)
> + if (handle != NULL && buf == NULL)
> return -EINVAL;
> - handle->areas = buf;
> - handle->areas_per_page = (page_size - sizeof(loff_t)) /
> - sizeof(struct swap_area);
> - handle->next_swap = (loff_t *)(handle->areas + handle->areas_per_page);
> - handle->page_buffer = (char *)buf + page_size;
> - handle->read_buffer = handle->page_buffer + page_size;
> +
> + /* This MUST NOT be less than page_size */
> + max_block_size = page_size;
> +#ifdef CONFIG_COMPRESS
> + max_block_size += sizeof(short);
> +#endif
> + buffer_size = BUFFER_PAGES * max_block_size;
> +
> + if (handle != NULL) {
> + handle->areas = (struct swap_area *)p;
> + handle->areas_per_page = (page_size - sizeof(loff_t)) /
> + sizeof(struct swap_area);
> + handle->next_swap = (loff_t *)(handle->areas +
> handle->areas_per_page);
> + }
> + p += page_size;
> + if (handle != NULL)
> + handle->page_buffer = p;
> + p += page_size;
> + if (handle != NULL)
> + handle->read_buffer = p;
> + p += buffer_size;
> #ifdef CONFIG_ENCRYPT
> - handle->decrypt_buffer = handle->read_buffer + buffer_size;
> + if (handle != NULL)
> + handle->decrypt_buffer = p;
> + p += buffer_size;
> #endif
> +
> + if (handle == NULL)
> + return p-(char *)buf;
> +
> + if (!start)
> + return -EINVAL;
> +
> + handle->buffer_size = buffer_size;
> +
Please don't touch init_swap_reader() if you don't have to.
> error = read_area(fd, handle->areas, start, page_size);
> if (error)
> return error;
> @@ -276,21 +304,28 @@ static int restore(struct swap_map_handl
> void *buf = handle->page_buffer;
>
> block = (struct buf_block *)(handle->read_buffer + disp);
> +#ifdef CONFIG_COMPRESS
> if (decompress) {
> unsigned short cnt = block->size;
>
> if (cnt == page_size) {
> memcpy(buf, block->data, page_size);
> } else if (cnt < page_size) {
> - cnt = lzf_decompress(block->data, cnt, buf, page_size);
> - if (cnt != page_size)
> + lzo_uint result_len = page_size;
> +
> + if (
> + lzo1x_decompress_safe((lzo_bytep)block->data,
> cnt, buf, &result_len, NULL) != LZO_E_OK ||
> + result_len != page_size
> + ) {
> return -ENODATA;
> + }
> } else {
> return -EINVAL;
> }
> block->size += sizeof(short);
> return block->size;
> }
> +#endif
> memcpy(buf, block, page_size);
> return page_size;
> }
> @@ -566,7 +601,7 @@ static int read_image(int dev, int fd, s
> static unsigned char orig_checksum[16], checksum[16];
> int ret, error = 0;
> struct swsusp_info *header = mem_pool;
> - char *buffer = (char *)mem_pool + page_size;
> + char *buffer = (char *)mem_pool + workpool_size;
> unsigned int nr_pages;
> unsigned int size = sizeof(struct swsusp_header);
> unsigned int shift = (resume_offset + 1) * page_size - size;
> @@ -589,6 +624,10 @@ static int read_image(int dev, int fd, s
> printf("resume: Compressed image\n");
> #ifdef CONFIG_COMPRESS
> decompress = 1;
> + if (lzo_init() != LZO_E_OK) {
> + fprintf(stderr, "suspend: Could not initialize
> compress\n");
> + error = -ENOPKG;
> + }
> #else
> fprintf(stderr,"resume: Compression not supported\n");
> error = -EINVAL;
> @@ -818,13 +857,12 @@ int main(int argc, char *argv[])
> splash_param = SPL_RESUME;
>
> page_size = getpagesize();
> - buffer_size = BUFFER_PAGES * page_size;
> + workpool_size = page_size;
> +
> + mem_size = workpool_size + init_swap_reader (NULL, 0, 0, NULL);
> #ifdef CONFIG_ENCRYPT
> printf("resume: libgcrypt version: %s\n", gcry_check_version(NULL));
> gcry_control(GCRYCTL_INIT_SECMEM, page_size, 0);
> - mem_size = 3 * page_size + 2 * buffer_size;
> -#else
> - mem_size = 3 * page_size + buffer_size;
> #endif
> mem_pool = malloc(mem_size);
> if (!mem_pool) {
> diff -urNp suspend.org/suspend.c suspend.compress1/suspend.c
> --- suspend.org/suspend.c 2007-07-29 21:30:27.000000000 +0300
> +++ suspend.compress1/suspend.c 2007-08-04 14:47:04.000000000 +0300
> @@ -33,9 +33,7 @@
> #include <signal.h>
> #include <termios.h>
> #ifdef CONFIG_COMPRESS
> -#include <lzf.h>
> -#else
> -#define lzf_compress(a, b, c, d) 0
> +#include <lzo/lzo1x.h>
> #endif
>
> #include "swsusp.h"
> @@ -61,10 +59,11 @@ static int suspend_loglevel = SUSPEND_LO
> static char compute_checksum;
> #ifdef CONFIG_COMPRESS
> static char compress;
> +static lzo_byte compress_work_buffer[LZO1X_1_MEM_COMPRESS];
> #else
> #define compress 0
> #endif
> -static unsigned long compr_diff;
> +static unsigned long compr_diff = 0;
> #ifdef CONFIG_ENCRYPT
> static char encrypt;
> static char use_RSA;
> @@ -170,9 +169,8 @@ static struct config_par parameters[PARA
> };
>
> static unsigned int page_size;
> -/* This MUST NOT be less than page_size */
> -static unsigned int max_block_size;
> -static unsigned int buffer_size;
> +/* This is for misc operations */
> +static unsigned int workpool_size;
> static void *mem_pool;
> #ifdef CONFIG_ENCRYPT
> struct key_data *key_data;
> @@ -263,26 +261,68 @@ struct swap_map_handle {
> unsigned int k;
> int dev, fd;
> struct md5_ctx ctx;
> + unsigned int buffer_size;
> + unsigned int max_block_size;
> #ifdef CONFIG_ENCRYPT
> unsigned char *encrypt_buffer;
> #endif
> };
>
> +/*
> + * If handle and buf are NULL, returns required memory size
> + */
> static int
> init_swap_writer(struct swap_map_handle *handle, int dev, int fd, void *buf)
> {
> - if (!buf)
> + unsigned int buffer_size;
> + unsigned int max_block_size;
> + char *p = (char *)buf;
> +
> + if (handle != NULL && buf == NULL)
> return -EINVAL;
> - handle->areas = buf;
> - handle->areas_per_page = (page_size - sizeof(loff_t)) /
> - sizeof(struct swap_area);
> - handle->next_swap = (loff_t *)(handle->areas + handle->areas_per_page);
> - handle->page_buffer = (char *)buf + page_size;
> - handle->write_buffer = handle->page_buffer + page_size;
> +
> + /* This MUST NOT be less than page_size */
> + max_block_size = page_size;
> +#ifdef CONFIG_COMPRESS
> + max_block_size += sizeof(short);
> +#endif
> +
> + buffer_size = BUFFER_PAGES * max_block_size;
> +
> + if (handle != NULL) {
> + handle->areas = (struct swap_area *)p;
> + handle->areas_per_page = (page_size - sizeof(loff_t)) /
> + sizeof(struct swap_area);
> + handle->next_swap = (loff_t *)(handle->areas +
> handle->areas_per_page);
> + }
> + p += page_size;
> + if (handle != NULL)
> + handle->page_buffer = p;
> + p += page_size;
> + if (handle != NULL)
> + handle->write_buffer = p;
> + p += buffer_size;
> +#ifdef CONFIG_COMPRESS
> + /*
> + * This leaves room for safeguard of lzo
> + * uncompressable block
> + * operations, as we use block size
> + * of page_size, we should have this
> + * amount at the end of the buffer.
> + */
> + p += page_size/16+64+3;
> +#endif
> #ifdef CONFIG_ENCRYPT
> - handle->encrypt_buffer = (unsigned char *)(handle->write_buffer +
> - buffer_size);
> + if (handle != NULL)
> + handle->encrypt_buffer = (unsigned char *)p;
> + p += buffer_size;
> #endif
> + if (handle == NULL)
> + return p-(char *)buf;
> +
> + handle->buffer_size = buffer_size;
> + handle->max_block_size = max_block_size;
> +
> memset(handle->areas, 0, page_size);
> handle->cur_swap = get_swap_page(dev);
> if (!handle->cur_swap)
Ditto.
> @@ -306,12 +346,15 @@ static int prepare(struct swap_map_handl
> void *buf = handle->page_buffer;
>
> block = (struct buf_block *)(handle->write_buffer + disp);
> +#ifdef CONFIG_COMPRESS
> if (compress) {
> - unsigned short cnt;
> + lzo_uint cnt = 0;
>
> - cnt = lzf_compress(buf, page_size,
> - block->data, page_size - sizeof(short));
> - if (!cnt) {
> + /* NOTICE: Assuming block->data large enough as lzo does not
> check for limits */
We can't make this assumption in general.
> + if (
> + lzo1x_1_compress(buf, page_size,
> (lzo_bytep)block->data, &cnt, compress_work_buffer) != LZO_E_OK ||
> + cnt >= page_size - sizeof(short)
> + ) {
> memcpy(block->data, buf, page_size);
> cnt = page_size;
> } else {
> @@ -322,6 +365,7 @@ static int prepare(struct swap_map_handl
> cnt += sizeof(short);
> return cnt;
> }
> +#endif
> memcpy(block, buf, page_size);
> return page_size;
> }
> @@ -381,8 +425,8 @@ static int swap_write_page(struct swap_m
> return try_get_more_swap(handle);
> }
>
> - if (handle->cur_alloc + max_block_size <= buffer_size) {
> - if (handle->cur_area.size + max_block_size <=
> handle->cur_alloc) {
> + if (handle->cur_alloc + handle->max_block_size <= handle->buffer_size) {
> + if (handle->cur_area.size + handle->max_block_size <=
> handle->cur_alloc) {
> handle->cur_area.size += prepare(handle,
> handle->cur_area.size);
> return 0;
> }
> @@ -596,17 +640,15 @@ int write_image(int snapshot_fd, int res
> return -ENOSPC;
> }
> error = init_swap_writer(&handle, snapshot_fd, resume_fd,
> - (char *)mem_pool + page_size);
> + (char *)mem_pool + workpool_size);
> if (!error) {
> header->map_start = handle.cur_swap;
> header->image_flags = 0;
> - max_block_size = page_size;
> if (compute_checksum)
> header->image_flags |= IMAGE_CHECKSUM;
>
> if (compress) {
> header->image_flags |= IMAGE_COMPRESSED;
> - max_block_size += sizeof(short);
> }
>
> #ifdef CONFIG_ENCRYPT
> @@ -1294,6 +1336,9 @@ int main(int argc, char *argv[])
> int orig_loglevel, orig_swappiness, ret;
> struct rlimit rlim;
> static char chroot_path[MAX_STR_LEN];
> +#ifdef CONFIG_ENCRYPT
> + int encrypt_buffer_size;
> +#endif
>
> /* Make sure the 0, 1, 2 descriptors are open before opening the
> * snapshot and resume devices
> @@ -1346,13 +1391,24 @@ int main(int argc, char *argv[])
> shutdown_method = SHUTDOWN_METHOD_REBOOT;
> }
>
> +#ifdef CONFIG_COMPRESS
> + if (compress) {
> + if (lzo_init() != LZO_E_OK) {
> + fprintf(stderr, "suspend: Could not initialize
> compress\n");
> + return ENOPKG;
> + }
> + }
> +#endif
> +
> page_size = getpagesize();
> - buffer_size = BUFFER_PAGES * page_size;
> + workpool_size = page_size;
>
> - mem_size = 3 * page_size + buffer_size;
> + mem_size = workpool_size + init_swap_writer (NULL, 0, 0, NULL);
> #ifdef CONFIG_ENCRYPT
> + /* Am I missing something, or this can be sizeof (struct key_data)? */
> + encrypt_buffer_size = BUFFER_PAGES * page_size;
> if (encrypt)
> - mem_size += buffer_size;
> + mem_size += encrypt_buffer_size;
> #endif
> mem_pool = malloc(mem_size);
> if (!mem_pool) {
> @@ -1374,7 +1430,7 @@ int main(int argc, char *argv[])
> }
> }
> if (encrypt) {
> - mem_size -= buffer_size;
> + mem_size -= encrypt_buffer_size;
> key_data = (struct key_data *)((char *)mem_pool + mem_size);
> generate_key();
> }
>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
Suspend-devel mailing list
Suspend-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/suspend-devel
