diff mbox series

[v4] malloc: send freed small chunks to smallbin

Message ID YYU7Cu7CCrVtJDV7Es3bAL4ZTpH0Uxw2KOmSqKmLyTdrZrd8Tp5Z2vGmCdBZXGLuGwYD1x_hTT-8Buk_QcFZJinKpAPZeYrZiydWacvjj6E=@proton.me
State New
Headers show
Series [v4] malloc: send freed small chunks to smallbin | expand

Commit Message

k4lizen Oct. 31, 2024, 6:47 p.m. UTC
Large chunks get added to the unsorted bin since
sorting them takes time, for small chunks the
benefit of adding them to the unsorted bin is
non-existant, actually hurting performance.

Splitting and malloc_consolidate still add small
chunks to unsorted, but we can hint the compiler
that that is a relatively rare occurance.
Benchmarking shows this to be consistently good.

Signed-off-by: k4lizen <k4lizen@proton.me>
---
 malloc/malloc.c | 53 +++++++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 19 deletions(-)

Comments

Carlos O'Donell Nov. 4, 2024, 2:30 p.m. UTC | #1
On 10/31/24 2:47 PM, k4lizen wrote:
> Large chunks get added to the unsorted bin since
> sorting them takes time, for small chunks the
> benefit of adding them to the unsorted bin is
> non-existant, actually hurting performance.
> 
> Splitting and malloc_consolidate still add small
> chunks to unsorted, but we can hint the compiler
> that that is a relatively rare occurance.
> Benchmarking shows this to be consistently good.
> 
> Signed-off-by: k4lizen <k4lizen@proton.me>

Please note that under DCO you must use your real name:
https://sourceware.org/glibc/wiki/Contribution%20checklist

It is one of the consequences of DCO, though pseudonyms can
be used with copyright attribution via the FSF.

May you please confirm your real name?
Florian Weimer Nov. 4, 2024, 2:54 p.m. UTC | #2
* Carlos O'Donell:

> On 10/31/24 2:47 PM, k4lizen wrote:
>> Large chunks get added to the unsorted bin since
>> sorting them takes time, for small chunks the
>> benefit of adding them to the unsorted bin is
>> non-existant, actually hurting performance.
>> 
>> Splitting and malloc_consolidate still add small
>> chunks to unsorted, but we can hint the compiler
>> that that is a relatively rare occurance.
>> Benchmarking shows this to be consistently good.
>> 
>> Signed-off-by: k4lizen <k4lizen@proton.me>
>
> Please note that under DCO you must use your real name:
> https://sourceware.org/glibc/wiki/Contribution%20checklist
>
> It is one of the consequences of DCO, though pseudonyms can
> be used with copyright attribution via the FSF.
>
> May you please confirm your real name?

Alternatively, you can start the copyright assignment process with the
FSF.  They will guard your identity.  The available forms are linked
from here: <https://sourceware.org/glibc/wiki/CopyrightFSForDisclaim>
If in doubt, use the request-assign.future form.

Thanks,
Florian
k4lizen Nov. 5, 2024, 3:35 a.m. UTC | #3
Hello,

Carlos:
> Please note that under DCO you must use your real name:
> https://sourceware.org/glibc/wiki/Contribution%20checklist
> 
> It is one of the consequences of DCO, though pseudonyms can
> be used with copyright attribution via the FSF.
> 
> May you please confirm your real name?

Florian:
> Alternatively, you can start the copyright assignment process with the
> FSF.  They will guard your identity.  The available forms are linked
> from here: <https://sourceware.org/glibc/wiki/CopyrightFSForDisclaim>
> If in doubt, use the request-assign.future form.

I would rather not connect my pseudonym to my legal name publicly,
and applying for copyright assignment to the FSF requires submitting
even more personally identifiable information.

I am relatively new to this process and I realize you have experience
with this, but according to what I can tell the DCO does not require
a legal name.

For one, it does not specify in its text that the sign-off has to
include a legal name. This interpretation is corroborated by many
organizations including the Linux kernel foundation (for which the
certificate was originally created if I understand correctly?).

See for example the kernel submitting patches guide:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> then you just add a line saying:
> Signed-off-by: Random J Developer <random@developer.example.org>
> using a known identity (sorry, no anonymous contributions.)

It was written like that explicitly to allow pseudonyms. See the
commit by Linus: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330
It seems the glibc's contribution guidelines copied that part,
but failed to change it when it got updated at the source?

Of interest may also be the Cloud Native Computing Foundation's
note on the matter: https://github.com/cncf/foundation/blob/659fd32c86dc/dco-guidelines.md#dco-and-real-names
> The DCO requires the use of a real name that can be used to 
> identify someone in case there is an issue about a contribution
> they made.
> 
> A real name does not require a legal name, nor a birth name, nor
> any name that appears on an official ID (e.g. a passport). Your 
> real name is the name you convey to people in the community for them 
> to use to identify you as you. The key concern is that your 
> identification is sufficient enough to contact you if an issue were
> to arise in the future about your contribution.
> 
> Your real name should not be an anonymous id or false name that
> misrepresents who you are.

Some reasoning for this was also given in the documentation patch
for qemu: https://lore.kernel.org/all/e013ddeb-185a-48d6-86a7-4a5697f7a3fd@linaro.org/

k4lizen is the name I use for all my contributions, and you'd have
a much easier time reaching me through that name than my legal name.

