On Fri, Aug 11, 2017 at 12:43:28PM +0900, Byungchul Park wrote:
> On Thu, Aug 10, 2017 at 09:17:37PM +0800, Boqun Feng wrote:
> > > > > > @@ -4826,6 +4851,7 @@ static inline int depend_after(struct held_lock
> > > > > *hlock)
> > > > > > * Check if the xhlock is valid, which would be false if,
> > > > > > *
> > > > > > * 1. Has not used after initializaion yet.
> > > > > > + * 2. Got invalidated.
> > > > > > *
> > > > > > * Remind hist_lock is implemented as a ring buffer.
> > > > > > */
> > > > > > @@ -4857,6 +4883,7 @@ static void add_xhlock(struct held_lock *hlock)
> > > > > >
> > > > > > /* Initialize hist_lock's members */
> > > > > > xhlock->hlock = *hlock;
> > > > > > + xhlock->hist_id = current->hist_id++;
> > >
> > > Besides, is this code correct? Does this just make xhlock->hist_id
> > > one-less-than the curr->hist_id, which cause the invalidation every time
> > > you do ring buffer unwinding?
> > >
> > > Regards,
> > > Boqun
> > >
> >
> > So basically, I'm suggesting do this on top of your patch, there is also
> > a fix in commit_xhlocks(), which I think you should swap the parameters
> > in before(...), no matter using task_struct::hist_id or using
> > task_struct::xhlock_idx as the timestamp.
> >
> > Hope this could make my point more clear, and if I do miss something,
> > please point it out, thanks ;-)
>
> Sorry for mis-understanding. I like your patch. I think it works.
>
Thanks for taking a look at it ;-)
> Additionally.. See below..
>
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 074872f016f8..886ba79bfc38 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -854,9 +854,6 @@ struct task_struct {
> > unsigned int xhlock_idx;
> > /* For restoring at history boundaries */
> > unsigned int xhlock_idx_hist[XHLOCK_NR];
> > - unsigned int hist_id;
> > - /* For overwrite check at each context exit */
> > - unsigned int hist_id_save[XHLOCK_NR];
> > #endif
> >
> > #ifdef CONFIG_UBSAN
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 699fbeab1920..04c6c8d68e18 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -4752,10 +4752,8 @@ void crossrelease_hist_start(enum xhlock_context_t c)
> > {
> > struct task_struct *cur = current;
> >
> > - if (cur->xhlocks) {
> > + if (cur->xhlocks)
> > cur->xhlock_idx_hist[c] = cur->xhlock_idx;
> > - cur->hist_id_save[c] = cur->hist_id;
> > - }
> > }
> >
> > void crossrelease_hist_end(enum xhlock_context_t c)
> > @@ -4769,7 +4767,7 @@ void crossrelease_hist_end(enum xhlock_context_t c)
> > cur->xhlock_idx = idx;
> >
> > /* Check if the ring was overwritten. */
> > - if (h->hist_id != cur->hist_id_save[c])
> > + if (h->hist_id != idx)
> > invalidate_xhlock(h);
> > }
> > }
> > @@ -4849,7 +4847,7 @@ static void add_xhlock(struct held_lock *hlock)
> >
> > /* Initialize hist_lock's members */
> > xhlock->hlock = *hlock;
> > - xhlock->hist_id = current->hist_id++;
> > + xhlock->hist_id = idx;
> >
> > xhlock->trace.nr_entries = 0;
> > xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> > @@ -5005,7 +5003,7 @@ static int commit_xhlock(struct cross_lock *xlock, struct hist_lock *xhlock)
> > static void commit_xhlocks(struct cross_lock *xlock)
> > {
> > unsigned int cur = current->xhlock_idx;
> > - unsigned int prev_hist_id = xhlock(cur).hist_id;
> > + unsigned int prev_hist_id = cur + 1;
>
> I should have named it another. Could you suggest a better one?
>
I think "prev" is fine, because I thought the "previous" means the
xhlock item we visit _previously_.
> > unsigned int i;
> >
> > if (!graph_lock())
> > @@ -5030,7 +5028,7 @@ static void commit_xhlocks(struct cross_lock *xlock)
> > * hist_id than the following one, which is impossible
> > * otherwise.
>
> Or we need to modify the comment so that the word 'prev' does not make
> readers confused. It was my mistake.
>
I think the comment needs some help, but before you do it, could you
have another look at what Peter proposed previously? Note you have a
same_context_xhlock() check in the commit_xhlocks(), so the your
previous overwrite case actually could be detected, I think.
However, one thing may not be detected is this case:
ppppppppppppppppppppppppppppppppppwwwwwwww
wrapped > wwwwwww
where p: process and w: worker.
, because p and w are in the same task_irq_context(). I discussed this
with Peter yesterday, and he has a good idea: unconditionally do a reset
on the ring buffer whenever we do a crossrelease_hist_end(XHLOCK_PROC).
Basically it means we empty the lock history whenever we finished a
worker function in a worker thread or we are about to return to
userspace after we finish the syscall. This could further save some
memory and so I think this may be better than my approach.
How does this sound to you?
Regards,
Boqun
> Thanks,
> Byungchul
>
Attachment:
signature.asc
Description: PGP signature
