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

Groups > linux.kernel > #1589439 > unrolled thread

Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-02-28 14:20 +0100
    Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-02-28 15:20 +0100
      Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-02-28 19:40 +0100
        Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-03-01 06:30 +0100

#1589439 — Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature

> +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> +
> +#define idx(t)			((t)->xhlock_idx)
> +#define idx_prev(i)		((i) ? (i) - 1 : MAX_XHLOCKS_NR - 1)
> +#define idx_next(i)		(((i) + 1) % MAX_XHLOCKS_NR)

Note that:

#define idx_prev(i)		(((i) - 1) % MAX_XHLOCKS_NR)
#define idx_next(i)		(((i) + 1) % MAX_XHLOCKS_NR)

is more symmetric and easier to understand.

> +
> +/* For easy access to xhlock */
> +#define xhlock(t, i)		((t)->xhlocks + (i))
> +#define xhlock_prev(t, l)	xhlock(t, idx_prev((l) - (t)->xhlocks))
> +#define xhlock_curr(t)		xhlock(t, idx(t))

So these result in an xhlock pointer

> +#define xhlock_incr(t)		({idx(t) = idx_next(idx(t));})

This does not; which is confusing seeing how they share the same
namespace; also incr is weird.

[toc] | [next] | [standalone]

#1589498

On Tue, Feb 28, 2017 at 02:10:12PM +0100, Peter Zijlstra wrote:
> > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > +
> > +#define idx(t)			((t)->xhlock_idx)
> > +#define idx_prev(i)		((i) ? (i) - 1 : MAX_XHLOCKS_NR - 1)
> > +#define idx_next(i)		(((i) + 1) % MAX_XHLOCKS_NR)
> 
> Note that:
> 
> #define idx_prev(i)		(((i) - 1) % MAX_XHLOCKS_NR)
> #define idx_next(i)		(((i) + 1) % MAX_XHLOCKS_NR)
> 
> is more symmetric and easier to understand.

OK. I will do it after forcing MAX_XHLOCKS_NR to be power of 2. Current
value of it is already power of 2 but I need to add comment explaning it.

> > +
> > +/* For easy access to xhlock */
> > +#define xhlock(t, i)		((t)->xhlocks + (i))
> > +#define xhlock_prev(t, l)	xhlock(t, idx_prev((l) - (t)->xhlocks))
> > +#define xhlock_curr(t)		xhlock(t, idx(t))
> 
> So these result in an xhlock pointer
> 
> > +#define xhlock_incr(t)		({idx(t) = idx_next(idx(t));})
> 
> This does not; which is confusing seeing how they share the same
> namespace; also incr is weird.

OK.. Could you suggest a better name? xhlock_adv()? advance_xhlock()?
And.. replace it with a function?

[toc] | [prev] | [next] | [standalone]

#1589688

On Tue, Feb 28, 2017 at 10:24:44PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 02:10:12PM +0100, Peter Zijlstra wrote:

> > > +/* For easy access to xhlock */
> > > +#define xhlock(t, i)		((t)->xhlocks + (i))
> > > +#define xhlock_prev(t, l)	xhlock(t, idx_prev((l) - (t)->xhlocks))
> > > +#define xhlock_curr(t)		xhlock(t, idx(t))
> > 
> > So these result in an xhlock pointer
> > 
> > > +#define xhlock_incr(t)		({idx(t) = idx_next(idx(t));})
> > 
> > This does not; which is confusing seeing how they share the same
> > namespace; also incr is weird.
> 
> OK.. Could you suggest a better name? xhlock_adv()? advance_xhlock()?
> And.. replace it with a function?

How about doing: xhlocks_idx++ ? That is, keep all the indexes as
regular u32 and only reduce the space when using them as index.

Also, I would write the loop:

> +static int commit_xhlocks(struct cross_lock *xlock)
> +{
> +     struct task_struct *curr = current;
> +     struct hist_lock *xhlock_c = xhlock_curr(curr);
> +     struct hist_lock *xhlock = xhlock_c;
> +
> +     do {
> +             xhlock = xhlock_prev(curr, xhlock);
> +
> +             if (!xhlock_used(xhlock))
> +                     break;
> +
> +             if (before(xhlock->hlock.gen_id, xlock->hlock.gen_id))
> +                     break;
> +
> +             if (same_context_xhlock(xhlock) &&
> +                 before(xhlock->prev_gen_id, xlock->hlock.gen_id) &&
> +                 !commit_xhlock(xlock, xhlock))
> +                     return 0;
> +     } while (xhlock_c != xhlock);
> +
> +     return 1;
> +}

like:

#define xhlock(i)	current->xhlocks[i % MAX_XHLOCKS_NR]

	for (i = 0; i < MAX_XHLOCKS_NR; i++) {
		xhlock = xhlock(curr->xhlock_idx - i);

		/* ... */
	}

That avoids that horrible xhlock_prev() thing.

[toc] | [prev] | [next] | [standalone]

#1589982

On Tue, Feb 28, 2017 at 07:29:02PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 28, 2017 at 10:24:44PM +0900, Byungchul Park wrote:
> > On Tue, Feb 28, 2017 at 02:10:12PM +0100, Peter Zijlstra wrote:
> 
> > > > +/* For easy access to xhlock */
> > > > +#define xhlock(t, i)		((t)->xhlocks + (i))
> > > > +#define xhlock_prev(t, l)	xhlock(t, idx_prev((l) - (t)->xhlocks))
> > > > +#define xhlock_curr(t)		xhlock(t, idx(t))
> > > 
> > > So these result in an xhlock pointer
> > > 
> > > > +#define xhlock_incr(t)		({idx(t) = idx_next(idx(t));})
> > > 
> > > This does not; which is confusing seeing how they share the same
> > > namespace; also incr is weird.
> > 
> > OK.. Could you suggest a better name? xhlock_adv()? advance_xhlock()?
> > And.. replace it with a function?
> 
> How about doing: xhlocks_idx++ ? That is, keep all the indexes as
> regular u32 and only reduce the space when using them as index.

OK.

> 
> Also, I would write the loop:
> 
> > +static int commit_xhlocks(struct cross_lock *xlock)
> > +{
> > +     struct task_struct *curr = current;
> > +     struct hist_lock *xhlock_c = xhlock_curr(curr);
> > +     struct hist_lock *xhlock = xhlock_c;
> > +
> > +     do {
> > +             xhlock = xhlock_prev(curr, xhlock);
> > +
> > +             if (!xhlock_used(xhlock))
> > +                     break;
> > +
> > +             if (before(xhlock->hlock.gen_id, xlock->hlock.gen_id))
> > +                     break;
> > +
> > +             if (same_context_xhlock(xhlock) &&
> > +                 before(xhlock->prev_gen_id, xlock->hlock.gen_id) &&
> > +                 !commit_xhlock(xlock, xhlock))
> > +                     return 0;
> > +     } while (xhlock_c != xhlock);
> > +
> > +     return 1;
> > +}
> 
> like:
> 
> #define xhlock(i)	current->xhlocks[i % MAX_XHLOCKS_NR]
> 
> 	for (i = 0; i < MAX_XHLOCKS_NR; i++) {
> 		xhlock = xhlock(curr->xhlock_idx - i);
> 
> 		/* ... */
> 	}
> 
> That avoids that horrible xhlock_prev() thing.

Right. I decided to force MAX_XHLOCKS_NR to be power of 2 and everything
became easy. Thank you very much.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web