Florian, DJ,
I have tested DJ's aligned chunk scanning code in Fedora Rawhide by doing a full toolchain bootstrap and looking for problems. I found no issues and didn't see any regressions in either glibc or gcc's testsuites.
I am fairly confident that the code works and doesn't cause serious problems, but I'd like a double check from both of you.
I have pushed the commit to Fedora Rawhide so you can view it there, but I haven't issued a Rawhide build yet.
* Carlos O'Donell:
I am fairly confident that the code works and doesn't cause serious problems, but I'd like a double check from both of you.
The new algorithm will not find a chunk if it has been consolidated with its preceding chunk (at a lower address), but not with its successor. The unsorted bins aren't searched, either.
I expected that you would search bins which are larger than the request size, too, in case the aligned pointer is further into the allocation.
The list updates lack the usual security checks, and there are some implicit null pointer checks (style-only issue).
Thanks, Florian
On 6/24/19 7:11 AM, Florian Weimer wrote:
- Carlos O'Donell:
I am fairly confident that the code works and doesn't cause serious problems, but I'd like a double check from both of you.
The new algorithm will not find a chunk if it has been consolidated with its preceding chunk (at a lower address), but not with its successor.
This is true.
We could improve it to try look for this case.
I see 3 cases:
1- Consolidation has not happened, and we have the header, chunk, and remainder, and chunk is aligned, we find it and we return.
2- Consolidation has happened and we have one large chunk again, and it's unaligned, but we'll use it, and split it into a header, chunk and remainder; returning the aligned chunk.
3- Consolidation has happened and we have consolidated only with the header, and we have a perfect opportunity to split just the header off.
We handle case 1 and 2, but not 3.
Did I describe your position correctly?
The unsorted bins aren't searched, either.
This is a potentially costly operation.
I expected that you would search bins which are larger than the request size, too, in case the aligned pointer is further into the allocation.
This is equivalent to case 3 above, you can think of it as a case where consolidation occurred, and we might want to split into a header + chunk.
The list updates lack the usual security checks, and there are some implicit null pointer checks (style-only issue).
We can clean these up with some subsequent refinements.
So let me summarize:
a- We support only the additional case 1, which is sufficiently sized and aligned chunks.
b- We don't support case 3, which is a consolidated header and aligned chunk.
c- We don't support scanning for chunks which fall inbetween a correctly sized and aligned chunk, and a chunk that is sized correctly for the aligned chunk to produce a header + footer only (no remainder, though internal fragmentation can happen here if a remainder is too small to be split out for chunk minsize). This kind of chunk is never returned via the fallback naive method because it's not large enough.
d- New code needs more security checks.
e- New code has style issues.
Do we need to address any of (a) through (e) before we start testing this in Rawhide?
We can't release F31 with this patch so we will have to back it out before too long. Is this a problem given the imminent release? Do we need to wait for F31 to branch before doing this kind of testing?
* Carlos O'Donell:
On 6/24/19 7:11 AM, Florian Weimer wrote:
- Carlos O'Donell:
I am fairly confident that the code works and doesn't cause serious problems, but I'd like a double check from both of you.
The new algorithm will not find a chunk if it has been consolidated with its preceding chunk (at a lower address), but not with its successor.
This is true.
We could improve it to try look for this case.
I see 3 cases:
1- Consolidation has not happened, and we have the header, chunk, and remainder, and chunk is aligned, we find it and we return.
2- Consolidation has happened and we have one large chunk again, and it's unaligned, but we'll use it, and split it into a header, chunk and remainder; returning the aligned chunk.
3- Consolidation has happened and we have consolidated only with the header, and we have a perfect opportunity to split just the header off.
We handle case 1 and 2, but not 3.
Did I describe your position correctly?
I failed to mention that consolidation with its successor moves a chunk to a different size bin. We won't find that either.
The unsorted bins aren't searched, either.
This is a potentially costly operation.
I don't think so, regular malloc has to do it as well, so at worst we search it twice, but it could also search it once, with a better search criterion.
I expected that you would search bins which are larger than the request size, too, in case the aligned pointer is further into the allocation.
This is equivalent to case 3 above, you can think of it as a case where consolidation occurred, and we might want to split into a header + chunk.
The list updates lack the usual security checks, and there are some implicit null pointer checks (style-only issue).
We can clean these up with some subsequent refinements.
So let me summarize:
a- We support only the additional case 1, which is sufficiently sized and aligned chunks.
Not sufficiently sized, exactly sized.
My concern is that it's very hard to describe what this change does.
We can't release F31 with this patch so we will have to back it out before too long. Is this a problem given the imminent release? Do we need to wait for F31 to branch before doing this kind of testing?
Ideally, we would wait, yes.
Thanks, Florian
On 6/24/19 10:44 AM, Florian Weimer wrote:
We can't release F31 with this patch so we will have to back it out before too long. Is this a problem given the imminent release? Do we need to wait for F31 to branch before doing this kind of testing?
Ideally, we would wait, yes.
Patch reverted and revert pushed. We never did a build so the NEVRA is unused.
We'll come back to this and improve the patch.