As such, could I please request that you allow me to sign-off patches
under this name?

Best regards,
k4lizen
Florian Weimer Nov. 5, 2024, 10:08 a.m. UTC | #4
> Carlos:
>> Please note that under DCO you must use your real name:
>> https://sourceware.org/glibc/wiki/Contribution%20checklist
>> 
>> It is one of the consequences of DCO, though pseudonyms can
>> be used with copyright attribution via the FSF.
>> 
>> May you please confirm your real name?
>
> Florian:
>> Alternatively, you can start the copyright assignment process with the
>> FSF.  They will guard your identity.  The available forms are linked
>> from here: <https://sourceware.org/glibc/wiki/CopyrightFSForDisclaim>
>> If in doubt, use the request-assign.future form.
>
> I would rather not connect my pseudonym to my legal name publicly,
> and applying for copyright assignment to the FSF requires submitting
> even more personally identifiable information.
>
> I am relatively new to this process and I realize you have experience
> with this, but according to what I can tell the DCO does not require
> a legal name.
>
> For one, it does not specify in its text that the sign-off has to
> include a legal name. This interpretation is corroborated by many
> organizations including the Linux kernel foundation (for which the
> certificate was originally created if I understand correctly?).

Sorry that your contribution is caught up in this.  The real name
requirement is specific to the glibc project.  I don't know why it's
there.

However, the way the DCO process works is that you just need someone to
submit your patch with their Signed-off-by: line under option (c) of the
DCO (“The contribution was provided directly to me by some other person
who certified (a), (b) or (c) and I have not modified it.”).

Carlos, why is the real name requirement in place?  I think it's not
entirely uncommon that someone submits work on behalf of an anonymous
corporate contributor unknown to us.

Thanks,
Florian
Carlos O'Donell Nov. 22, 2024, 4:56 p.m. UTC | #5
On 11/5/24 5:08 AM, Florian Weimer wrote:
> Carlos, why is the real name requirement in place?  I think it's not
> entirely uncommon that someone submits work on behalf of an anonymous
> corporate contributor unknown to us.

My apologies. Let me clarify.

I wanted to make sure that "k4lizen" was the correctly configured git name to use
when the contribution is committed and attribution given. I should have asked that
question specifically.

Please note that the "real name" requirement is not a requirement for the persons
legal name or another stricter requirement. The intent is for the "real name" to 
be a known identity (if that person is using a pseudonym).

The FSF process, although allowing for psedonymous public contributions (including
using the pseudonym in the GNU Maintainers "sources" file for assignments), still
requires the person to identify themselves to the FSF and that information may
eventually leak.
diff mbox series

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index bcb6e5b83c..60b537992c 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4156,9 +4156,9 @@  _int_malloc (mstate av, size_t bytes)
 #endif
             }
 
-          /* place chunk in bin */
-
-          if (in_smallbin_range (size))
+          /* Place chunk in bin.  Only malloc_consolidate() and splitting can put
+             small chunks into the unsorted bin. */
+          if (__glibc_unlikely (in_smallbin_range (size)))
             {
               victim_index = smallbin_index (size);
               bck = bin_at (av, victim_index);
@@ -4723,23 +4723,39 @@  _int_free_create_chunk (mstate av, mchunkptr p, INTERNAL_SIZE_T size,
       } else
 	clear_inuse_bit_at_offset(nextchunk, 0);
 
-      /*
-	Place the chunk in unsorted chunk list. Chunks are
-	not placed into regular bins until after they have
-	been given one chance to be used in malloc.
-      */
+      mchunkptr bck, fwd;
+
+      if (!in_smallbin_range (size))
+        {
+          /* Place large chunks in unsorted chunk list.  Large chunks are
+             not placed into regular bins until after they have
+             been given one chance to be used in malloc.
+
+             This branch is first in the if-statement to help branch
+             prediction on consecutive adjacent frees. */
+          bck = unsorted_chunks (av);
+          fwd = bck->fd;
+          if (__glibc_unlikely (fwd->bk != bck))
+            malloc_printerr ("free(): corrupted unsorted chunks");
+          p->fd_nextsize = NULL;
+          p->bk_nextsize = NULL;
+        }
+      else
+        {
+          /* Place small chunks directly in their smallbin, so they
+             don't pollute the unsorted bin. */
+          int chunk_index = smallbin_index (size);
+          bck = bin_at (av, chunk_index);
+          fwd = bck->fd;
+
+          if (__glibc_unlikely (fwd->bk != bck))
+            malloc_printerr ("free(): chunks in smallbin corrupted");
+
+          mark_bin (av, chunk_index);
+        }
 
-      mchunkptr bck = unsorted_chunks (av);
-      mchunkptr fwd = bck->fd;
-      if (__glibc_unlikely (fwd->bk != bck))
-	malloc_printerr ("free(): corrupted unsorted chunks");
-      p->fd = fwd;
       p->bk = bck;
-      if (!in_smallbin_range(size))
-	{
-	  p->fd_nextsize = NULL;
-	  p->bk_nextsize = NULL;
-	}
+      p->fd = fwd;
       bck->fd = p;
       fwd->bk = p;
 
@@ -4748,7 +4764,6 @@  _int_free_create_chunk (mstate av, mchunkptr p, INTERNAL_SIZE_T size,
 
       check_free_chunk(av, p);
     }
-
   else
     {
       /* If the chunk borders the current high end of memory,