[ccan] [PATCH] altstack: stack alignment and accounting tweaks
Dan Good
dan at dancancode.com
Wed Feb 17 04:41:43 AEDT 2016
I see. Thank you.
On Tue, Feb 16, 2016 at 5:04 AM David Gibson <david at gibson.dropbear.id.au>
wrote:
> On Mon, Feb 15, 2016 at 06:29:10PM +0000, Dan Good wrote:
> > I thought I understood from our emails at the end of last year that a
> > single "polished" commit was desirable. Is that not the same thing as
> > taking several very small commits and bundling them into one?
>
> Often not, in practice. The ultimate goal is reviewability (both
> before merging and when looking back at the commit history). But
> that's often accomplished different ways depending on the situation.
> In particular it tends to be different for initial commits of new code
> from changes to existing code.
>
> For initial commits, it's generally best to elide patches which
> correct false steps made during development, or really anything which
> doesn't increase the total code size or complexity significantly.
> That gives less commits to review, and cleaner code in them to
> understand.
>
> For changes to existing code, however, the important thing is
> understanding why the change is desirable and then that the changes
> actually accomplish that goal. That tends towards small, single
> purpose commits, so it's easy to see what changes belong to what goal
> (explained in the commit message).
>
> Of course, that's a very broad rule of tumb, with plenty of exceptions
> and wiggle room.
>
> > On Mon, Feb 15, 2016 at 5:53 AM David Gibson <
> david at gibson.dropbear.id.au>
> > wrote:
> >
> > > On Wed, Feb 03, 2016 at 11:42:11PM +0000, Dan Good wrote:
> > > > "messy bitser" - what does that mean? That's bad, isn't it?
> > >
> > > It's... mildly bad.
> > >
> > > By "bitser" I mean that it contains a number of changes that aren't
> > > closely related to each other. In general that makes a patch more
> > > difficult to review. How important that is depends on the complexity
> > > of the changes and how mature the code being altered is, so it's not
> > > really a big deal.
> > >
> > > The fact that altstack has been breaking Travis builds for a while
> > > probably predisposed me to be uncharitable.
> > >
> > > > Can I help fix the builds?
> > >
> > > So, I've figured out what the problem is, patch coming shortly.
> > >
> > > >
> > > > On Wed, Feb 3, 2016 at 4:28 PM David Gibson <
> david at gibson.dropbear.id.au
> > > >
> > > > wrote:
> > > >
> > > > > On Mon, Feb 01, 2016 at 02:43:16AM +0000, Dan Good wrote:
> > > > > >
> > > > > > * add altstack_remn, returns amount of stack remaining
> > > > > > * increase mapping by 1 page to handle abutment case
> > > > > > * capture rsp earlier
> > > > > > * align stack to 16 bytes
> > > > > >
> > > > > > Signed-off-by: Dan Good <dan at dancancode.com>
> > > > >
> > > > > Bit of a messy bitser, but doesn't look it will make anything
> > > > > worse. so go ahead and push.
> > > > >
> > > > > Unfortunately, altstack is breaking travis builds at the moment
> (see,
> > > > > e.g. https://travis-ci.org/dgibson/ccan/jobs/106661592) - but not
> > > > > local builds for me which is making it difficult to debug.
> > > > >
> > > > > Some investigation showed that the test prog was getting a SIGBUS,
> but
> > > > > I didn't get further than that.
> > > > >
> > > > > altstack also breaks "make all" on any non-x86_64 platform
> (including
> > > > > 32-bit x86) which is pretty horrid. For that I think we need to
> > > > > adjust the makefiles to look at rusty's new "ported" stuff from
> _info.
> > > > >
> > > > >
> > > > > > ---
> > > > > > ccan/altstack/altstack.c | 17 ++++++++++++++---
> > > > > > ccan/altstack/altstack.h | 14 ++++++++++++++
> > > > > > ccan/altstack/test/run.c | 20 +++++++++++++-------
> > > > > > 3 files changed, 41 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
> > > > > > index 6faf38f..b71c64f 100644
> > > > > > --- a/ccan/altstack/altstack.c
> > > > > > +++ b/ccan/altstack/altstack.c
> > > > > > @@ -8,6 +8,7 @@
> > > > > > #include <signal.h>
> > > > > > #include <stdio.h>
> > > > > > #include <string.h>
> > > > > > +#include <unistd.h>
> > > > > > #include <sys/mman.h>
> > > > > >
> > > > > > static __thread char ebuf[ALTSTACK_ERR_MAXLEN];
> > > > > > @@ -37,6 +38,11 @@ static void segvjmp(int signum)
> > > > > > }
> > > > > >
> > > > > > static __thread void *rsp_save_[2];
> > > > > > +static __thread rlim_t max_;
> > > > > > +
> > > > > > +rlim_t altstack_max(void) {
> > > > > > + return max_;
> > > > > > +}
> > > > > >
> > > > > > static ptrdiff_t rsp_save(unsigned i) {
> > > > > > assert(i < 2);
> > > > > > @@ -57,6 +63,7 @@ static __thread void *arg_, *out_;
> > > > > >
> > > > > > int altstack(rlim_t max, void *(*fn)(void *), void *arg, void
> **out)
> > > > > > {
> > > > > > + long pgsz = sysconf(_SC_PAGESIZE);
> > > > > > int ret = -1, undo = 0;
> > > > > > char *m;
> > > > > > struct rlimit rl_save;
> > > > > > @@ -69,11 +76,16 @@ int altstack(rlim_t max, void *(*fn)(void *),
> > > void
> > > > > *arg, void **out)
> > > > > > fn_ = fn;
> > > > > > arg_ = arg;
> > > > > > out_ = 0;
> > > > > > + max_ = max;
> > > > > > ebuf[elen = 0] = '\0';
> > > > > > if (out) *out = 0;
> > > > > >
> > > > > > + // if the first page below the mapping is in use, we get
> > > max-pgsz
> > > > > usable bytes
> > > > > > + // add pgsz to max to guarantee at least max usable bytes
> > > > > > + max += pgsz;
> > > > > > +
> > > > > > ok(getrlimit(RLIMIT_STACK, &rl_save), 1);
> > > > > > - ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { max,
> > > > > rl_save.rlim_max }), 1);
> > > > > > + ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { max_,
> > > > > rl_save.rlim_max }), 1);
> > > > > > undo++;
> > > > > >
> > > > > > ok(m = mmap(0, max, PROT_READ|PROT_WRITE,
> > > > > MAP_PRIVATE|MAP_ANONYMOUS|MAP_GROWSDOWN|MAP_NORESERVE, -1, 0), 1);
> > > > > > @@ -91,8 +103,7 @@ int altstack(rlim_t max, void *(*fn)(void *),
> void
> > > > > *arg, void **out)
> > > > > > ok(sigaction(SIGSEGV, &sa, &sa_save), 1);
> > > > > > undo++;
> > > > > >
> > > > > > - asm volatile ("movq %%rsp, %%r10\nmov %0,
> %%rsp\npush
> > > > > %%r10" : : "g" (m + max) : "r10");
> > > > > > - rsp_save(0);
> > > > > > + asm volatile ("movq %%rsp, %%r10\nmov %1,
> %%rsp\nmov
> > > > > %%rsp, %0\nsub $8, %%rsp\npush %%r10" : "=g" (rsp_save_[0]) : "g"
> (m +
> > > max)
> > > > > : "r10");
> > > > > > out_ = fn_(arg_);
> > > > > > asm volatile ("pop %rsp");
> > > > > > ret = 0;
> > > > > > diff --git a/ccan/altstack/altstack.h b/ccan/altstack/altstack.h
> > > > > > index 5570e7b..4445a2a 100644
> > > > > > --- a/ccan/altstack/altstack.h
> > > > > > +++ b/ccan/altstack/altstack.h
> > > > > > @@ -104,6 +104,20 @@ char *altstack_geterr(void);
> > > > > > ptrdiff_t altstack_used(void);
> > > > > >
> > > > > > /**
> > > > > > + * altstack_max - return usable stack size
> > > > > > + *
> > > > > > + * Returns: max value from altstack() call
> > > > > > + */
> > > > > > +rlim_t altstack_max(void);
> > > > > > +
> > > > > > +/**
> > > > > > + * altstack_remn - return amount of stack remaining
> > > > > > + *
> > > > > > + * Returns: altstack_max() minus altstack_used()
> > > > > > + */
> > > > > > +#define altstack_remn() (altstack_max() - altstack_used())
> > > > > > +
> > > > > > +/**
> > > > > > * altstack_rsp_save - set initial rsp value
> > > > > > *
> > > > > > * Capture the current value of rsp for future altstack_used()
> > > > > > diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
> > > > > > index adc1020..d0b8d28 100644
> > > > > > --- a/ccan/altstack/test/run.c
> > > > > > +++ b/ccan/altstack/test/run.c
> > > > > > @@ -4,6 +4,7 @@
> > > > > > #include <setjmp.h>
> > > > > > #include <signal.h>
> > > > > > #include <string.h>
> > > > > > +#include <unistd.h>
> > > > > > #include <sys/mman.h>
> > > > > > #include <ccan/tap/tap.h>
> > > > > > #include <ccan/altstack/altstack.h>
> > > > > > @@ -20,13 +21,13 @@ enum {
> > > > > > };
> > > > > > int fail, call1, call2;
> > > > > > char *m_;
> > > > > > -rlim_t max_;
> > > > > > +rlim_t msz_;
> > > > > > #define e(x) (900+(x))
> > > > > > #define seterr(x) (errno = e(x))
> > > > > > #define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |=
> errno ||
> > > > > out_ ? (x) : 0))
> > > > > > #define getrlimit(...) (fail&getrlimit_ ?
> > > > > (seterr(getrlimit_), -1) : (setcall(getrlimit_),
> > > > > getrlimit(__VA_ARGS__)))
> > > > > > #define mmap(...) (fail&mmap_ ?
> > > (seterr(mmap_),
> > > > > (void *)-1) : (setcall(mmap_), mmap(__VA_ARGS__)))
> > > > > > -#define munmap(a, b) (fail&munmap_ ?
> > > > > (seterr(munmap_), -1) : (setcall(munmap_),
> > > > > munmap(m_=(a), max_=(b))))
> > > > > > +#define munmap(a, b) (fail&munmap_ ?
> > > > > (seterr(munmap_), -1) : (setcall(munmap_),
> > > > > munmap(m_=(a), msz_=(b))))
> > > > > > #define setrlimit(...) (fail&setrlimit_ ?
> > > > > (seterr(setrlimit_), -1) : (setcall(setrlimit_),
> > > > > setrlimit(__VA_ARGS__)))
> > > > > > #define sigaltstack(...) (fail&sigaltstack_ ?
> > > > > (seterr(sigaltstack_), -1) : (setcall(sigaltstack_),
> > > > > sigaltstack(__VA_ARGS__)))
> > > > > > #define sigaction(...) (fail&sigaction_ ?
> > > > > (seterr(sigaction_), -1) : (setcall(sigaction_),
> > > > > sigaction(__VA_ARGS__)))
> > > > > > @@ -58,7 +59,9 @@ static void *wrap(void *i)
> > > > > >
> > > > > > int main(void)
> > > > > > {
> > > > > > - plan_tests(16);
> > > > > > + long pgsz = sysconf(_SC_PAGESIZE);
> > > > > > +
> > > > > > + plan_tests(17);
> > > > > >
> > > > > > #define chkfail(x, y, z, c1, c2) (call1 = 0, call2 = 0, errno =
> 0,
> > > > > ok1((fail = x) && (y) && errno == (z) && call1 == (c1) && call2 ==
> > > (c2)));
> > > > > > #define chkok( y, z, c1, c2) (call1 = 0, call2 = 0, errno =
> 0,
> > > fail
> > > > > = 0, ok1((y) && errno == (z) && call1 == (c1) && call2 ==
> (c2)));
> > > > > > @@ -86,7 +89,7 @@ int main(void)
> > > > > > chkfail(munmap_, altstack(8*MiB, wrap, 0, 0) == 1,
> > > > > e(munmap_),
> > > > > >
> getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > > > > setrlimit_|sigaltstack_|sigaction_);
> > > > > > - if (fail = 0, munmap(m_, max_) == -1)
> > > > > > + if (fail = 0, munmap(m_, msz_) == -1)
> > > > > > err(1, "munmap");
> > > > > >
> > > > > > chkok( altstack(1*MiB, wrap, (void *)
> > > 1000000, 0)
> > > > > == -1, EOVERFLOW,
> > > > > > @@ -102,10 +105,12 @@ int main(void)
> > > > > > chkfail(munmap_, altstack(1*MiB, wrap, (void *)
> > > 1000000, 0)
> > > > > == -1, EOVERFLOW,
> > > > > >
> getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > > > > setrlimit_|sigaltstack_|sigaction_);
> > > > > > - if (fail = 0, munmap(m_, max_) == -1)
> > > > > > + if (fail = 0, munmap(m_, msz_) == -1)
> > > > > > err(1, "munmap");
> > > > > >
> > > > > > - ok1(used > 1*MiB-1*KiB && used < 1*MiB);
> > > > > > + ok1(altstack_max() == 1*MiB);
> > > > > > + diag("used: %lu", used);
> > > > > > + ok1(used >= 1*MiB && used <= 1*MiB + pgsz);
> > > > > >
> > > > > > char *p;
> > > > > > for(p = altstack_geterr(); *p; p++)
> > > > > > @@ -128,7 +133,8 @@ int main(void)
> > > > > >
> getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > > > > setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > > > >
> > > > > > - ok1(used > 8*MiB-8*KiB && used < 8*MiB);
> > > > > > + diag("used: %lu", used);
> > > > > > + ok1(used >= 8*MiB && used <= 8*MiB + pgsz);
> > > > > >
> > > > > > used = 0;
> > > > > > chkok( altstack(8*MiB, wrap, (void *)
> 100000,
> > > 0)
> > > > > == 0, 0,
> > > > >
> > > > >
> > >
> > >
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20160216/5a54fa2f/attachment-0001.html>
More information about the ccan
mailing list