Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290761 > unrolled thread

Re: [PATCH 1/2] zram: Less checks in zram_bvec_write() after error detection

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] zram: Less checks in zram_bvec_write() after error  detection Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-12-14 01:30 +0100
    Re: [PATCH 1/2] zram: Less checks in zram_bvec_write() after error  detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-14 08:00 +0100
      Re: [PATCH 1/2] zram: Less checks in zram_bvec_write() after error  detection Julia Lawall <julia.lawall@lip6.fr> - 2015-12-14 08:20 +0100
      Re: [PATCH 1/2] zram: Less checks in zram_bvec_write() after error  detection Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-12-14 11:10 +0100
        Re: [PATCH 1/2] zram: Less checks in zram_bvec_write() after error  detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-14 15:10 +0100

#1290761 — Re: [PATCH 1/2] zram: Less checks in zram_bvec_write() after error detection

On (12/11/15 19:24), SF Markus Elfring wrote:
[..]
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 47915d7..69d7fcd 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -652,9 +652,9 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	size_t clen;
>  	unsigned long handle;
>  	struct page *page;
> -	unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
> +	unsigned char *user_mem, *cmem, *src, *uncmem;
>  	struct zram_meta *meta = zram->meta;
> -	struct zcomp_strm *zstrm = NULL;
> +	struct zcomp_strm *zstrm;
>  	unsigned long alloced_pages;
>  
>  	page = bvec->bv_page;
> @@ -664,13 +664,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		 * before to write the changes.
>  		 */
>  		uncmem = kmalloc(PAGE_SIZE, GFP_NOIO);
> -		if (!uncmem) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> +		if (!uncmem)
> +			return -ENOMEM;

ok.

>  		ret = zram_decompress_page(zram, uncmem, index);
>  		if (ret)
> -			goto out;
> +			goto free_uncmem;

here and later, I don't want to split `out' label.
you still need to do both 'if zstrm' and 'if is_partial_io' checks anyway, what's the gain?
the more labels we have the trickier it may get.

>  	}
>  
>  	zstrm = zcomp_strm_find(zram->comp);
> @@ -696,7 +694,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  
>  		atomic64_inc(&zram->stats.zero_pages);
>  		ret = 0;
> -		goto out;
> +		goto check_strm;
>  	}
>  
>  	ret = zcomp_compress(zram->comp, zstrm, uncmem, &clen);
> @@ -708,7 +706,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  
>  	if (unlikely(ret)) {
>  		pr_err("Compression failed! err=%d\n", ret);
> -		goto out;
> +		goto check_strm;
>  	}
>  	src = zstrm->buffer;
>  	if (unlikely(clen > max_zpage_size)) {
> @@ -722,7 +720,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		pr_err("Error allocating memory for compressed page: %u, size=%zu\n",
>  			index, clen);
>  		ret = -ENOMEM;
> -		goto out;
> +		goto check_strm;
>  	}
>  
>  	alloced_pages = zs_get_total_pages(meta->mem_pool);
> @@ -731,7 +729,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	if (zram->limit_pages && alloced_pages > zram->limit_pages) {
>  		zs_free(meta->mem_pool, handle);
>  		ret = -ENOMEM;
> -		goto out;
> +		goto check_strm;
>  	}
>  
>  	cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_WO);
> @@ -762,11 +760,13 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	/* Update stats */
>  	atomic64_add(clen, &zram->stats.compr_data_size);
>  	atomic64_inc(&zram->stats.pages_stored);
> -out:
> +check_strm:
>  	if (zstrm)
>  		zcomp_strm_release(zram->comp, zstrm);
> -	if (is_partial_io(bvec))
> +	if (is_partial_io(bvec)) {
> +free_uncmem:
>  		kfree(uncmem);
> +	}

a label inside of `if'?   no.
keep it the way it is please.

>  	return ret;
>  }

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1290892

>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>> index 47915d7..69d7fcd 100644
>> --- a/drivers/block/zram/zram_drv.c
>> +++ b/drivers/block/zram/zram_drv.c
>> @@ -652,9 +652,9 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>  	size_t clen;
>>  	unsigned long handle;
>>  	struct page *page;
>> -	unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
>> +	unsigned char *user_mem, *cmem, *src, *uncmem;
>>  	struct zram_meta *meta = zram->meta;
>> -	struct zcomp_strm *zstrm = NULL;
>> +	struct zcomp_strm *zstrm;
>>  	unsigned long alloced_pages;
>>  
>>  	page = bvec->bv_page;
>> @@ -664,13 +664,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>  		 * before to write the changes.
>>  		 */
>>  		uncmem = kmalloc(PAGE_SIZE, GFP_NOIO);
>> -		if (!uncmem) {
>> -			ret = -ENOMEM;
>> -			goto out;
>> -		}
>> +		if (!uncmem)
>> +			return -ENOMEM;
> 
> ok.

Thanks for your terse acknowledgement.


>>  		ret = zram_decompress_page(zram, uncmem, index);
>>  		if (ret)
>> -			goto out;
>> +			goto free_uncmem;
> 
> here and later, I don't want to split `out' label.

I guess that corresponding software design concerns can evolve a bit.


> you still need to do both 'if zstrm' and 'if is_partial_io' checks anyway, what's the gain?

How are the chances to reduce the number of dispensable sanity checks?


> the more labels we have the trickier it may get.

I hope that more unique jump labels can make the involved exception handling also clearer.


>> @@ -762,11 +760,13 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>  	/* Update stats */
>>  	atomic64_add(clen, &zram->stats.compr_data_size);
>>  	atomic64_inc(&zram->stats.pages_stored);
>> -out:
>> +check_strm:
>>  	if (zstrm)
>>  		zcomp_strm_release(zram->comp, zstrm);
>> -	if (is_partial_io(bvec))
>> +	if (is_partial_io(bvec)) {
>> +free_uncmem:
>>  		kfree(uncmem);
>> +	}
> 
> a label inside of `if'?   no.

Do any more software developers find such an use case interesting?


> keep it the way it is please.

I suggest to make the affected exception handling a bit more efficient.
Such source code fine-tuning has got a few special consequences.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290897

> I suggest to make the affected exception handling a bit more efficient.
> Such source code fine-tuning has got a few special consequences.

Exception handling is by definition exceptional, and thus its efficiency 
is rarely important.  What is important is that it should be correct, and 
ideally clearly correct, so that someone can check its correctness easily.  
Optimizations, if they have any effect at all, typically make the 
correctness less obvious.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291033

On (12/14/15 07:58), SF Markus Elfring wrote:
[..]
> > keep it the way it is please.
> 
> I suggest to make the affected exception handling a bit more efficient.
> Such source code fine-tuning has got a few special consequences.

by 'more efficient' you mean saving cpu cycles on 'bvec->bv_len != PAGE_SIZE'
comparison in exception/error path?

...
check_strm:
	if (zstrm)
		zcomp_strm_release(zram->comp, zstrm);
	if (is_partial_io(bvec)) {
free_uncmem:
		kfree(uncmem);
	}
...


no.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291213

>> I suggest to make the affected exception handling a bit more efficient.
>> Such source code fine-tuning has got a few special consequences.
> 
> by 'more efficient' you mean saving cpu cycles on 'bvec->bv_len != PAGE_SIZE'
> comparison in exception/error path?

Yes …


> ...
> check_strm:
> 	if (zstrm)
> 		zcomp_strm_release(zram->comp, zstrm);
> 	if (is_partial_io(bvec)) {
> free_uncmem:
> 		kfree(uncmem);
> 	}
> ...

I propose to jump over two sanity checks.


> no.

Thanks for your feedback.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web