Hi - Nice. I like how opening and closing the strace file is what controls the tracing. No magic ctl messages.
My main concerns are about concurrency issues. Also, there's some follow-up work we need to do, like consolidating the syscall tracers. Comments below. On 2016-01-26 at 01:42 ron minnich <[email protected]> wrote: > how do you spell strace on akaros? > > 2016/01/25 17:34:06 Pull is The following changes since commit > 915eac00a7e0f578f9e921af2b205b6efa3739b5: > > Slices: A growable list of pointers. (2016-01-25 11:02:20 -0500) > > are available in the git repository at: > > [email protected]:rminnich/akaros strace > From 20efeb14ed9d474aa3c99864d53e881d0746e22c Mon Sep 17 00:00:00 2001 > From: "Ronald G. Minnich" <[email protected]> > Date: Mon, 25 Jan 2016 14:26:22 -0800 > Subject: New and easy strace framework. > diff --git a/kern/drivers/dev/proc.c b/kern/drivers/dev/proc.c > @@ -730,8 +740,19 @@ static void procclose(struct chan *c) > */ > spin_unlock(&tlock); > } > + if (QID(c->qid) == Qsyscall) { > + if (c->aux) > + qclose(c->aux); > + c->aux = NULL; > + } What's the deal with Qsyscall here? Just leftovers from hacking? > if (QID(c->qid) == Qns && c->aux != 0) > kfree(c->aux); > + if (QID(c->qid) == Qstrace) { > + /* The tracer unconditionally closes the queue. */ > + if (c->aux) > + qclose(c->aux); > + c->aux = NULL; > + } > } > @@ -1317,6 +1342,13 @@ static long procwrite(struct chan *c, void *va, long > n, int64_t off) > procctlreq(p, va, n); > break; > > + /* this lets your write a marker into the data stream, > + * which is a very powerful tool. */ > + // IF ONLY IT DID NOT BREAK THE QUEUE. BUT IT DOES :-( > + case Qstrace: > + if (c->aux && (!qisclosed(c->aux))) > + return qwrite(c->aux, va, n); > + Can we fix or remove this? > diff --git a/kern/include/env.h b/kern/include/env.h > + /* I'm sorry to grow proc for this, but boy is it useful. > + * If strace is not nil, and the queue is open, then format > + * a syscall trace record and push it in. */ > + struct chan *strace; No apology needed. =) > diff --git a/kern/include/ns.h b/kern/include/ns.h > -void cclose(struct chan *); > +int cclose(struct chan *); This might be unnecessary (more down below). > diff --git a/kern/src/hexdump.c b/kern/src/hexdump.c > /* Print a string, with printables preserved, and \xxx where not possible. */ > -int printdump(char *buf, int buflen, uint8_t *data) > +int printdump(char *buf, int numprint, int buflen, uint8_t *data) > { > int ret = 0; > int ix = 0; > - while (ret < buflen) { > + buf[ret++] = '\''; I think there's an issue when buflen is too small, since we assume there's room for at least the initial \'. > + while (ix < numprint && ret < (buflen-2)) { > if (isprint(data[ix])) { > buf[ret++] = data[ix]; > } else if (ret < buflen - 4) { > @@ -87,5 +88,6 @@ int printdump(char *buf, int buflen, uint8_t *data) > } > ix++; > } > + buf[ret++] = '\''; > return ret; > } > diff --git a/kern/src/ns/chan.c b/kern/src/ns/chan.c > index 40802cdb95d3..be6f931630bd 100644 > --- a/kern/src/ns/chan.c > +++ b/kern/src/ns/chan.c > @@ -305,15 +305,16 @@ void chanfree(struct chan *c) > spin_unlock(&(&chanalloc)->lock); > } > > -void cclose(struct chan *c) > +int cclose(struct chan *c) > { > if (c == 0) > - return; > + return 0; > > if (c->flag & CFREE) > panic("cclose %p", getcallerpc(&c)); > > kref_put(&c->ref); > + return kref_refcnt(&c->ref); This is a little dangerous, in general. What you're really doing is getting rid of your ref and finding out about how many other refs there are. You don't know who has the other ref or what they are about to do with it (such as incref!). Later on, it looks like you want to do a "second to last one out, do a hangup". I think we need to come up with something else. Even doing something like "once we're down to one ref, do FOO" is often broken, unless you have some other method to make sure no one increfs and uses your object in the interim. I go through some gymnastics doing this with the dentry cache. More on this below. > diff --git a/kern/src/process.c b/kern/src/process.c > @@ -882,6 +882,15 @@ void proc_destroy(struct proc *p) > close_fdt(&p->open_files, FALSE); > /* Tell the ksched about our death, and which cores we freed up */ > __sched_proc_destroy(p, pc_arr, nr_cores_revoked); > + /* It's ok to do this now, there's nothing left to trace. */ > + /* there is one special case here: if the refcnt is 1 after the cclose. > + * If so, the tracer has it open and will never close. We need to > + * force drain it. */ > + if (p->strace) { > + if (cclose(p->strace) == 1) > + qhangup(p->strace->aux, "Tracing done"); > + p->strace = NULL; > + } There might be some races here. We're in proc_destroy, but there are a bunch of process references out there, and there are still some syscalls in flight. We could have a case where a syscall is finishing right as we do the cclose. A better place to clear p->strace is in __proc_free, which is where we know no one has a ref on p, meaning there are no syscalls in flight. We'll still need to sort out what the deal is with hangup. Perhaps in __proc_free() we can do it? Is the concern that we want to hangup the queue once we are sure there are no more syscalls ever? In that case, we can just do it in __proc_free, and not worry about cclose returning the refcnt. > diff --git a/kern/src/syscall.c b/kern/src/syscall.c > +struct systrace_record *sctrace(struct systrace_record *trace, I like this function in general. We have a bunch of half-baked syscall tracers going on right now - as future work, we need to combine them and probably just use this sctrace. One option would be to call this during run_local_syscall(). That's where the other tracing is (systrace_start_trace(). Until we merge that stuff together, we basically have the same code in multiple places. Also, if you call sctrace from run_local_syscall(), you have access to the struct syscall *, if that helps at all. > + struct proc *p, > + uintreg_t sc_num, > + uintreg_t a0, > + uintreg_t a1, > + uintreg_t a2, > + uintreg_t a3, > + uintreg_t a4, > + uintreg_t a5, > + uintreg_t ret) > +{ > + int n; > + uintreg_t cp = 0; > + int datalen = 0; > + > + if (!p->strace) > + return NULL; I don't think this check protects us from anything. We check for p->strace before calling this every time, and if there's a race, we'd just get hosed anyways. I know it's nice to be safe, but overall I'm concerned about concurrency with all of this. > + if (!p->strace->aux || qisclosed(p->strace->aux)) { > + cclose(p->strace); > + p->strace = NULL; > + return NULL; > + } Do we ever have p->strace but not p->strace->aux? If not, then we should assert here (or just not check). The line that sets p->strace = NULL is a bit worrisome. Other code should be doing that, if at all. (I think we need to hold off on zeroing it until __proc_free(). > intreg_t syscall(struct proc *p, uintreg_t sc_num, uintreg_t a0, uintreg_t > a1, > uintreg_t a2, uintreg_t a3, uintreg_t a4, uintreg_t a5) > { > + struct systrace_record *trace = NULL; > struct per_cpu_info *pcpui = &per_cpu_info[core_id()]; > intreg_t ret = -1; > ERRSTACK(1); > > + if (p->strace) > + trace = sctrace(NULL, p, sc_num, a0, a1, a2, a3, a4, a5, 0); > + > if (sc_num > max_syscall || syscall_table[sc_num].call == NULL) { > printk("[kernel] Invalid syscall %d for proc %d\n", sc_num, > p->pid); > printk("\tArgs: %p, %p, %p, %p, %p, %p\n", a0, a1, a2, a3, a4, > a5); > @@ -2495,11 +2590,18 @@ intreg_t syscall(struct proc *p, uintreg_t sc_num, > uintreg_t a0, uintreg_t a1, > /* if we got here, then the errbuf was right. > * no need to check! > */ > + kfree(trace); > return -1; > } > //printd("before syscall errstack %p\n", errstack); > //printd("before syscall errstack base %p\n", get_cur_errbuf()); > ret = syscall_table[sc_num].call(p, a0, a1, a2, a3, a4, a5); > + /* This is a little hokey if tracing got turned on between call and > + * return. We'll probably want to improve this a bit. */ > + if (p->strace && trace) > + sctrace(trace, p, sc_num, a0, a1, a2, a3, a4, a5, ret); Does p->strace ever go away? If so, then we definitely have a problem. (we read p->strace, it gets freed, then we use it...). If not, then we probably should do the qisclosed check before bothing to call sctrace. o/w we'll pay the marshalling tax on every syscall for a process that ever was straced. (minor thing. or we pass in sysc*, and call this from somewhere else). > diff --git a/tests/strace.c b/tests/strace.c > + pid = fork(); > + if (pid == 0) { > + /* block until the parent is ready to watch us. */ > + if (read(pfd[1], buf, 1) < 1) { > + fprintf(stderr, "Read from child pipe %d: %r\n", > pfd[1]); > + exit(1); > + } > + if (execv(argv[1], argv+1)) { > + fprintf(stderr, "Exec %s: %r\n", argv[1]); > + exit(1); > + } > + } For future reference, you might be able to get rid of the pipe, fork, and exec. We can create processes (see tests/old/spawn.c), then open their strace file, and then make them runnable. create/runnable is the preferred method of making processes in Akaros. fork/exec is more for legacy apps. Barret -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
