Add __divmoddi4 and __udivmoddi4 for 32-bit arch #636
Conversation
gcc-7 seems to use __udivmoddi4 for 64-bit division on 32-bit arch. This patch implement them so we don't get undefined reference error. Signed-off-by: Chunwei Chen <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]> Requires-spl:refs/pull/636/head
There was a problem hiding this comment.
Thanks! This looks right to my eye but we should run the splat generic:Unsigned Div-64 Test and generic:Signed Div-64 Test test cases in this environment. While there were written for __divdi3 to verify the math is correct, they should work for __udivmoddi4 too.
|
I don't have a 32bit with gcc-7 VM around, so I haven't really tested it yet. |
Me either. I was secretly hoping you'd already set one up. Let me see about spinning one up, I suppose a 32-bit Fedora 26 install is the easiest way to go. |
|
I've reproduced openzfs/zfs#6417 on archlinux a couple of days ago, will test this fix in a bit. EDIT: i also tried to reproduce on a i686 debian (unstable) box with gcc7 without success, i wonder if this has anything to do with the gcc7 version: Archlinux: Maybe even i386 fedora could have the same problem reproducing? |
|
So far so good, got both SPL and ZFS compiled and loaded correctly: Tests: |
|
Looks good, thanks for doing this testing and verifying the test case was using the new functions. If you still have the test environment setup the only additional thing I'd suggest would be to run the ZFS Test Suite to verify basic functionality. |
|
@behlendorf this is going to take a little more time, my storage is severely underpowered to handle the IO needed to run the full ZTS: i'll report back as soon as it finishes. |
|
@loli10K from a practical point of view we should really only need to run a handful of tests from the test suite to verify this is correct (along with the splat test case). Division is used pretty extensively in the ZFS source so if it was fundamentally broken I'd expect things to fall apart almost immediately. |
|
@behlendorf it seems noinline raidz_map_t *
vdev_raidz_map_alloc(zio_t *zio, uint64_t ashift, uint64_t dcols,
uint64_t nparity)
{
raidz_map_t *rm;
/* The starting RAIDZ (parent) vdev sector of the block. */
uint64_t b = zio->io_offset >> ashift;
/* The zio's size in units of the vdev's minimum sector size. */
uint64_t s = zio->io_size >> ashift;
/* The first column for this stripe. */
uint64_t f = b % dcols;
/* The starting byte offset on each child vdev. */
uint64_t o = (b / dcols) << ashift; /* <=== __udivmoddi4() */
uint64_t q, r, c, bc, col, acols, scols, coff, devidx, asize, tot;
uint64_t off = 0;
/*
* "Quotient": The number of data sectors for this stripe on all but
* the "big column" child vdevs that also contain "remainder" data.
*/
q = s / (dcols - nparity); /* <=== __udivmoddi4() */The ZTS is running smoothly so far: |
|
That's interesting. Perhaps the other cases still end up using |
|
@loli10K how does the testing look? |
|
@behlendorf i'll be back in ~1h to check, i left it running at home this morning. |
|
You probably won't believe it because i don't do it myself, but the test suite is still running, output here: https://gist.github.com/loli10K/4ad76bd8f5c4f4cf943d953a6ec90fa1. Most of these failures must be unrelated, it's just my home lab being slow. I'm sorry i've kept you waiting, LGTM. |
|
No problem! Thanks for volunteering a system to put this patch through its paces. The test results are about what I'd expect so LGTM. I'll get it merged right away. |
gcc-7 seems to use __udivmoddi4 for 64-bit division on 32-bit arch. This patch implement them so we don't get undefined reference error. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: loli10K <[email protected]> Signed-off-by: Chunwei Chen <[email protected]> Closes openzfs/zfs#6417 Closes #636
gcc-7 seems to use __udivmoddi4 for 64-bit division on 32-bit arch. This patch implement them so we don't get undefined reference error. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: loli10K <[email protected]> Signed-off-by: Chunwei Chen <[email protected]> Closes openzfs/zfs#6417 Closes openzfs#636
gcc-7 seems to use __udivmoddi4 for 64-bit division on 32-bit arch. This
patch implement them so we don't get undefined reference error.
Signed-off-by: Chunwei Chen [email protected]