Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1709142

Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring buffer overwrite

Show all headers | View raw

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.

> 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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v8 06/14] lockdep: Detect and handle hist_lock ring buffer overwrite Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
  Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Peter Zijlstra <peterz@infradead.org> - 2017-08-09 16:20 +0200
    Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Byungchul Park <byungchul.park@lge.com> - 2017-08-10 03:40 +0200
      Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Peter Zijlstra <peterz@infradead.org> - 2017-08-10 11:30 +0200
    Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Byungchul Park <byungchul.park@lge.com> - 2017-08-10 12:40 +0200
  Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Boqun Feng <boqun.feng@gmail.com> - 2017-08-10 14:00 +0200
    RE: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring buffer overwrite "Byungchul Park" <byungchul.park@lge.com> - 2017-08-10 14:30 +0200
      Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Boqun Feng <boqun.feng@gmail.com> - 2017-08-10 15:00 +0200
        Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Boqun Feng <boqun.feng@gmail.com> - 2017-08-10 15:20 +0200
          Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Byungchul Park <byungchul.park@lge.com> - 2017-08-11 02:50 +0200
          Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Byungchul Park <byungchul.park@lge.com> - 2017-08-11 05:50 +0200
            Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Boqun Feng <boqun.feng@gmail.com> - 2017-08-11 10:10 +0200
              Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Byungchul Park <byungchul.park@lge.com> - 2017-08-11 11:00 +0200
                Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Byungchul Park <byungchul.park@lge.com> - 2017-08-11 11:50 +0200
                Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring buffer overwrite Byungchul Park <max.byungchul.park@gmail.com> - 2017-08-11 15:10 +0200
        Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Byungchul Park <byungchul.park@lge.com> - 2017-08-11 02:50 +0200
          Re: [PATCH v8 06/14] lockdep: Detect and handle hist_lock ring  buffer overwrite Boqun Feng <boqun.feng@gmail.com> - 2017-08-11 03:10 +0200
  [tip:locking/core] locking/lockdep: Detect and handle hist_lock  ring buffer overwrite tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-10 14:30 +0200

csiph-web