Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

msgpass benchmark and its refactoring dependencies #659

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

nwf-msr
Copy link
Contributor

@nwf-msr nwf-msr commented May 24, 2024

This is just enough of #637 to get the benchmark itself landed, but that also pulls in the freelist refactoring and the option of tweaking obfuscation keys (but not any usage of that internally).

It's probably worth getting someone with more cryptographic and/or red-teaming chops to think about the particular tweaking function, too.

Comment on lines +45 to +72
#ifdef SNMALLOC_PASS_THROUGH
struct MyAlloc
{
MyAlloc() {}
void* alloc(size_t sz)
{
return malloc(sz);
}
void dealloc(void* p)
{
free(p);
}
};
#else
struct MyAlloc
{
snmalloc::Alloc& a;
MyAlloc() : a(ThreadAlloc::get()) {}
void* alloc(size_t sz)
{
return a.alloc(sz);
}
void dealloc(void* p)
{
a.dealloc(p);
}
};
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just use malloc and free in the example and do something like

#ifndef SNMALLOC_PASS_THROUGH
#include <snmalloc/override/malloc.cc>
#endif

I have been wondering about doing this for more of the examples, so that the tests could be less dependent on snmalloc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do. The ability to grab the thread allocator once per thread in the statically-snmalloc case seemed kind of nice (and you can get the dynamic behavior by using the malloc-passthrough and LD_PRELOAD-ing snmallocshim), but maybe it's not actually useful.

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think it can be merged as is, and we can make the changes in a future PR.

Comment on lines +146 to +168
void proxy(const struct params* param, size_t qix)
{
auto& myq = param->msgqueue[qix];
auto& qs = param->msgqueue;

chatty("Px %zu q is %p\n", qix, &myq);

xoroshiro::p128r32 r(1234 + qix, qix);
do
{
if (myq.can_dequeue(domesticate_nop, domesticate_nop))
{
myq.dequeue(
domesticate_nop, domesticate_nop, [qs, qix, &r](freelist::HeadPtr o) {
auto rcptqix = r.next() % qix;

chatty(
"Px %zu send %p to %zu\n", qix, o.as_void().unsafe_ptr(), rcptqix);

qs[rcptqix].enqueue(o, o, domesticate_nop);
return true;
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be better if the proxy did a bit of deallocation, and allocation. I.e. passes some things on, deallocates somethings, and allocates new things of a different size, which it then passes on. I think we can leave as is for now, but perhaps change that in a subsequent PR.

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think it can be merged as is, and we can make the changes in a future PR.

@nwf-msr
Copy link
Contributor Author

nwf-msr commented Jun 13, 2024

Matt makes the good point that tweaking just one side of the obfuscation feels less wrong than tweaking both, so I'll make that change and force push.

Approximate a message-passing application as a set of producers, a set of
consumers, and a set of proxies that do both.  We'll use this for some initial
insight for #634 but it seems worth
having in general.
@nwf-msr nwf-msr merged commit 835ab51 into microsoft:main Jun 13, 2024
47 checks passed
@nwf-msr nwf-msr deleted the 202405-msgpass branch June 13, 2024 21:28
@nwf-msr nwf-msr mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants