On Fri, Aug 11, 2017 at 09:40:21AM +0900, Byungchul Park wrote:
> On Thu, Aug 10, 2017 at 08:51:33PM +0800, Boqun Feng wrote:
> > > > > void crossrelease_hist_end(enum context_t c)
> > > > > {
> > > > > - if (current->xhlocks)
> > > > > - current->xhlock_idx = current->xhlock_idx_hist[c];
> > > > > + struct task_struct *cur = current;
> > > > > +
> > > > > + if (cur->xhlocks) {
> > > > > + unsigned int idx = cur->xhlock_idx_hist[c];
> > > > > + struct hist_lock *h = &xhlock(idx);
> > > > > +
> > > > > + cur->xhlock_idx = idx;
> > > > > +
> > > > > + /* Check if the ring was overwritten. */
> > > > > + if (h->hist_id != cur->hist_id_save[c])
> > > >
> > > > Could we use:
> > > >
> > > > if (h->hist_id != idx)
> > >
> > > No, we cannot.
> > >
> >
> > Hey, I'm not buying it. task_struct::hist_id and task_struct::xhlock_idx
> > are increased at the same place(in add_xhlock()), right?
>
> Right.
>
> > And, yes, xhlock_idx will get decreased when we do ring-buffer
>
> This is why we should keep both of them.
>
> > unwinding, but that's OK, because we need to throw away those recently
> > added items.
> >
> > And xhlock_idx always points to the most recently added valid item,
>
> No, it's not true in case that the ring buffer was wrapped like:
>
> ppppppppppppppppppppppppiiiiiiiiiiiiiiiiiiiiiiii
> wrapped > iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii................
> ^
> xhlock_idx points here after unwinding,
> and it's not a valid one.
>
> where p represents an acquisition in process context,
> i represents an acquisition in irq context.
>
Yeah, but we can detect this with comparison between the
hist_lock::hist_id and the task_struct::xhlock_idx in
commit_xhlocks()(see my patch), no?
Regards,
Boqun
> > right? Any other item's idx must "before()" the most recently added
> > one's, right? So ::xhlock_idx acts just like a timestamp, doesn't it?
>
> Both of two answers are _no_.
>
> > Maybe I'm missing something subtle, but could you show me an example,
> > that could end up being a problem if we use xhlock_idx as the hist_id?
>
> See the example above. We cannot detect whether it was wrapped or not using
> xhlock_idx.
>
> >
> > > hist_id is a kind of timestamp and used to detect overwriting
> > > data into places of same indexes of the ring buffer. And idx is
> > > just an index. :) IOW, they mean different things.
> > >
> > > >
> > > > here, and
> > > >
> > > > > + invalidate_xhlock(h);
> > > > > + }
> > > > > }
> > > > >
> > > > > static int cross_lock(struct lockdep_map *lock)
> > > > > @@ -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?
>
> Right. "save = hist_id++" should be "save = ++hist_id". Could you fix it?
>
> Thank you,
> Byungchul
>
Attachment:
signature.asc
Description: PGP signature
