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

Groups > linux.kernel > #1626151 > unrolled thread

Re: [PATCH v6 05/15] 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 v6 05/15] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-04-19 16:30 +0200
    Re: [PATCH v6 05/15] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-04-24 07:20 +0200
      Re: [PATCH v6 05/15] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-04-24 12:20 +0200

#1626151 — Re: [PATCH v6 05/15] lockdep: Implement crossrelease feature

On Tue, Mar 14, 2017 at 05:18:52PM +0900, Byungchul Park wrote:
> +struct hist_lock {
> +	/*
> +	 * Each work of workqueue might run in a different context,
> +	 * thanks to concurrency support of workqueue. So we have to
> +	 * distinguish each work to avoid false positive.
> +	 */
> +	unsigned int		work_id;
>  };

> @@ -1749,6 +1749,14 @@ struct task_struct {
>  	struct held_lock held_locks[MAX_LOCK_DEPTH];
>  	gfp_t lockdep_reclaim_gfp;
>  #endif
> +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> +#define MAX_XHLOCKS_NR 64UL
> +	struct hist_lock *xhlocks; /* Crossrelease history locks */
> +	unsigned int xhlock_idx;
> +	unsigned int xhlock_idx_soft; /* For backing up at softirq entry */
> +	unsigned int xhlock_idx_hard; /* For backing up at hardirq entry */
> +	unsigned int work_id;
> +#endif
>  #ifdef CONFIG_UBSAN
>  	unsigned int in_ubsan;
>  #endif

> +/*
> + * Crossrelease needs to distinguish each work of workqueues.
> + * Caller is supposed to be a worker.
> + */
> +void crossrelease_work_start(void)
> +{
> +	if (current->xhlocks)
> +		current->work_id++;
> +}

> +/*
> + * Only access local task's data, so irq disable is only required.
> + */
> +static int same_context_xhlock(struct hist_lock *xhlock)
> +{
> +	struct task_struct *curr = current;
> +
> +	/* In the case of hardirq context */
> +	if (curr->hardirq_context) {
> +		if (xhlock->hlock.irq_context & 2) /* 2: bitmask for hardirq */
> +			return 1;
> +	/* In the case of softriq context */
> +	} else if (curr->softirq_context) {
> +		if (xhlock->hlock.irq_context & 1) /* 1: bitmask for softirq */
> +			return 1;
> +	/* In the case of process context */
> +	} else {
> +		if (xhlock->work_id == curr->work_id)
> +			return 1;
> +	}
> +	return 0;
> +}

I still don't like work_id; it doesn't have anything to do with
workqueues per se, other than the fact that they end up using it.

It's a history generation id; touching it completely invalidates our
history. Workqueues need this because they run independent work from the
same context.

But the same is true for other sites. Last time I suggested
lockdep_assert_empty() to denote all suck places (and note we already
have lockdep_sys_exit() that hooks into the return to user path).

[toc] | [next] | [standalone]

#1629198

On Wed, Apr 19, 2017 at 04:25:03PM +0200, Peter Zijlstra wrote:
> On Tue, Mar 14, 2017 at 05:18:52PM +0900, Byungchul Park wrote:
> > +struct hist_lock {
> > +	/*
> > +	 * Each work of workqueue might run in a different context,
> > +	 * thanks to concurrency support of workqueue. So we have to
> > +	 * distinguish each work to avoid false positive.
> > +	 */
> > +	unsigned int		work_id;
> >  };
> 
> > @@ -1749,6 +1749,14 @@ struct task_struct {
> >  	struct held_lock held_locks[MAX_LOCK_DEPTH];
> >  	gfp_t lockdep_reclaim_gfp;
> >  #endif
> > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > +#define MAX_XHLOCKS_NR 64UL
> > +	struct hist_lock *xhlocks; /* Crossrelease history locks */
> > +	unsigned int xhlock_idx;
> > +	unsigned int xhlock_idx_soft; /* For backing up at softirq entry */
> > +	unsigned int xhlock_idx_hard; /* For backing up at hardirq entry */
> > +	unsigned int work_id;
> > +#endif
> >  #ifdef CONFIG_UBSAN
> >  	unsigned int in_ubsan;
> >  #endif
> 
> > +/*
> > + * Crossrelease needs to distinguish each work of workqueues.
> > + * Caller is supposed to be a worker.
> > + */
> > +void crossrelease_work_start(void)
> > +{
> > +	if (current->xhlocks)
> > +		current->work_id++;
> > +}
> 
> > +/*
> > + * Only access local task's data, so irq disable is only required.
> > + */
> > +static int same_context_xhlock(struct hist_lock *xhlock)
> > +{
> > +	struct task_struct *curr = current;
> > +
> > +	/* In the case of hardirq context */
> > +	if (curr->hardirq_context) {
> > +		if (xhlock->hlock.irq_context & 2) /* 2: bitmask for hardirq */
> > +			return 1;
> > +	/* In the case of softriq context */
> > +	} else if (curr->softirq_context) {
> > +		if (xhlock->hlock.irq_context & 1) /* 1: bitmask for softirq */
> > +			return 1;
> > +	/* In the case of process context */
> > +	} else {
> > +		if (xhlock->work_id == curr->work_id)
> > +			return 1;
> > +	}
> > +	return 0;
> > +}
> 
> I still don't like work_id; it doesn't have anything to do with
> workqueues per se, other than the fact that they end up using it.
> 
> It's a history generation id; touching it completely invalidates our
> history. Workqueues need this because they run independent work from the
> same context.
> 
> But the same is true for other sites. Last time I suggested
> lockdep_assert_empty() to denote all suck places (and note we already
> have lockdep_sys_exit() that hooks into the return to user path).

I'm sorry but I don't understand what you intend. It would be appriciated
if you explain more.

You might know why I introduced the 'work_id'.. Is there any alternative?

Thank you.

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

#1629392

On Mon, Apr 24, 2017 at 02:11:02PM +0900, Byungchul Park wrote:
> On Wed, Apr 19, 2017 at 04:25:03PM +0200, Peter Zijlstra wrote:

> > I still don't like work_id; it doesn't have anything to do with
> > workqueues per se, other than the fact that they end up using it.
> > 
> > It's a history generation id; touching it completely invalidates our
> > history. Workqueues need this because they run independent work from the
> > same context.
> > 
> > But the same is true for other sites. Last time I suggested
> > lockdep_assert_empty() to denote all suck places (and note we already
> > have lockdep_sys_exit() that hooks into the return to user path).
> 
> I'm sorry but I don't understand what you intend. It would be appriciated
> if you explain more.
> 
> You might know why I introduced the 'work_id'.. Is there any alternative?

My complaint is mostly about naming.. and "hist_gen_id" might be a
better name.

But let me explain.


The reason workqueues need this is because the lock history for each
'work' are independent. The locks of Work-B do not depend on the locks
of the preceding Work-A, because the completion of Work-B is not
dependent on those locks.

But this is true for many things; pretty much all kthreads fall in this
pattern, where they have an 'idle' state and future completions do not
depend on past completions. Its just that since they all have the 'same'
form -- the kthread does the same over and over -- it doesn't matter
much.

The same is true for system-calls, once a system call is complete (we've
returned to userspace) the next system call does not depend on the lock
history of the previous one.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web