| Message ID | 476b5678-41b2-d2f8-1890-ba315354ebc0@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | read-cache: fix division by zero core-dump | expand |
On 9/27/2018 6:24 PM, Ramsay Jones wrote: > > commit 225df8a468 ("ieot: add Index Entry Offset Table (IEOT) > extension", 2018-09-26) added a 'DIV_ROUND_UP(entries, ieot_blocks) > expression, where ieot_blocks was set to zero for a single cpu > platform. This caused an SIGFPE and a core dump in practically > every test in the test-suite, until test t4056-diff-order.sh, which > then went into an infinite loop! > > Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> > --- > > Hi Ben, > > Could you please squash this into the relevant commits on your > 'bp/read-cache-parallel' branch. (The first hunk fixes a sparse > warning about using an integer as a NULL pointer). > Absolutely - thanks for the patch. I don't know how long it's been since I've been on a single core CPU - I'm sad for you. ;-) > Thanks! > > ATB, > Ramsay Jones > > read-cache.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/read-cache.c b/read-cache.c > index 6755d58877..40f096f70a 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -2141,7 +2141,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) > size_t extension_offset = 0; > #ifndef NO_PTHREADS > int nr_threads, cpus; > - struct index_entry_offset_table *ieot = 0; > + struct index_entry_offset_table *ieot = NULL; > #endif > > if (istate->initialized) > @@ -2771,7 +2771,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > if (ieot_blocks < 1) > ieot_blocks = 1; > cpus = online_cpus(); > - if (ieot_blocks > cpus - 1) > + if (cpus > 1 && ieot_blocks > cpus - 1) > ieot_blocks = cpus - 1; > } else { > ieot_blocks = nr; >
On 28/09/18 02:20, Ben Peart wrote: > > > On 9/27/2018 6:24 PM, Ramsay Jones wrote: >> >> commit 225df8a468 ("ieot: add Index Entry Offset Table (IEOT) >> extension", 2018-09-26) added a 'DIV_ROUND_UP(entries, ieot_blocks) >> expression, where ieot_blocks was set to zero for a single cpu >> platform. This caused an SIGFPE and a core dump in practically >> every test in the test-suite, until test t4056-diff-order.sh, which >> then went into an infinite loop! >> >> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> >> --- >> >> Hi Ben, >> >> Could you please squash this into the relevant commits on your >> 'bp/read-cache-parallel' branch. (The first hunk fixes a sparse >> warning about using an integer as a NULL pointer). >> > > Absolutely - thanks for the patch. > > I don't know how long it's been since I've been on a single core CPU - I'm sad for you. ;-) Heh, don't be - whilst I do still have a single cpu laptop about the place _somewhere_, I haven't booted it up in about 15 years! :-D I used to regularly test git (and other software) on my old 32-bit laptop (windows xp/Linux Mint 17.x, Core2 duo), but just lately I have taken to using a 32-bit VM on my current laptop (4th gen i5) instead. (The git test-suite would take approx 50 min to run on the 32-bit hardware, whereas it only takes 8 min on the VM!). I have configured the 32-bit VM with a single cpu, because when the VM was configured with two cpus the git test-suite would take longer to run (approx. 8 -> 10 min)! Taking more resources from the host, but increasing the running time, didn't seem like a good return. ;-) Also, this is not the first time some multi-threaded code in git has 'failed' by assuming more than one cpu, so ... ATB, Ramsay Jones
On 9/28/2018 11:31 AM, Ramsay Jones wrote: > Also, this is not the first time some multi-threaded code in git > has 'failed' by assuming more than one cpu, so ... > I wonder if this is a good time to create a GIT_TEST_CPU_COUNT variable so we can mock out single-processor environments instead of relying on old hardware or custom VMs. Thanks, -Stolee
diff --git a/read-cache.c b/read-cache.c index 6755d58877..40f096f70a 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2141,7 +2141,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist) size_t extension_offset = 0; #ifndef NO_PTHREADS int nr_threads, cpus; - struct index_entry_offset_table *ieot = 0; + struct index_entry_offset_table *ieot = NULL; #endif if (istate->initialized) @@ -2771,7 +2771,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, if (ieot_blocks < 1) ieot_blocks = 1; cpus = online_cpus(); - if (ieot_blocks > cpus - 1) + if (cpus > 1 && ieot_blocks > cpus - 1) ieot_blocks = cpus - 1; } else { ieot_blocks = nr;
commit 225df8a468 ("ieot: add Index Entry Offset Table (IEOT) extension", 2018-09-26) added a 'DIV_ROUND_UP(entries, ieot_blocks) expression, where ieot_blocks was set to zero for a single cpu platform. This caused an SIGFPE and a core dump in practically every test in the test-suite, until test t4056-diff-order.sh, which then went into an infinite loop! Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Hi Ben, Could you please squash this into the relevant commits on your 'bp/read-cache-parallel' branch. (The first hunk fixes a sparse warning about using an integer as a NULL pointer). Thanks! ATB, Ramsay Jones read-cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)