All of lore.kernel.org
 help / color / mirror / Atom feed
From: Adam Borowski <kilobyte@angband.pl>
To: dsterba@suse.cz, Nick Terrell <terrelln@fb.com>,
	kernel-team@fb.com, Chris Mason <clm@fb.com>,
	Yann Collet <cyan@fb.com>,
	squashfs-devel@lists.sourceforge.net,
	linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Adam Borowski <kilobyte@angband.pl>
Subject: [PATCH] lib/zstd: use div_u64() to let it build on 32-bit
Date: Tue, 27 Jun 2017 06:18:23 +0200	[thread overview]
Message-ID: <20170627041823.1300-1-kilobyte@angband.pl> (raw)
In-Reply-To: <20170626121230.GF2866@twin.jikos.cz>

David Sterba wrote:
> > Thus, you want do_div() instead of /; do check widths and signedness of
> > arguments.
>
> No do_div please, div_u64 or div64_u64.

Good to know, the interface of do_div() is indeed weird.

I guess Nick has found and fixed the offending divisions in his tree
already, but this patch I'm sending is what I'm testing.

One thing to note is that it divides u64 by size_t, so the actual operation
differs on 32 vs 64-bit.  Yet the code fails to handle compressing pieces
bigger than 4GB in other places -- so use of size_t is misleading.  Perhaps
u32 would better convey this limitation?

Anyway, that this code didn't even compile on 32-bit also means it hasn't
been tested.  I just happen to have such an ARM machine doing Debian archive
rebuilds; I've rewritten the chroots with compress=zstd; this should be a
nice non-artificial test.  The load consists of snapshot+dpkg+gcc/etc+
assorted testsuites, two sbuild instances.  Seems to work fine for a whole
hour (yay!) already, let's see if there'll be any explosions.


-- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 ---- >8 --
Note that "total" is limited to 2³²-1 elsewhere despite being declared
as size_t, so it's ok to use 64/32 -- it's much faster on eg. x86-32
than 64/64.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 lib/zstd/fse_compress.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/zstd/fse_compress.c b/lib/zstd/fse_compress.c
index e016bb177833..f59f9ebfe9c0 100644
--- a/lib/zstd/fse_compress.c
+++ b/lib/zstd/fse_compress.c
@@ -49,6 +49,7 @@
 #include "fse.h"
 #include <linux/compiler.h>
 #include <linux/string.h> /* memcpy, memset */
+#include <linux/math64.h>
 
 /* **************************************************************
 *  Error Management
@@ -575,7 +576,7 @@ static size_t FSE_normalizeM2(short *norm, U32 tableLog, const unsigned *count,
 	{
 		U64 const vStepLog = 62 - tableLog;
 		U64 const mid = (1ULL << (vStepLog - 1)) - 1;
-		U64 const rStep = ((((U64)1 << vStepLog) * ToDistribute) + mid) / total; /* scale on remaining */
+		U64 const rStep = div_u64((((U64)1 << vStepLog) * ToDistribute) + mid, total); /* scale on remaining */
 		U64 tmpTotal = mid;
 		for (s = 0; s <= maxSymbolValue; s++) {
 			if (norm[s] == NOT_YET_ASSIGNED) {
@@ -609,7 +610,7 @@ size_t FSE_normalizeCount(short *normalizedCounter, unsigned tableLog, const uns
 	{
 		U32 const rtbTable[] = {0, 473195, 504333, 520860, 550000, 700000, 750000, 830000};
 		U64 const scale = 62 - tableLog;
-		U64 const step = ((U64)1 << 62) / total; /* <== here, one division ! */
+		U64 const step = div_u64((U64)1 << 62, total); /* <== here, one division ! */
 		U64 const vStep = 1ULL << (scale - 20);
 		int stillToDistribute = 1 << tableLog;
 		unsigned s;
-- 
2.13.1


  parent reply	other threads:[~2017-06-27  4:21 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-22 22:01 [PATCH 1/4] lib: Add xxhash module Nick Terrell
2017-06-22 22:01 ` [PATCH 2/4] lib: Add zstd modules Nick Terrell
2017-06-22 22:01 ` [PATCH 3/4] btrfs: Add zstd support Nick Terrell
2017-06-25 15:02   ` kbuild test robot
2017-06-25 19:03   ` kbuild test robot
2017-06-25 21:30     ` Adam Borowski
2017-06-26 12:12       ` David Sterba
2017-06-26 16:54         ` Nick Terrell
2017-06-26 16:54           ` Nick Terrell
2017-06-27  4:18         ` Adam Borowski [this message]
2017-06-27  5:27           ` [PATCH] lib/zstd: use div_u64() to let it build on 32-bit Nick Terrell
2017-06-27  5:27             ` Nick Terrell
2017-06-27 12:57             ` David Sterba
2017-06-27 12:57               ` David Sterba
2017-06-28  5:29               ` Nick Terrell
2017-06-28  5:29                 ` Nick Terrell
2017-06-29  1:02             ` Adam Borowski
2017-06-29  3:01               ` [PATCH] btrfs: Keep one more workspace around Nick Terrell
2017-06-29 13:59                 ` David Sterba
2017-06-22 22:01 ` [PATCH 4/4] squashfs: Add zstd support Nick Terrell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170627041823.1300-1-kilobyte@angband.pl \
    --to=kilobyte@angband.pl \
    --cc=clm@fb.com \
    --cc=cyan@fb.com \
    --cc=dsterba@suse.cz \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=squashfs-devel@lists.sourceforge.net \
    --cc=terrelln@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.