[PATCH] Add Safe-Linking to fastbins and tcache

[Eyal Itkin]

Hi,

I went over the patch and fixed it to conform to the coding standards.
I'm also added it as an attachment, hoping that this way lines won't
break during the transmission of the email.

Regarding the benchmarks, I've just ran them on a simple GCP instance
(1 vCPU) before and after the patch. As you can see below, the change
isn't measurable in the "malloc-simple" tests. Sometimes the vanilla
version is faster (as it should be), but sometimes the patched version
is faster, which usually means that the change is lower than the noise
level on the testing server.

malloc-simple-128 (before patch):
{
 "timing_type": "hp_timing",
 "functions": {
  "malloc": {
   "": {
    "malloc_block_size": 128,
    "max_rss": 1676,
    "main_arena_st_allocs_0025_time": 77.511,
    "main_arena_st_allocs_0100_time": 94.4398,
    "main_arena_st_allocs_0400_time": 93.528,
    "main_arena_st_allocs_1600_time": 161.315,
    "main_arena_mt_allocs_0025_time": 117.634,
    "main_arena_mt_allocs_0100_time": 144.564,
    "main_arena_mt_allocs_0400_time": 159.591,
    "main_arena_mt_allocs_1600_time": 230.882,
    "thread_arena__allocs_0025_time": 120.277,
    "thread_arena__allocs_0100_time": 146.871,
    "thread_arena__allocs_0400_time": 157.205,
    "thread_arena__allocs_1600_time": 223.46
   }
  }
 }
}

malloc-simple-128 (with the patch):
{
 "timing_type": "hp_timing",
 "functions": {
  "malloc": {
   "": {
    "malloc_block_size": 128,
    "max_rss": 1672,
    "main_arena_st_allocs_0025_time": 81.6073,
    "main_arena_st_allocs_0100_time": 94.0351,
    "main_arena_st_allocs_0400_time": 97.1118,
    "main_arena_st_allocs_1600_time": 159.065,
    "main_arena_mt_allocs_0025_time": 123.422,
    "main_arena_mt_allocs_0100_time": 146.074,
    "main_arena_mt_allocs_0400_time": 151.536,
    "main_arena_mt_allocs_1600_time": 215.798,
    "thread_arena__allocs_0025_time": 116.577,
    "thread_arena__allocs_0100_time": 142.024,
    "thread_arena__allocs_0400_time": 158.38,
    "thread_arena__allocs_1600_time": 233.216
   }
  }
 }
}

Will be more than happy to help if there are more questions or
additional fixes needed.
Thanks for your time,
Eyal.

On Thu, Mar 19, 2020 at 5:09 PM Eyal Itkin <eyal.itkin@gmail.com> wrote:
>
> Hi,
>
> Thanks for the feedback, I'll will work on it now and send an updated version.
> I passed all of GLIBC's tests and also benchmakred the results in a
> test I made. I wasn't aware of the fact that there is already a
> benchmarking suite for malloc in GLIBC, and I will now try to use it.
>
> In the assembly level, it adds 3/4 assembly instructions per malloc()
> / free(), and we had great benchmarking results for this feature in
> all of the tests we made thus far so I am not worried about it, but I
> will use your benchmarking suite as well and send you back the
> results.
>
> Thanks again,
> Eyal.
>
> On Thu, Mar 19, 2020 at 3:33 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> >
> > Hi,
> >
> > I tried applying your patch - there seem to be issues with formatting:
> >
> > @@ -327,6 +327,15 @@ __malloc_assert (const char *assertion, const
> > char *file, unsigned int line,
> >  # define MAX_TCACHE_COUNT UINT16_MAX
> >
> > and
> >
> > +#define PROTECT_PTR(pos, ptr, type)     ((type)((((size_t)pos) >>
> > PAGE_SHIFT) ^ ((size_t)ptr)))
> >
> > It seems something cut off those lines as being too long...
> >
> > +          if (__glibc_unlikely(!aligned_OK(p)))
> >
> > There should be spaces before each '('. See the GLIBC coding style:
> > https://www.gnu.org/prep/standards/standards.html#Formatting
> >
> > Did you run the GLIBC malloc benchmarks before/after this change?
> >
> > Cheers,
> > Wilco
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-Safe-Linking-to-fastbins-and-tcache.patch
Type: application/octet-stream
Size: 9117 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20200319/102a307c/attachment-0001.obj>