Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > + /*
> > +* 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.
> > +*
> > +* TODO: We can also add dependencies between two acquisitions
> > +* of different work_id, if they don't cause a sleep so make
> > +* the worker stalled.
> > +*/
> > + unsigned intwork_id;
>
> > +/*
> > + * 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++;
> > +}
>
> So what you're trying to do with that 'work_id' thing is basically wipe
> the entire history when we're at the bottom of a context.
>
> Which is a useful operation, but should arguably also be done on the
> return to userspace path. Any historical lock from before the current
> syscall is irrelevant.
Yes. I agree with that each syscall is irrelevant to others. But should
we do that? Is it a problem if we don't distinguish between each syscall
context in crossrelease check? IMHO, it's ok to perform commit if the
target crosslock can be seen when releasing it. No? (As you know, in case
of work queue, each work should be distinguished. See the comment in code.)
If we have to do it.. do you mean to modify architecture code for syscall
entry? Or is there architecture independent code where we can be aware of
the entry? It would be appriciated if you answer them.
>
> (And we should not be returning to userspace with locks held anyway --
> lockdep already has a check for that).
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > + /*
> > +* 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.
> > +*
> > +* TODO: We can also add dependencies between two acquisitions
> > +* of different work_id, if they don't cause a sleep so make
> > +* the worker stalled.
> > +*/
> > + unsigned intwork_id;
>
> > +/*
> > + * 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++;
> > +}
>
> So what you're trying to do with that 'work_id' thing is basically wipe
> the entire history when we're at the bottom of a context.
>
> Which is a useful operation, but should arguably also be done on the
> return to userspace path. Any historical lock from before the current
> syscall is irrelevant.
Yes. I agree with that each syscall is irrelevant to others. But should
we do that? Is it a problem if we don't distinguish between each syscall
context in crossrelease check? IMHO, it's ok to perform commit if the
target crosslock can be seen when releasing it. No? (As you know, in case
of work queue, each work should be distinguished. See the comment in code.)
If we have to do it.. do you mean to modify architecture code for syscall
entry? Or is there architecture independent code where we can be aware of
the entry? It would be appriciated if you answer them.
>
> (And we should not be returning to userspace with locks held anyway --
> lockdep already has a check for that).
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Sun, Mar 05, 2017 at 12:08:45PM +0900, Byungchul Park wrote: > On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote: > > > > Now the problem with the above condition is that it makes reports > > harder to decipher, because by avoiding adding redundant links to our > > graph we loose a possible shorter path. > > Let's see the following example: > >A -> B -> C > >where A, B and C are typical lock class. > > Assume the graph above was built and operations happena in the > following order: > >CONTEXT X CONTEXT Y >- - >acquire DX > acquire A > acquire B > acquire C > > release and commit DX > >where A, B and C are typical lock class, DX is a crosslock class. > > The graph will grow as following _without_ prev_gen_id. > > -> A -> B -> C >/// >DX --- > >where A, B and C are typical lock class, DX is a crosslock class. > > The graph will grow as following _with_ prev_gen_id. > >DX -> A -> B -> C > >where A, B and C are typical lock class, DX is a crosslock class. > > You said the former is better because it has smaller cost in bfs. No, I said the former is better because when you report a DX inversion against C, A and B are not required and the report is easier to understand by _humans_. I don't particularly care about the BFS cost itself. > But it has to use _much_ more memory to keep additional nodes in > graph. Without exaggeration, every crosslock would get linked with all > locks in history locks, on commit, unless redundant. It might be > pretty more than we expect - I will check and let you know how many it > is. Is it still good? Dunno, probably not.. but it would be good to have numbers.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Sun, Mar 05, 2017 at 12:08:45PM +0900, Byungchul Park wrote: > On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote: > > > > Now the problem with the above condition is that it makes reports > > harder to decipher, because by avoiding adding redundant links to our > > graph we loose a possible shorter path. > > Let's see the following example: > >A -> B -> C > >where A, B and C are typical lock class. > > Assume the graph above was built and operations happena in the > following order: > >CONTEXT X CONTEXT Y >- - >acquire DX > acquire A > acquire B > acquire C > > release and commit DX > >where A, B and C are typical lock class, DX is a crosslock class. > > The graph will grow as following _without_ prev_gen_id. > > -> A -> B -> C >/// >DX --- > >where A, B and C are typical lock class, DX is a crosslock class. > > The graph will grow as following _with_ prev_gen_id. > >DX -> A -> B -> C > >where A, B and C are typical lock class, DX is a crosslock class. > > You said the former is better because it has smaller cost in bfs. No, I said the former is better because when you report a DX inversion against C, A and B are not required and the report is easier to understand by _humans_. I don't particularly care about the BFS cost itself. > But it has to use _much_ more memory to keep additional nodes in > graph. Without exaggeration, every crosslock would get linked with all > locks in history locks, on commit, unless redundant. It might be > pretty more than we expect - I will check and let you know how many it > is. Is it still good? Dunno, probably not.. but it would be good to have numbers.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 08:50:03AM +0900, Byungchul Park wrote: > On Thu, Mar 02, 2017 at 06:39:49AM -0800, Matthew Wilcox wrote: > > On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote: > > > From: Matthew Wilcox [mailto:wi...@infradead.org] > > > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > > > > > (And we should not be returning to userspace with locks held anyway -- > > > > > lockdep already has a check for that). > > > > > > > > Don't we return to userspace with page locks held, eg during async > > > > directio? > > > > > > Hello, > > > > > > I think that the check when returning to user with crosslocks held > > > should be an exception. Don't you think so? > > > > Oh yes. We have to keep the pages locked during reads, and we have to > > return to userspace before I/O is complete, therefore we have to return > > to userspace with pages locked. They'll be unlocked by the interrupt > > handler in page_endio(). > > Agree. > > > Speaking of which ... this feature is far too heavy for use in production > > on pages. You're almost trebling the size of struct page. Can we > > do something like make all struct pages share the same lockdep_map? > > We'd have to not complain about holding one crossdep lock and acquiring > > another one of the same type, but with millions of pages in the system, > > it must surely be creating a gargantuan graph right now? > > Um.. I will try it for page locks to work with one lockmap. That is also > what Peterz pointed out and what I worried about when implementing.. I've thought it more and it seems not to be good. We could not use subclass feature if we make page locks work with only one lockmap instance. And there are several things we have to give up, that are, things using each field in struct lockdep_map. So now, I'm not sure I should change the current implementation. What do you think about it?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 08:50:03AM +0900, Byungchul Park wrote: > On Thu, Mar 02, 2017 at 06:39:49AM -0800, Matthew Wilcox wrote: > > On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote: > > > From: Matthew Wilcox [mailto:wi...@infradead.org] > > > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > > > > > (And we should not be returning to userspace with locks held anyway -- > > > > > lockdep already has a check for that). > > > > > > > > Don't we return to userspace with page locks held, eg during async > > > > directio? > > > > > > Hello, > > > > > > I think that the check when returning to user with crosslocks held > > > should be an exception. Don't you think so? > > > > Oh yes. We have to keep the pages locked during reads, and we have to > > return to userspace before I/O is complete, therefore we have to return > > to userspace with pages locked. They'll be unlocked by the interrupt > > handler in page_endio(). > > Agree. > > > Speaking of which ... this feature is far too heavy for use in production > > on pages. You're almost trebling the size of struct page. Can we > > do something like make all struct pages share the same lockdep_map? > > We'd have to not complain about holding one crossdep lock and acquiring > > another one of the same type, but with millions of pages in the system, > > it must surely be creating a gargantuan graph right now? > > Um.. I will try it for page locks to work with one lockmap. That is also > what Peterz pointed out and what I worried about when implementing.. I've thought it more and it seems not to be good. We could not use subclass feature if we make page locks work with only one lockmap instance. And there are several things we have to give up, that are, things using each field in struct lockdep_map. So now, I'm not sure I should change the current implementation. What do you think about it?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 10:13:38AM +0100, Peter Zijlstra wrote: > On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote: > > Two boots + a make defconfig, the first didn't have the redundant bit > in, the second did (full diff below still includes the reclaim rework, > because that was still in that kernel and I forgot to reset the tree). > > > lock-classes: 1168 1169 [max: 8191] > direct dependencies: 7688 5812 [max: 32768] > indirect dependencies: 25492 25937 > all direct dependencies:220113 217512 > dependency chains:9005 9008 [max: 65536] > dependency chain hlocks: 34450 34366 [max: 327680] > in-hardirq chains: 55 51 > in-softirq chains: 371378 > in-process chains:8579 8579 > stack-trace entries:108073 88474 [max: 524288] > combined max dependencies: 178738560 169094640 > > max locking depth: 15 15 > max bfs queue depth: 320329 > > cyclic checks:9123 9190 > > redundant checks:5046 > redundant links: 1828 > > find-mask forwards checks:2564 2599 > find-mask backwards checks: 39521 39789 > > > So it saves nearly 2k links and a fair chunk of stack-trace entries, but It's as we expect. > as expected, makes no real difference on the indirect dependencies. It looks that the indirect dependencies increased to me. This result is also somewhat anticipated. > At the same time, you see the max BFS depth increase, which is also Yes. The depth should increase. > expected, although it could easily be boot variance -- these numbers are > not entirely stable between boots. > > Could you run something similar? Or I'll take a look on your next spin > of the patches. I will check same thing you did and let you know the result at next spin.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 10:13:38AM +0100, Peter Zijlstra wrote: > On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote: > > Two boots + a make defconfig, the first didn't have the redundant bit > in, the second did (full diff below still includes the reclaim rework, > because that was still in that kernel and I forgot to reset the tree). > > > lock-classes: 1168 1169 [max: 8191] > direct dependencies: 7688 5812 [max: 32768] > indirect dependencies: 25492 25937 > all direct dependencies:220113 217512 > dependency chains:9005 9008 [max: 65536] > dependency chain hlocks: 34450 34366 [max: 327680] > in-hardirq chains: 55 51 > in-softirq chains: 371378 > in-process chains:8579 8579 > stack-trace entries:108073 88474 [max: 524288] > combined max dependencies: 178738560 169094640 > > max locking depth: 15 15 > max bfs queue depth: 320329 > > cyclic checks:9123 9190 > > redundant checks:5046 > redundant links: 1828 > > find-mask forwards checks:2564 2599 > find-mask backwards checks: 39521 39789 > > > So it saves nearly 2k links and a fair chunk of stack-trace entries, but It's as we expect. > as expected, makes no real difference on the indirect dependencies. It looks that the indirect dependencies increased to me. This result is also somewhat anticipated. > At the same time, you see the max BFS depth increase, which is also Yes. The depth should increase. > expected, although it could easily be boot variance -- these numbers are > not entirely stable between boots. > > Could you run something similar? Or I'll take a look on your next spin > of the patches. I will check same thing you did and let you know the result at next spin.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:
> On Fri, Mar 03, 2017 at 09:17:37AM +0900, Byungchul Park wrote:
> > On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
>
> > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > index a95e5d1..7baea89 100644
> > > --- a/kernel/locking/lockdep.c
> > > +++ b/kernel/locking/lockdep.c
> > > @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct
> > > held_lock *prev,
> > > }
> > > }
> > >
> > > + /*
> > > + * Is the -> redundant?
> > > + */
> > > + this.class = hlock_class(prev);
> > > + this.parent = NULL;
> > > + ret = check_noncircular(, hlock_class(next), _entry);
> > > + if (!ret) /* exists, redundant */
> > > + return 2;
> > > + if (ret < 0)
> > > + return print_bfs_bug(ret);
> > > +
> > > if (!*stack_saved) {
> > > if (!save_trace())
> > > return 0;
> >
> > This whoud be very nice if you allow to add this code. However, prev_gen_id
> > thingy is still useful, the code above can achieve it though. Agree?
>
> So my goal was to avoid prev_gen_id, and yes I think the above does
> that.
>
> Now the problem with the above condition is that it makes reports
> harder to decipher, because by avoiding adding redundant links to our
> graph we loose a possible shorter path.
Let's see the following example:
A -> B -> C
where A, B and C are typical lock class.
Assume the graph above was built and operations happena in the
following order:
CONTEXT XCONTEXT Y
--
acquire DX
acquire A
acquire B
acquire C
release and commit DX
where A, B and C are typical lock class, DX is a crosslock class.
The graph will grow as following _without_ prev_gen_id.
-> A -> B -> C
///
DX ---
where A, B and C are typical lock class, DX is a crosslock class.
The graph will grow as following _with_ prev_gen_id.
DX -> A -> B -> C
where A, B and C are typical lock class, DX is a crosslock class.
You said the former is better because it has smaller cost in bfs. But it
has to use _much_ more memory to keep additional nodes in graph. Without
exaggeration, every crosslock would get linked with all locks in history
locks, on commit, unless redundant. It might be pretty more than we
expect - I will check and let you know how many it is. Is it still good?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:
> On Fri, Mar 03, 2017 at 09:17:37AM +0900, Byungchul Park wrote:
> > On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
>
> > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > index a95e5d1..7baea89 100644
> > > --- a/kernel/locking/lockdep.c
> > > +++ b/kernel/locking/lockdep.c
> > > @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct
> > > held_lock *prev,
> > > }
> > > }
> > >
> > > + /*
> > > + * Is the -> redundant?
> > > + */
> > > + this.class = hlock_class(prev);
> > > + this.parent = NULL;
> > > + ret = check_noncircular(, hlock_class(next), _entry);
> > > + if (!ret) /* exists, redundant */
> > > + return 2;
> > > + if (ret < 0)
> > > + return print_bfs_bug(ret);
> > > +
> > > if (!*stack_saved) {
> > > if (!save_trace())
> > > return 0;
> >
> > This whoud be very nice if you allow to add this code. However, prev_gen_id
> > thingy is still useful, the code above can achieve it though. Agree?
>
> So my goal was to avoid prev_gen_id, and yes I think the above does
> that.
>
> Now the problem with the above condition is that it makes reports
> harder to decipher, because by avoiding adding redundant links to our
> graph we loose a possible shorter path.
Let's see the following example:
A -> B -> C
where A, B and C are typical lock class.
Assume the graph above was built and operations happena in the
following order:
CONTEXT XCONTEXT Y
--
acquire DX
acquire A
acquire B
acquire C
release and commit DX
where A, B and C are typical lock class, DX is a crosslock class.
The graph will grow as following _without_ prev_gen_id.
-> A -> B -> C
///
DX ---
where A, B and C are typical lock class, DX is a crosslock class.
The graph will grow as following _with_ prev_gen_id.
DX -> A -> B -> C
where A, B and C are typical lock class, DX is a crosslock class.
You said the former is better because it has smaller cost in bfs. But it
has to use _much_ more memory to keep additional nodes in graph. Without
exaggeration, every crosslock would get linked with all locks in history
locks, on commit, unless redundant. It might be pretty more than we
expect - I will check and let you know how many it is. Is it still good?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:
> That said; I'd be fairly interested in numbers on how many links this
> avoids, I'll go make a check_redundant() version of the above and put a
> proper counter in so I can see what it does for a regular boot etc..
Two boots + a make defconfig, the first didn't have the redundant bit
in, the second did (full diff below still includes the reclaim rework,
because that was still in that kernel and I forgot to reset the tree).
lock-classes: 1168 1169 [max: 8191]
direct dependencies: 7688 5812 [max: 32768]
indirect dependencies: 25492 25937
all direct dependencies:220113 217512
dependency chains:9005 9008 [max: 65536]
dependency chain hlocks: 34450 34366 [max: 327680]
in-hardirq chains: 55 51
in-softirq chains: 371378
in-process chains:8579 8579
stack-trace entries:108073 88474 [max: 524288]
combined max dependencies: 178738560 169094640
max locking depth: 15 15
max bfs queue depth: 320329
cyclic checks:9123 9190
redundant checks:5046
redundant links: 1828
find-mask forwards checks:2564 2599
find-mask backwards checks: 39521 39789
So it saves nearly 2k links and a fair chunk of stack-trace entries, but
as expected, makes no real difference on the indirect dependencies.
At the same time, you see the max BFS depth increase, which is also
expected, although it could easily be boot variance -- these numbers are
not entirely stable between boots.
Could you run something similar? Or I'll take a look on your next spin
of the patches.
---
include/linux/lockdep.h| 11 +---
include/linux/sched.h | 1 -
kernel/locking/lockdep.c | 114 +
kernel/locking/lockdep_internals.h | 2 +
kernel/locking/lockdep_proc.c | 4 ++
kernel/locking/lockdep_states.h| 1 -
mm/internal.h | 40 +
mm/page_alloc.c| 13 -
mm/slab.h | 7 ++-
mm/slob.c | 8 ++-
mm/vmscan.c| 13 ++---
11 files changed, 104 insertions(+), 110 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1e327bb..6ba1a65 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -29,7 +29,7 @@ extern int lock_stat;
* We'd rather not expose kernel/lockdep_states.h this wide, but we do need
* the total number of states... :-(
*/
-#define XXX_LOCK_USAGE_STATES (1+3*4)
+#define XXX_LOCK_USAGE_STATES (1+2*4)
/*
* NR_LOCKDEP_CACHING_CLASSES ... Number of classes
@@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map
*lock,
lock_set_class(lock, lock->name, lock->key, subclass, ip);
}
-extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
-extern void lockdep_clear_current_reclaim_state(void);
-extern void lockdep_trace_alloc(gfp_t mask);
-
struct pin_cookie { unsigned int val; };
#define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
@@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map
*lock);
extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
-# define INIT_LOCKDEP .lockdep_recursion = 0,
.lockdep_reclaim_gfp = 0,
+# define INIT_LOCKDEP .lockdep_recursion = 0,
#define lockdep_depth(tsk) (debug_locks ? (tsk)->lockdep_depth : 0)
@@ -413,9 +409,6 @@ static inline void lockdep_on(void)
# define lock_release(l, n, i) do { } while (0)
# define lock_set_class(l, n, k, s, i) do { } while (0)
# define lock_set_subclass(l, s, i)do { } while (0)
-# define lockdep_set_current_reclaim_state(g) do { } while (0)
-# define lockdep_clear_current_reclaim_state() do { } while (0)
-# define lockdep_trace_alloc(g)do { } while (0)
# define lockdep_info()do { } while (0)
# define lockdep_init_map(lock, name, key, sub) \
do { (void)(name); (void)(key); } while (0)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d67eee8..0fa8a8f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -806,7 +806,6 @@ struct task_struct {
int lockdep_depth;
unsigned intlockdep_recursion;
struct held_lockheld_locks[MAX_LOCK_DEPTH];
- gfp_t
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote:
> That said; I'd be fairly interested in numbers on how many links this
> avoids, I'll go make a check_redundant() version of the above and put a
> proper counter in so I can see what it does for a regular boot etc..
Two boots + a make defconfig, the first didn't have the redundant bit
in, the second did (full diff below still includes the reclaim rework,
because that was still in that kernel and I forgot to reset the tree).
lock-classes: 1168 1169 [max: 8191]
direct dependencies: 7688 5812 [max: 32768]
indirect dependencies: 25492 25937
all direct dependencies:220113 217512
dependency chains:9005 9008 [max: 65536]
dependency chain hlocks: 34450 34366 [max: 327680]
in-hardirq chains: 55 51
in-softirq chains: 371378
in-process chains:8579 8579
stack-trace entries:108073 88474 [max: 524288]
combined max dependencies: 178738560 169094640
max locking depth: 15 15
max bfs queue depth: 320329
cyclic checks:9123 9190
redundant checks:5046
redundant links: 1828
find-mask forwards checks:2564 2599
find-mask backwards checks: 39521 39789
So it saves nearly 2k links and a fair chunk of stack-trace entries, but
as expected, makes no real difference on the indirect dependencies.
At the same time, you see the max BFS depth increase, which is also
expected, although it could easily be boot variance -- these numbers are
not entirely stable between boots.
Could you run something similar? Or I'll take a look on your next spin
of the patches.
---
include/linux/lockdep.h| 11 +---
include/linux/sched.h | 1 -
kernel/locking/lockdep.c | 114 +
kernel/locking/lockdep_internals.h | 2 +
kernel/locking/lockdep_proc.c | 4 ++
kernel/locking/lockdep_states.h| 1 -
mm/internal.h | 40 +
mm/page_alloc.c| 13 -
mm/slab.h | 7 ++-
mm/slob.c | 8 ++-
mm/vmscan.c| 13 ++---
11 files changed, 104 insertions(+), 110 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1e327bb..6ba1a65 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -29,7 +29,7 @@ extern int lock_stat;
* We'd rather not expose kernel/lockdep_states.h this wide, but we do need
* the total number of states... :-(
*/
-#define XXX_LOCK_USAGE_STATES (1+3*4)
+#define XXX_LOCK_USAGE_STATES (1+2*4)
/*
* NR_LOCKDEP_CACHING_CLASSES ... Number of classes
@@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map
*lock,
lock_set_class(lock, lock->name, lock->key, subclass, ip);
}
-extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
-extern void lockdep_clear_current_reclaim_state(void);
-extern void lockdep_trace_alloc(gfp_t mask);
-
struct pin_cookie { unsigned int val; };
#define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
@@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map
*lock);
extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
-# define INIT_LOCKDEP .lockdep_recursion = 0,
.lockdep_reclaim_gfp = 0,
+# define INIT_LOCKDEP .lockdep_recursion = 0,
#define lockdep_depth(tsk) (debug_locks ? (tsk)->lockdep_depth : 0)
@@ -413,9 +409,6 @@ static inline void lockdep_on(void)
# define lock_release(l, n, i) do { } while (0)
# define lock_set_class(l, n, k, s, i) do { } while (0)
# define lock_set_subclass(l, s, i)do { } while (0)
-# define lockdep_set_current_reclaim_state(g) do { } while (0)
-# define lockdep_clear_current_reclaim_state() do { } while (0)
-# define lockdep_trace_alloc(g)do { } while (0)
# define lockdep_info()do { } while (0)
# define lockdep_init_map(lock, name, key, sub) \
do { (void)(name); (void)(key); } while (0)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d67eee8..0fa8a8f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -806,7 +806,6 @@ struct task_struct {
int lockdep_depth;
unsigned intlockdep_recursion;
struct held_lockheld_locks[MAX_LOCK_DEPTH];
- gfp_t
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 10:13:38AM +0100, Peter Zijlstra wrote: > On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote: > > > That said; I'd be fairly interested in numbers on how many links this > > avoids, I'll go make a check_redundant() version of the above and put a > > proper counter in so I can see what it does for a regular boot etc.. > > Two boots + a make defconfig, the first didn't have the redundant bit > in, the second did (full diff below still includes the reclaim rework, > because that was still in that kernel and I forgot to reset the tree). > > > lock-classes: 1168 1169 [max: 8191] > direct dependencies: 7688 5812 [max: 32768] > indirect dependencies: 25492 25937 > all direct dependencies:220113 217512 > dependency chains:9005 9008 [max: 65536] > dependency chain hlocks: 34450 34366 [max: 327680] > in-hardirq chains: 55 51 > in-softirq chains: 371378 > in-process chains:8579 8579 > stack-trace entries:108073 88474 [max: 524288] > combined max dependencies: 178738560 169094640 > > max locking depth: 15 15 > max bfs queue depth: 320329 > > cyclic checks:9123 9190 > > redundant checks:5046 > redundant links: 1828 > > find-mask forwards checks:2564 2599 > find-mask backwards checks: 39521 39789 > OK, last email, I promise, then I'll go bury myself in futexes. find-mask forwards checks:2999 find-mask backwards checks: 56134 Is with a clean kernel, which shows how many __bfs() calls we save by doing away with that RECLAIM state. OTOH: lock-classes: 1167 [max: 8191] direct dependencies: 7254 [max: 32768] indirect dependencies: 23763 all direct dependencies:219093 Shows that the added reclaim class isn't entirely free either ;-)
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 10:13:38AM +0100, Peter Zijlstra wrote: > On Fri, Mar 03, 2017 at 09:14:16AM +0100, Peter Zijlstra wrote: > > > That said; I'd be fairly interested in numbers on how many links this > > avoids, I'll go make a check_redundant() version of the above and put a > > proper counter in so I can see what it does for a regular boot etc.. > > Two boots + a make defconfig, the first didn't have the redundant bit > in, the second did (full diff below still includes the reclaim rework, > because that was still in that kernel and I forgot to reset the tree). > > > lock-classes: 1168 1169 [max: 8191] > direct dependencies: 7688 5812 [max: 32768] > indirect dependencies: 25492 25937 > all direct dependencies:220113 217512 > dependency chains:9005 9008 [max: 65536] > dependency chain hlocks: 34450 34366 [max: 327680] > in-hardirq chains: 55 51 > in-softirq chains: 371378 > in-process chains:8579 8579 > stack-trace entries:108073 88474 [max: 524288] > combined max dependencies: 178738560 169094640 > > max locking depth: 15 15 > max bfs queue depth: 320329 > > cyclic checks:9123 9190 > > redundant checks:5046 > redundant links: 1828 > > find-mask forwards checks:2564 2599 > find-mask backwards checks: 39521 39789 > OK, last email, I promise, then I'll go bury myself in futexes. find-mask forwards checks:2999 find-mask backwards checks: 56134 Is with a clean kernel, which shows how many __bfs() calls we save by doing away with that RECLAIM state. OTOH: lock-classes: 1167 [max: 8191] direct dependencies: 7254 [max: 32768] indirect dependencies: 23763 all direct dependencies:219093 Shows that the added reclaim class isn't entirely free either ;-)
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 09:17:37AM +0900, Byungchul Park wrote:
> On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index a95e5d1..7baea89 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct
> > held_lock *prev,
> > }
> > }
> >
> > + /*
> > +* Is the -> redundant?
> > +*/
> > + this.class = hlock_class(prev);
> > + this.parent = NULL;
> > + ret = check_noncircular(, hlock_class(next), _entry);
> > + if (!ret) /* exists, redundant */
> > + return 2;
> > + if (ret < 0)
> > + return print_bfs_bug(ret);
> > +
> > if (!*stack_saved) {
> > if (!save_trace())
> > return 0;
>
> This whoud be very nice if you allow to add this code. However, prev_gen_id
> thingy is still useful, the code above can achieve it though. Agree?
So my goal was to avoid prev_gen_id, and yes I think the above does
that.
Now the problem with the above condition is that it makes reports
harder to decipher, because by avoiding adding redundant links to our
graph we loose a possible shorter path.
So while for correctness sake it doesn't matter, it is irrelevant how
long the cycle is after all, all that matters is that there is a cycle.
But the humans on the receiving end tend to like shorter cycles.
And I think the same is true for crossrelease, avoiding redundant links
increases cycle length.
(And remember, BFS will otherwise find the shortest cycle.)
That said; I'd be fairly interested in numbers on how many links this
avoids, I'll go make a check_redundant() version of the above and put a
proper counter in so I can see what it does for a regular boot etc..
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Fri, Mar 03, 2017 at 09:17:37AM +0900, Byungchul Park wrote:
> On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index a95e5d1..7baea89 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct
> > held_lock *prev,
> > }
> > }
> >
> > + /*
> > +* Is the -> redundant?
> > +*/
> > + this.class = hlock_class(prev);
> > + this.parent = NULL;
> > + ret = check_noncircular(, hlock_class(next), _entry);
> > + if (!ret) /* exists, redundant */
> > + return 2;
> > + if (ret < 0)
> > + return print_bfs_bug(ret);
> > +
> > if (!*stack_saved) {
> > if (!save_trace())
> > return 0;
>
> This whoud be very nice if you allow to add this code. However, prev_gen_id
> thingy is still useful, the code above can achieve it though. Agree?
So my goal was to avoid prev_gen_id, and yes I think the above does
that.
Now the problem with the above condition is that it makes reports
harder to decipher, because by avoiding adding redundant links to our
graph we loose a possible shorter path.
So while for correctness sake it doesn't matter, it is irrelevant how
long the cycle is after all, all that matters is that there is a cycle.
But the humans on the receiving end tend to like shorter cycles.
And I think the same is true for crossrelease, avoiding redundant links
increases cycle length.
(And remember, BFS will otherwise find the shortest cycle.)
That said; I'd be fairly interested in numbers on how many links this
avoids, I'll go make a check_redundant() version of the above and put a
proper counter in so I can see what it does for a regular boot etc..
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> [*] A while ago someone, and I cannot find the email just now, asked if
> we could not implement the RECLAIM_FS inversion stuff with a 'fake' lock
It looks interesting to me.
> like we use for other things like workqueues etc. I think this should be
> possible which allows reducing the 'irq' states and will reduce the
> amount of __bfs() lookups we do.
>
> Removing the 1 IRQ state, would result in 4 less __bfs() walks if I'm
> not mistaken, more than making up for the 1 we'd have to add to detect
> redundant links.
OK.
Thanks,
Byungchul
>
>
> include/linux/lockdep.h | 11 +-
> include/linux/sched.h | 1 -
> kernel/locking/lockdep.c| 87
> +
> kernel/locking/lockdep_states.h | 1 -
> mm/internal.h | 40 +++
> mm/page_alloc.c | 13 --
> mm/slab.h | 7 +++-
> mm/slob.c | 8 +++-
> mm/vmscan.c | 13 +++---
> 9 files changed, 71 insertions(+), 110 deletions(-)
>
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 1e327bb..6ba1a65 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -29,7 +29,7 @@ extern int lock_stat;
> * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
> * the total number of states... :-(
> */
> -#define XXX_LOCK_USAGE_STATES(1+3*4)
> +#define XXX_LOCK_USAGE_STATES(1+2*4)
>
> /*
> * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
> @@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map
> *lock,
> lock_set_class(lock, lock->name, lock->key, subclass, ip);
> }
>
> -extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
> -extern void lockdep_clear_current_reclaim_state(void);
> -extern void lockdep_trace_alloc(gfp_t mask);
> -
> struct pin_cookie { unsigned int val; };
>
> #define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
> @@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map
> *lock);
> extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
> extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
>
> -# define INIT_LOCKDEP.lockdep_recursion = 0,
> .lockdep_reclaim_gfp = 0,
> +# define INIT_LOCKDEP.lockdep_recursion = 0,
>
> #define lockdep_depth(tsk) (debug_locks ? (tsk)->lockdep_depth : 0)
>
> @@ -413,9 +409,6 @@ static inline void lockdep_on(void)
> # define lock_release(l, n, i) do { } while (0)
> # define lock_set_class(l, n, k, s, i) do { } while (0)
> # define lock_set_subclass(l, s, i) do { } while (0)
> -# define lockdep_set_current_reclaim_state(g)do { } while (0)
> -# define lockdep_clear_current_reclaim_state() do { } while (0)
> -# define lockdep_trace_alloc(g) do { } while (0)
> # define lockdep_info() do { } while (0)
> # define lockdep_init_map(lock, name, key, sub) \
> do { (void)(name); (void)(key); } while (0)
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d67eee8..0fa8a8f 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -806,7 +806,6 @@ struct task_struct {
> int lockdep_depth;
> unsigned intlockdep_recursion;
> struct held_lockheld_locks[MAX_LOCK_DEPTH];
> - gfp_t lockdep_reclaim_gfp;
> #endif
>
> #ifdef CONFIG_UBSAN
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index a95e5d1..1051600 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -343,14 +343,12 @@ EXPORT_SYMBOL(lockdep_on);
> #if VERBOSE
> # define HARDIRQ_VERBOSE 1
> # define SOFTIRQ_VERBOSE 1
> -# define RECLAIM_VERBOSE 1
> #else
> # define HARDIRQ_VERBOSE 0
> # define SOFTIRQ_VERBOSE 0
> -# define RECLAIM_VERBOSE 0
> #endif
>
> -#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE || RECLAIM_VERBOSE
> +#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE
> /*
> * Quick filtering for interesting events:
> */
> @@ -2553,14 +2551,6 @@ static int SOFTIRQ_verbose(struct lock_class *class)
> return 0;
> }
>
> -static int RECLAIM_FS_verbose(struct lock_class *class)
> -{
> -#if RECLAIM_VERBOSE
> - return class_filter(class);
> -#endif
> - return 0;
> -}
> -
> #define STRICT_READ_CHECKS 1
>
> static int (*state_verbose_f[])(struct lock_class *class) = {
> @@ -2856,51 +2846,6 @@ void trace_softirqs_off(unsigned long ip)
> debug_atomic_inc(redundant_softirqs_off);
> }
>
> -static void __lockdep_trace_alloc(gfp_t gfp_mask, unsigned long flags)
> -{
> - struct
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> [*] A while ago someone, and I cannot find the email just now, asked if
> we could not implement the RECLAIM_FS inversion stuff with a 'fake' lock
It looks interesting to me.
> like we use for other things like workqueues etc. I think this should be
> possible which allows reducing the 'irq' states and will reduce the
> amount of __bfs() lookups we do.
>
> Removing the 1 IRQ state, would result in 4 less __bfs() walks if I'm
> not mistaken, more than making up for the 1 we'd have to add to detect
> redundant links.
OK.
Thanks,
Byungchul
>
>
> include/linux/lockdep.h | 11 +-
> include/linux/sched.h | 1 -
> kernel/locking/lockdep.c| 87
> +
> kernel/locking/lockdep_states.h | 1 -
> mm/internal.h | 40 +++
> mm/page_alloc.c | 13 --
> mm/slab.h | 7 +++-
> mm/slob.c | 8 +++-
> mm/vmscan.c | 13 +++---
> 9 files changed, 71 insertions(+), 110 deletions(-)
>
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 1e327bb..6ba1a65 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -29,7 +29,7 @@ extern int lock_stat;
> * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
> * the total number of states... :-(
> */
> -#define XXX_LOCK_USAGE_STATES(1+3*4)
> +#define XXX_LOCK_USAGE_STATES(1+2*4)
>
> /*
> * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
> @@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map
> *lock,
> lock_set_class(lock, lock->name, lock->key, subclass, ip);
> }
>
> -extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
> -extern void lockdep_clear_current_reclaim_state(void);
> -extern void lockdep_trace_alloc(gfp_t mask);
> -
> struct pin_cookie { unsigned int val; };
>
> #define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
> @@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map
> *lock);
> extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
> extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
>
> -# define INIT_LOCKDEP.lockdep_recursion = 0,
> .lockdep_reclaim_gfp = 0,
> +# define INIT_LOCKDEP.lockdep_recursion = 0,
>
> #define lockdep_depth(tsk) (debug_locks ? (tsk)->lockdep_depth : 0)
>
> @@ -413,9 +409,6 @@ static inline void lockdep_on(void)
> # define lock_release(l, n, i) do { } while (0)
> # define lock_set_class(l, n, k, s, i) do { } while (0)
> # define lock_set_subclass(l, s, i) do { } while (0)
> -# define lockdep_set_current_reclaim_state(g)do { } while (0)
> -# define lockdep_clear_current_reclaim_state() do { } while (0)
> -# define lockdep_trace_alloc(g) do { } while (0)
> # define lockdep_info() do { } while (0)
> # define lockdep_init_map(lock, name, key, sub) \
> do { (void)(name); (void)(key); } while (0)
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d67eee8..0fa8a8f 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -806,7 +806,6 @@ struct task_struct {
> int lockdep_depth;
> unsigned intlockdep_recursion;
> struct held_lockheld_locks[MAX_LOCK_DEPTH];
> - gfp_t lockdep_reclaim_gfp;
> #endif
>
> #ifdef CONFIG_UBSAN
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index a95e5d1..1051600 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -343,14 +343,12 @@ EXPORT_SYMBOL(lockdep_on);
> #if VERBOSE
> # define HARDIRQ_VERBOSE 1
> # define SOFTIRQ_VERBOSE 1
> -# define RECLAIM_VERBOSE 1
> #else
> # define HARDIRQ_VERBOSE 0
> # define SOFTIRQ_VERBOSE 0
> -# define RECLAIM_VERBOSE 0
> #endif
>
> -#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE || RECLAIM_VERBOSE
> +#if VERBOSE || HARDIRQ_VERBOSE || SOFTIRQ_VERBOSE
> /*
> * Quick filtering for interesting events:
> */
> @@ -2553,14 +2551,6 @@ static int SOFTIRQ_verbose(struct lock_class *class)
> return 0;
> }
>
> -static int RECLAIM_FS_verbose(struct lock_class *class)
> -{
> -#if RECLAIM_VERBOSE
> - return class_filter(class);
> -#endif
> - return 0;
> -}
> -
> #define STRICT_READ_CHECKS 1
>
> static int (*state_verbose_f[])(struct lock_class *class) = {
> @@ -2856,51 +2846,6 @@ void trace_softirqs_off(unsigned long ip)
> debug_atomic_inc(redundant_softirqs_off);
> }
>
> -static void __lockdep_trace_alloc(gfp_t gfp_mask, unsigned long flags)
> -{
> - struct
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 01:28:43PM +0100, Peter Zijlstra wrote:
> > On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:
>
> > > It's an optimization, but very essential and important optimization.
>
> Since its not for correctness, please put it in a separate patch with a
> good Changelog, the below would make a good beginning of that.
OK. I will do it.
> Also, I feel, the source comments can be improved.
>
> > > in hlocks[]
> > >
> > > A gen_id (4) --+
> > > | previous gen_id
> > > B gen_id (3) <-+
> > > C gen_id (3)
> > > D gen_id (2)
> > > oldest -> E gen_id (1)
> > >
> > > in xhlocks[]
> > >
> > >^ A gen_id (4) prev_gen_id (3: B's gen id)
> > >| B gen_id (3) prev_gen_id (3: C's gen id)
> > >| C gen_id (3) prev_gen_id (2: D's gen id)
> > >| D gen_id (2) prev_gen_id (1: E's gen id)
> > >| E gen_id (1) prev_gen_id (NA)
> > >
> > > Let's consider the case that the gen id of xlock to commit is 3.
> > >
> > > In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> > > and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> > > 'B -> A' which are already generated by original lockdep.
> > >
> > > I use the prev_gen_id to avoid adding this kind of redundant
> > > dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> > > means that the previous lock in hlocks[] is able to handle the
> > > dependency on its commit stage.
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index a95e5d1..7baea89 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct
> held_lock *prev,
> }
> }
>
> + /*
> + * Is the -> redundant?
> + */
> + this.class = hlock_class(prev);
> + this.parent = NULL;
> + ret = check_noncircular(, hlock_class(next), _entry);
> + if (!ret) /* exists, redundant */
> + return 2;
> + if (ret < 0)
> + return print_bfs_bug(ret);
> +
> if (!*stack_saved) {
> if (!save_trace())
> return 0;
This whoud be very nice if you allow to add this code. However, prev_gen_id
thingy is still useful, the code above can achieve it though. Agree?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 02:40:31PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 01:28:43PM +0100, Peter Zijlstra wrote:
> > On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:
>
> > > It's an optimization, but very essential and important optimization.
>
> Since its not for correctness, please put it in a separate patch with a
> good Changelog, the below would make a good beginning of that.
OK. I will do it.
> Also, I feel, the source comments can be improved.
>
> > > in hlocks[]
> > >
> > > A gen_id (4) --+
> > > | previous gen_id
> > > B gen_id (3) <-+
> > > C gen_id (3)
> > > D gen_id (2)
> > > oldest -> E gen_id (1)
> > >
> > > in xhlocks[]
> > >
> > >^ A gen_id (4) prev_gen_id (3: B's gen id)
> > >| B gen_id (3) prev_gen_id (3: C's gen id)
> > >| C gen_id (3) prev_gen_id (2: D's gen id)
> > >| D gen_id (2) prev_gen_id (1: E's gen id)
> > >| E gen_id (1) prev_gen_id (NA)
> > >
> > > Let's consider the case that the gen id of xlock to commit is 3.
> > >
> > > In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> > > and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> > > 'B -> A' which are already generated by original lockdep.
> > >
> > > I use the prev_gen_id to avoid adding this kind of redundant
> > > dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> > > means that the previous lock in hlocks[] is able to handle the
> > > dependency on its commit stage.
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index a95e5d1..7baea89 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct
> held_lock *prev,
> }
> }
>
> + /*
> + * Is the -> redundant?
> + */
> + this.class = hlock_class(prev);
> + this.parent = NULL;
> + ret = check_noncircular(, hlock_class(next), _entry);
> + if (!ret) /* exists, redundant */
> + return 2;
> + if (ret < 0)
> + return print_bfs_bug(ret);
> +
> if (!*stack_saved) {
> if (!save_trace())
> return 0;
This whoud be very nice if you allow to add this code. However, prev_gen_id
thingy is still useful, the code above can achieve it though. Agree?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 02:41:03PM +0100, Peter Zijlstra wrote: > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index a6c8db1..7890661 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -1042,6 +1042,19 @@ config DEBUG_LOCK_ALLOC > > spin_lock_init()/mutex_init()/etc., or whether there is any lock > > held during task exit. > > > > +config LOCKDEP_CROSSRELEASE > > + bool "Lock debugging: make lockdep work for crosslocks" > > + select LOCKDEP > > + select TRACE_IRQFLAGS > > + default n > > + help > > +This makes lockdep work for crosslock which is a lock allowed to > > +be released in a different context from the acquisition context. > > +Normally a lock must be released in the context acquiring the lock. > > +However, relexing this constraint helps synchronization primitives > > +such as page locks or completions can use the lock correctness > > +detector, lockdep. > > + > > config PROVE_LOCKING > > bool "Lock debugging: prove locking correctness" > > depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT > > && LOCKDEP_SUPPORT > > > Does CROSSRELEASE && !PROVE_LOCKING make any sense? No, it does not make sense. I will fix it. Thank you.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 02:41:03PM +0100, Peter Zijlstra wrote: > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index a6c8db1..7890661 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -1042,6 +1042,19 @@ config DEBUG_LOCK_ALLOC > > spin_lock_init()/mutex_init()/etc., or whether there is any lock > > held during task exit. > > > > +config LOCKDEP_CROSSRELEASE > > + bool "Lock debugging: make lockdep work for crosslocks" > > + select LOCKDEP > > + select TRACE_IRQFLAGS > > + default n > > + help > > +This makes lockdep work for crosslock which is a lock allowed to > > +be released in a different context from the acquisition context. > > +Normally a lock must be released in the context acquiring the lock. > > +However, relexing this constraint helps synchronization primitives > > +such as page locks or completions can use the lock correctness > > +detector, lockdep. > > + > > config PROVE_LOCKING > > bool "Lock debugging: prove locking correctness" > > depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT > > && LOCKDEP_SUPPORT > > > Does CROSSRELEASE && !PROVE_LOCKING make any sense? No, it does not make sense. I will fix it. Thank you.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 06:39:49AM -0800, Matthew Wilcox wrote: > On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote: > > From: Matthew Wilcox [mailto:wi...@infradead.org] > > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > > > > (And we should not be returning to userspace with locks held anyway -- > > > > lockdep already has a check for that). > > > > > > Don't we return to userspace with page locks held, eg during async > > > directio? > > > > Hello, > > > > I think that the check when returning to user with crosslocks held > > should be an exception. Don't you think so? > > Oh yes. We have to keep the pages locked during reads, and we have to > return to userspace before I/O is complete, therefore we have to return > to userspace with pages locked. They'll be unlocked by the interrupt > handler in page_endio(). Agree. > Speaking of which ... this feature is far too heavy for use in production > on pages. You're almost trebling the size of struct page. Can we > do something like make all struct pages share the same lockdep_map? > We'd have to not complain about holding one crossdep lock and acquiring > another one of the same type, but with millions of pages in the system, > it must surely be creating a gargantuan graph right now? Um.. I will try it for page locks to work with one lockmap. That is also what Peterz pointed out and what I worried about when implementing.. Thanks for your opinion.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 06:39:49AM -0800, Matthew Wilcox wrote: > On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote: > > From: Matthew Wilcox [mailto:wi...@infradead.org] > > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > > > > (And we should not be returning to userspace with locks held anyway -- > > > > lockdep already has a check for that). > > > > > > Don't we return to userspace with page locks held, eg during async > > > directio? > > > > Hello, > > > > I think that the check when returning to user with crosslocks held > > should be an exception. Don't you think so? > > Oh yes. We have to keep the pages locked during reads, and we have to > return to userspace before I/O is complete, therefore we have to return > to userspace with pages locked. They'll be unlocked by the interrupt > handler in page_endio(). Agree. > Speaking of which ... this feature is far too heavy for use in production > on pages. You're almost trebling the size of struct page. Can we > do something like make all struct pages share the same lockdep_map? > We'd have to not complain about holding one crossdep lock and acquiring > another one of the same type, but with millions of pages in the system, > it must surely be creating a gargantuan graph right now? Um.. I will try it for page locks to work with one lockmap. That is also what Peterz pointed out and what I worried about when implementing.. Thanks for your opinion.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 01:28:43PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:
> > It's an optimization, but very essential and important optimization.
Since its not for correctness, please put it in a separate patch with a
good Changelog, the below would make a good beginning of that.
Also, I feel, the source comments can be improved.
> > in hlocks[]
> >
> > A gen_id (4) --+
> > | previous gen_id
> > B gen_id (3) <-+
> > C gen_id (3)
> > D gen_id (2)
> > oldest -> E gen_id (1)
> >
> > in xhlocks[]
> >
> >^ A gen_id (4) prev_gen_id (3: B's gen id)
> >| B gen_id (3) prev_gen_id (3: C's gen id)
> >| C gen_id (3) prev_gen_id (2: D's gen id)
> >| D gen_id (2) prev_gen_id (1: E's gen id)
> >| E gen_id (1) prev_gen_id (NA)
> >
> > Let's consider the case that the gen id of xlock to commit is 3.
> >
> > In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> > and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> > 'B -> A' which are already generated by original lockdep.
> >
> > I use the prev_gen_id to avoid adding this kind of redundant
> > dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> > means that the previous lock in hlocks[] is able to handle the
> > dependency on its commit stage.
> >
>
> Aah, I completely missed it was against held_locks.
>
> Hurm.. it feels like this is solving a problem we shouldn't be solving
> though.
>
> That is, ideally we'd already be able to (quickly) tell if a relation
> exists or not, but given how the whole chain_hash stuff is build now, it
> looks like we cannot.
>
>
> Let me think about this a bit more.
OK, so neither this nor the chain-hash completely avoid redundant
dependencies. The only way to do that is doing graph-walks for every
proposed link.
Now, we already do a ton of __bfs() walks in check_prev_add(), so one
more might not hurt too much [*].
Esp. with the chain-hash avoiding all the obvious duplicate work, this
might just work.
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index a95e5d1..7baea89 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct
held_lock *prev,
}
}
+ /*
+* Is the -> redundant?
+*/
+ this.class = hlock_class(prev);
+ this.parent = NULL;
+ ret = check_noncircular(, hlock_class(next), _entry);
+ if (!ret) /* exists, redundant */
+ return 2;
+ if (ret < 0)
+ return print_bfs_bug(ret);
+
if (!*stack_saved) {
if (!save_trace())
return 0;
[*] A while ago someone, and I cannot find the email just now, asked if
we could not implement the RECLAIM_FS inversion stuff with a 'fake' lock
like we use for other things like workqueues etc. I think this should be
possible which allows reducing the 'irq' states and will reduce the
amount of __bfs() lookups we do.
Removing the 1 IRQ state, would result in 4 less __bfs() walks if I'm
not mistaken, more than making up for the 1 we'd have to add to detect
redundant links.
include/linux/lockdep.h | 11 +-
include/linux/sched.h | 1 -
kernel/locking/lockdep.c| 87 +
kernel/locking/lockdep_states.h | 1 -
mm/internal.h | 40 +++
mm/page_alloc.c | 13 --
mm/slab.h | 7 +++-
mm/slob.c | 8 +++-
mm/vmscan.c | 13 +++---
9 files changed, 71 insertions(+), 110 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1e327bb..6ba1a65 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -29,7 +29,7 @@ extern int lock_stat;
* We'd rather not expose kernel/lockdep_states.h this wide, but we do need
* the total number of states... :-(
*/
-#define XXX_LOCK_USAGE_STATES (1+3*4)
+#define XXX_LOCK_USAGE_STATES (1+2*4)
/*
* NR_LOCKDEP_CACHING_CLASSES ... Number of classes
@@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map
*lock,
lock_set_class(lock, lock->name, lock->key, subclass, ip);
}
-extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
-extern void lockdep_clear_current_reclaim_state(void);
-extern void lockdep_trace_alloc(gfp_t mask);
-
struct pin_cookie { unsigned int val; };
#define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
@@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map
*lock);
extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
extern void lock_unpin_lock(struct lockdep_map *lock,
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 01:28:43PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:
> > It's an optimization, but very essential and important optimization.
Since its not for correctness, please put it in a separate patch with a
good Changelog, the below would make a good beginning of that.
Also, I feel, the source comments can be improved.
> > in hlocks[]
> >
> > A gen_id (4) --+
> > | previous gen_id
> > B gen_id (3) <-+
> > C gen_id (3)
> > D gen_id (2)
> > oldest -> E gen_id (1)
> >
> > in xhlocks[]
> >
> >^ A gen_id (4) prev_gen_id (3: B's gen id)
> >| B gen_id (3) prev_gen_id (3: C's gen id)
> >| C gen_id (3) prev_gen_id (2: D's gen id)
> >| D gen_id (2) prev_gen_id (1: E's gen id)
> >| E gen_id (1) prev_gen_id (NA)
> >
> > Let's consider the case that the gen id of xlock to commit is 3.
> >
> > In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> > and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> > 'B -> A' which are already generated by original lockdep.
> >
> > I use the prev_gen_id to avoid adding this kind of redundant
> > dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> > means that the previous lock in hlocks[] is able to handle the
> > dependency on its commit stage.
> >
>
> Aah, I completely missed it was against held_locks.
>
> Hurm.. it feels like this is solving a problem we shouldn't be solving
> though.
>
> That is, ideally we'd already be able to (quickly) tell if a relation
> exists or not, but given how the whole chain_hash stuff is build now, it
> looks like we cannot.
>
>
> Let me think about this a bit more.
OK, so neither this nor the chain-hash completely avoid redundant
dependencies. The only way to do that is doing graph-walks for every
proposed link.
Now, we already do a ton of __bfs() walks in check_prev_add(), so one
more might not hurt too much [*].
Esp. with the chain-hash avoiding all the obvious duplicate work, this
might just work.
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index a95e5d1..7baea89 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1860,6 +1860,17 @@ check_prev_add(struct task_struct *curr, struct
held_lock *prev,
}
}
+ /*
+* Is the -> redundant?
+*/
+ this.class = hlock_class(prev);
+ this.parent = NULL;
+ ret = check_noncircular(, hlock_class(next), _entry);
+ if (!ret) /* exists, redundant */
+ return 2;
+ if (ret < 0)
+ return print_bfs_bug(ret);
+
if (!*stack_saved) {
if (!save_trace())
return 0;
[*] A while ago someone, and I cannot find the email just now, asked if
we could not implement the RECLAIM_FS inversion stuff with a 'fake' lock
like we use for other things like workqueues etc. I think this should be
possible which allows reducing the 'irq' states and will reduce the
amount of __bfs() lookups we do.
Removing the 1 IRQ state, would result in 4 less __bfs() walks if I'm
not mistaken, more than making up for the 1 we'd have to add to detect
redundant links.
include/linux/lockdep.h | 11 +-
include/linux/sched.h | 1 -
kernel/locking/lockdep.c| 87 +
kernel/locking/lockdep_states.h | 1 -
mm/internal.h | 40 +++
mm/page_alloc.c | 13 --
mm/slab.h | 7 +++-
mm/slob.c | 8 +++-
mm/vmscan.c | 13 +++---
9 files changed, 71 insertions(+), 110 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1e327bb..6ba1a65 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -29,7 +29,7 @@ extern int lock_stat;
* We'd rather not expose kernel/lockdep_states.h this wide, but we do need
* the total number of states... :-(
*/
-#define XXX_LOCK_USAGE_STATES (1+3*4)
+#define XXX_LOCK_USAGE_STATES (1+2*4)
/*
* NR_LOCKDEP_CACHING_CLASSES ... Number of classes
@@ -361,10 +361,6 @@ static inline void lock_set_subclass(struct lockdep_map
*lock,
lock_set_class(lock, lock->name, lock->key, subclass, ip);
}
-extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask);
-extern void lockdep_clear_current_reclaim_state(void);
-extern void lockdep_trace_alloc(gfp_t mask);
-
struct pin_cookie { unsigned int val; };
#define NIL_COOKIE (struct pin_cookie){ .val = 0U, }
@@ -373,7 +369,7 @@ extern struct pin_cookie lock_pin_lock(struct lockdep_map
*lock);
extern void lock_repin_lock(struct lockdep_map *lock, struct pin_cookie);
extern void lock_unpin_lock(struct lockdep_map *lock,
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote: > From: Matthew Wilcox [mailto:wi...@infradead.org] > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > > > (And we should not be returning to userspace with locks held anyway -- > > > lockdep already has a check for that). > > > > Don't we return to userspace with page locks held, eg during async > > directio? > > Hello, > > I think that the check when returning to user with crosslocks held > should be an exception. Don't you think so? Oh yes. We have to keep the pages locked during reads, and we have to return to userspace before I/O is complete, therefore we have to return to userspace with pages locked. They'll be unlocked by the interrupt handler in page_endio(). Speaking of which ... this feature is far too heavy for use in production on pages. You're almost trebling the size of struct page. Can we do something like make all struct pages share the same lockdep_map? We'd have to not complain about holding one crossdep lock and acquiring another one of the same type, but with millions of pages in the system, it must surely be creating a gargantuan graph right now?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Thu, Mar 02, 2017 at 01:45:35PM +0900, byungchul.park wrote: > From: Matthew Wilcox [mailto:wi...@infradead.org] > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > > > (And we should not be returning to userspace with locks held anyway -- > > > lockdep already has a check for that). > > > > Don't we return to userspace with page locks held, eg during async > > directio? > > Hello, > > I think that the check when returning to user with crosslocks held > should be an exception. Don't you think so? Oh yes. We have to keep the pages locked during reads, and we have to return to userspace before I/O is complete, therefore we have to return to userspace with pages locked. They'll be unlocked by the interrupt handler in page_endio(). Speaking of which ... this feature is far too heavy for use in production on pages. You're almost trebling the size of struct page. Can we do something like make all struct pages share the same lockdep_map? We'd have to not complain about holding one crossdep lock and acquiring another one of the same type, but with millions of pages in the system, it must surely be creating a gargantuan graph right now?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index a6c8db1..7890661 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -1042,6 +1042,19 @@ config DEBUG_LOCK_ALLOC >spin_lock_init()/mutex_init()/etc., or whether there is any lock >held during task exit. > > +config LOCKDEP_CROSSRELEASE > + bool "Lock debugging: make lockdep work for crosslocks" > + select LOCKDEP > + select TRACE_IRQFLAGS > + default n > + help > + This makes lockdep work for crosslock which is a lock allowed to > + be released in a different context from the acquisition context. > + Normally a lock must be released in the context acquiring the lock. > + However, relexing this constraint helps synchronization primitives > + such as page locks or completions can use the lock correctness > + detector, lockdep. > + > config PROVE_LOCKING > bool "Lock debugging: prove locking correctness" > depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT > && LOCKDEP_SUPPORT Does CROSSRELEASE && !PROVE_LOCKING make any sense?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index a6c8db1..7890661 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -1042,6 +1042,19 @@ config DEBUG_LOCK_ALLOC >spin_lock_init()/mutex_init()/etc., or whether there is any lock >held during task exit. > > +config LOCKDEP_CROSSRELEASE > + bool "Lock debugging: make lockdep work for crosslocks" > + select LOCKDEP > + select TRACE_IRQFLAGS > + default n > + help > + This makes lockdep work for crosslock which is a lock allowed to > + be released in a different context from the acquisition context. > + Normally a lock must be released in the context acquiring the lock. > + However, relexing this constraint helps synchronization primitives > + such as page locks or completions can use the lock correctness > + detector, lockdep. > + > config PROVE_LOCKING > bool "Lock debugging: prove locking correctness" > depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT > && LOCKDEP_SUPPORT Does CROSSRELEASE && !PROVE_LOCKING make any sense?
RE: [PATCH v5 06/13] lockdep: Implement crossrelease feature
> -Original Message- > From: Matthew Wilcox [mailto:wi...@infradead.org] > Sent: Thursday, March 02, 2017 1:20 PM > To: Peter Zijlstra > Cc: Byungchul Park; mi...@kernel.org; t...@linutronix.de; > wal...@google.com; boqun.f...@gmail.com; kir...@shutemov.name; linux- > ker...@vger.kernel.org; linux...@kvack.org; iamjoonsoo@lge.com; > a...@linux-foundation.org; npig...@gmail.com > Subject: Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > > (And we should not be returning to userspace with locks held anyway -- > > lockdep already has a check for that). > > Don't we return to userspace with page locks held, eg during async > directio? Hello, I think that the check when returning to user with crosslocks held should be an exception. Don't you think so? Thanks, Byungchul
RE: [PATCH v5 06/13] lockdep: Implement crossrelease feature
> -Original Message- > From: Matthew Wilcox [mailto:wi...@infradead.org] > Sent: Thursday, March 02, 2017 1:20 PM > To: Peter Zijlstra > Cc: Byungchul Park; mi...@kernel.org; t...@linutronix.de; > wal...@google.com; boqun.f...@gmail.com; kir...@shutemov.name; linux- > ker...@vger.kernel.org; linux...@kvack.org; iamjoonsoo@lge.com; > a...@linux-foundation.org; npig...@gmail.com > Subject: Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature > > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > > (And we should not be returning to userspace with locks held anyway -- > > lockdep already has a check for that). > > Don't we return to userspace with page locks held, eg during async > directio? Hello, I think that the check when returning to user with crosslocks held should be an exception. Don't you think so? Thanks, Byungchul
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > (And we should not be returning to userspace with locks held anyway -- > lockdep already has a check for that). Don't we return to userspace with page locks held, eg during async directio?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote: > (And we should not be returning to userspace with locks held anyway -- > lockdep already has a check for that). Don't we return to userspace with page locks held, eg during async directio?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 02:17:07PM +0900, Byungchul Park wrote:
> > > +void lock_commit_crosslock(struct lockdep_map *lock)
> > > +{
> > > + struct cross_lock *xlock;
> > > + unsigned long flags;
> > > +
> > > + if (!current->xhlocks)
> > > + return;
> > > +
> > > + if (unlikely(current->lockdep_recursion))
> > > + return;
> > > +
> > > + raw_local_irq_save(flags);
> > > + check_flags(flags);
> > > + current->lockdep_recursion = 1;
> > > +
> > > + if (unlikely(!debug_locks))
> > > + return;
> > > +
> > > + if (!graph_lock())
> > > + return;
> > > +
> > > + xlock = &((struct lockdep_map_cross *)lock)->xlock;
> > > + if (atomic_read(>ref) > 0 && !commit_xhlocks(xlock))
> >
> > You terminate with graph_lock() held.
>
> Oops. What did I do? I'll fix it.
I remembered it. It's no problem because it would terminate there, only
if _both_ 'xlock->ref > 0' and 'commit_xhlocks returns 0' are true.
Otherwise, it will unlock the lock safely.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 02:17:07PM +0900, Byungchul Park wrote:
> > > +void lock_commit_crosslock(struct lockdep_map *lock)
> > > +{
> > > + struct cross_lock *xlock;
> > > + unsigned long flags;
> > > +
> > > + if (!current->xhlocks)
> > > + return;
> > > +
> > > + if (unlikely(current->lockdep_recursion))
> > > + return;
> > > +
> > > + raw_local_irq_save(flags);
> > > + check_flags(flags);
> > > + current->lockdep_recursion = 1;
> > > +
> > > + if (unlikely(!debug_locks))
> > > + return;
> > > +
> > > + if (!graph_lock())
> > > + return;
> > > +
> > > + xlock = &((struct lockdep_map_cross *)lock)->xlock;
> > > + if (atomic_read(>ref) > 0 && !commit_xhlocks(xlock))
> >
> > You terminate with graph_lock() held.
>
> Oops. What did I do? I'll fix it.
I remembered it. It's no problem because it would terminate there, only
if _both_ 'xlock->ref > 0' and 'commit_xhlocks returns 0' are true.
Otherwise, it will unlock the lock safely.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 02:40:18PM +0100, Peter Zijlstra wrote:
> > > +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;
> > > +}
> >
> > So I'm still struggling with prev_gen_id; is it an optimization or is it
> > required for correctness?
>
> It's an optimization, but very essential and important optimization.
>
> in hlocks[]
>
> A gen_id (4) --+
> | previous gen_id
> B gen_id (3) <-+
> C gen_id (3)
> D gen_id (2)
> oldest -> E gen_id (1)
>
> in xhlocks[]
>
>^ A gen_id (4) prev_gen_id (3: B's gen id)
>| B gen_id (3) prev_gen_id (3: C's gen id)
>| C gen_id (3) prev_gen_id (2: D's gen id)
>| D gen_id (2) prev_gen_id (1: E's gen id)
>| E gen_id (1) prev_gen_id (NA)
>
> Let's consider the case that the gen id of xlock to commit is 3.
>
> In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> 'B -> A' which are already generated by original lockdep.
>
> I use the prev_gen_id to avoid adding this kind of redundant
> dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> means that the previous lock in hlocks[] is able to handle the
> dependency on its commit stage.
>
Aah, I completely missed it was against held_locks.
Hurm.. it feels like this is solving a problem we shouldn't be solving
though.
That is, ideally we'd already be able to (quickly) tell if a relation
exists or not, but given how the whole chain_hash stuff is build now, it
looks like we cannot.
Let me think about this a bit more.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 02:43:23PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 02:40:18PM +0100, Peter Zijlstra wrote:
> > > +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;
> > > +}
> >
> > So I'm still struggling with prev_gen_id; is it an optimization or is it
> > required for correctness?
>
> It's an optimization, but very essential and important optimization.
>
> in hlocks[]
>
> A gen_id (4) --+
> | previous gen_id
> B gen_id (3) <-+
> C gen_id (3)
> D gen_id (2)
> oldest -> E gen_id (1)
>
> in xhlocks[]
>
>^ A gen_id (4) prev_gen_id (3: B's gen id)
>| B gen_id (3) prev_gen_id (3: C's gen id)
>| C gen_id (3) prev_gen_id (2: D's gen id)
>| D gen_id (2) prev_gen_id (1: E's gen id)
>| E gen_id (1) prev_gen_id (NA)
>
> Let's consider the case that the gen id of xlock to commit is 3.
>
> In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
> and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
> 'B -> A' which are already generated by original lockdep.
>
> I use the prev_gen_id to avoid adding this kind of redundant
> dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
> means that the previous lock in hlocks[] is able to handle the
> dependency on its commit stage.
>
Aah, I completely missed it was against held_locks.
Hurm.. it feels like this is solving a problem we shouldn't be solving
though.
That is, ideally we'd already be able to (quickly) tell if a relation
exists or not, but given how the whole chain_hash stuff is build now, it
looks like we cannot.
Let me think about this a bit more.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 11:45:48AM +0100, Peter Zijlstra wrote: > On Wed, Mar 01, 2017 at 01:40:33PM +0900, Byungchul Park wrote: > > > Right. I decided to force MAX_XHLOCKS_NR to be power of 2 and everything > > became easy. Thank you very much. > > Something like: > > BUILD_BUG_ON(MAX_XHLOCKS_NR & (MAX_XHLOCK_NR - 1)); > > Should enforce I think. Good idea! Thank you very much.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 11:45:48AM +0100, Peter Zijlstra wrote: > On Wed, Mar 01, 2017 at 01:40:33PM +0900, Byungchul Park wrote: > > > Right. I decided to force MAX_XHLOCKS_NR to be power of 2 and everything > > became easy. Thank you very much. > > Something like: > > BUILD_BUG_ON(MAX_XHLOCKS_NR & (MAX_XHLOCK_NR - 1)); > > Should enforce I think. Good idea! Thank you very much.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 11:43:28AM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 04:21:28PM +0900, Byungchul Park wrote:
> > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > > + /*
> > > > +* 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.
> > > > +*
> > > > +* TODO: We can also add dependencies between two acquisitions
> > > > +* of different work_id, if they don't cause a sleep so make
> > > > +* the worker stalled.
> > > > +*/
> > > > + unsigned intwork_id;
> > >
> > > > +/*
> > > > + * 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++;
> > > > +}
> > >
> > > So what you're trying to do with that 'work_id' thing is basically wipe
> > > the entire history when we're at the bottom of a context.
> >
> > Sorry, but I do not understand what you are trying to say.
> >
> > What I was trying to do with the 'work_id' is to distinguish between
> > different works, which will be used to check if history locks were held
> > in the same context as a release one.
>
> The effect of changing work_id is that history disappears, yes? That is,
> by changing it, all our hist_locks don't match the context anymore and
> therefore we have no history.
Right. Now I understood your words.
> This is a useful operation.
>
> You would want to do this at points where you know there will not be any
> dependencies on prior action, and typically at the same points we want
> to not be holding any locks.
>
> Hence my term: 'bottom of a context', referring to an empty (held) lock
> stack.
Right.
> I would say this needs to be done for all 'work-queue' like things, and
Of course.
> there are quite a few outside of the obvious ones, smpboot threads and
> many other kthreads fall into this category.
Where can I check those?
> Similarly the return to userspace point that I already mentioned.
>
> I would propose something like:
>
> lockdep_assert_empty();
>
> Or something similar, which would verify the lock stack is indeed empty
> and wipe our entire hist_lock buffer when cross-release is enabled.
Right. I should do that.
> > > Which is a useful operation, but should arguably also be done on the
> > > return to userspace path. Any historical lock from before the current
> > > syscall is irrelevant.
Let me think more. It looks not a simple problem.
> >
> > Sorry. Could you explain it more?
>
> Does the above make things clear?
Perfect. Thank you very much.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 11:43:28AM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 04:21:28PM +0900, Byungchul Park wrote:
> > On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > > + /*
> > > > +* 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.
> > > > +*
> > > > +* TODO: We can also add dependencies between two acquisitions
> > > > +* of different work_id, if they don't cause a sleep so make
> > > > +* the worker stalled.
> > > > +*/
> > > > + unsigned intwork_id;
> > >
> > > > +/*
> > > > + * 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++;
> > > > +}
> > >
> > > So what you're trying to do with that 'work_id' thing is basically wipe
> > > the entire history when we're at the bottom of a context.
> >
> > Sorry, but I do not understand what you are trying to say.
> >
> > What I was trying to do with the 'work_id' is to distinguish between
> > different works, which will be used to check if history locks were held
> > in the same context as a release one.
>
> The effect of changing work_id is that history disappears, yes? That is,
> by changing it, all our hist_locks don't match the context anymore and
> therefore we have no history.
Right. Now I understood your words.
> This is a useful operation.
>
> You would want to do this at points where you know there will not be any
> dependencies on prior action, and typically at the same points we want
> to not be holding any locks.
>
> Hence my term: 'bottom of a context', referring to an empty (held) lock
> stack.
Right.
> I would say this needs to be done for all 'work-queue' like things, and
Of course.
> there are quite a few outside of the obvious ones, smpboot threads and
> many other kthreads fall into this category.
Where can I check those?
> Similarly the return to userspace point that I already mentioned.
>
> I would propose something like:
>
> lockdep_assert_empty();
>
> Or something similar, which would verify the lock stack is indeed empty
> and wipe our entire hist_lock buffer when cross-release is enabled.
Right. I should do that.
> > > Which is a useful operation, but should arguably also be done on the
> > > return to userspace path. Any historical lock from before the current
> > > syscall is irrelevant.
Let me think more. It looks not a simple problem.
> >
> > Sorry. Could you explain it more?
>
> Does the above make things clear?
Perfect. Thank you very much.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 01:40:33PM +0900, Byungchul Park wrote: > Right. I decided to force MAX_XHLOCKS_NR to be power of 2 and everything > became easy. Thank you very much. Something like: BUILD_BUG_ON(MAX_XHLOCKS_NR & (MAX_XHLOCK_NR - 1)); Should enforce I think.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 01:40:33PM +0900, Byungchul Park wrote: > Right. I decided to force MAX_XHLOCKS_NR to be power of 2 and everything > became easy. Thank you very much. Something like: BUILD_BUG_ON(MAX_XHLOCKS_NR & (MAX_XHLOCK_NR - 1)); Should enforce I think.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 04:21:28PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > + /*
> > > + * 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.
> > > + *
> > > + * TODO: We can also add dependencies between two acquisitions
> > > + * of different work_id, if they don't cause a sleep so make
> > > + * the worker stalled.
> > > + */
> > > + unsigned intwork_id;
> >
> > > +/*
> > > + * 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++;
> > > +}
> >
> > So what you're trying to do with that 'work_id' thing is basically wipe
> > the entire history when we're at the bottom of a context.
>
> Sorry, but I do not understand what you are trying to say.
>
> What I was trying to do with the 'work_id' is to distinguish between
> different works, which will be used to check if history locks were held
> in the same context as a release one.
The effect of changing work_id is that history disappears, yes? That is,
by changing it, all our hist_locks don't match the context anymore and
therefore we have no history.
This is a useful operation.
You would want to do this at points where you know there will not be any
dependencies on prior action, and typically at the same points we want
to not be holding any locks.
Hence my term: 'bottom of a context', referring to an empty (held) lock
stack.
I would say this needs to be done for all 'work-queue' like things, and
there are quite a few outside of the obvious ones, smpboot threads and
many other kthreads fall into this category.
Similarly the return to userspace point that I already mentioned.
I would propose something like:
lockdep_assert_empty();
Or something similar, which would verify the lock stack is indeed empty
and wipe our entire hist_lock buffer when cross-release is enabled.
> > Which is a useful operation, but should arguably also be done on the
> > return to userspace path. Any historical lock from before the current
> > syscall is irrelevant.
>
> Sorry. Could you explain it more?
Does the above make things clear?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 04:21:28PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > + /*
> > > + * 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.
> > > + *
> > > + * TODO: We can also add dependencies between two acquisitions
> > > + * of different work_id, if they don't cause a sleep so make
> > > + * the worker stalled.
> > > + */
> > > + unsigned intwork_id;
> >
> > > +/*
> > > + * 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++;
> > > +}
> >
> > So what you're trying to do with that 'work_id' thing is basically wipe
> > the entire history when we're at the bottom of a context.
>
> Sorry, but I do not understand what you are trying to say.
>
> What I was trying to do with the 'work_id' is to distinguish between
> different works, which will be used to check if history locks were held
> in the same context as a release one.
The effect of changing work_id is that history disappears, yes? That is,
by changing it, all our hist_locks don't match the context anymore and
therefore we have no history.
This is a useful operation.
You would want to do this at points where you know there will not be any
dependencies on prior action, and typically at the same points we want
to not be holding any locks.
Hence my term: 'bottom of a context', referring to an empty (held) lock
stack.
I would say this needs to be done for all 'work-queue' like things, and
there are quite a few outside of the obvious ones, smpboot threads and
many other kthreads fall into this category.
Similarly the return to userspace point that I already mentioned.
I would propose something like:
lockdep_assert_empty();
Or something similar, which would verify the lock stack is indeed empty
and wipe our entire hist_lock buffer when cross-release is enabled.
> > Which is a useful operation, but should arguably also be done on the
> > return to userspace path. Any historical lock from before the current
> > syscall is irrelevant.
>
> Sorry. Could you explain it more?
Does the above make things clear?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > + /*
> > +* 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.
> > +*
> > +* TODO: We can also add dependencies between two acquisitions
> > +* of different work_id, if they don't cause a sleep so make
> > +* the worker stalled.
> > +*/
> > + unsigned intwork_id;
>
> > +/*
> > + * 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++;
> > +}
>
> So what you're trying to do with that 'work_id' thing is basically wipe
> the entire history when we're at the bottom of a context.
Sorry, but I do not understand what you are trying to say.
What I was trying to do with the 'work_id' is to distinguish between
different works, which will be used to check if history locks were held
in the same context as a release one.
> Which is a useful operation, but should arguably also be done on the
> return to userspace path. Any historical lock from before the current
> syscall is irrelevant.
Sorry. Could you explain it more?
>
> (And we should not be returning to userspace with locks held anyway --
> lockdep already has a check for that).
Yes right. We should not be returning to userspace without reporting it
in that case.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 07:15:47PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > + /*
> > +* 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.
> > +*
> > +* TODO: We can also add dependencies between two acquisitions
> > +* of different work_id, if they don't cause a sleep so make
> > +* the worker stalled.
> > +*/
> > + unsigned intwork_id;
>
> > +/*
> > + * 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++;
> > +}
>
> So what you're trying to do with that 'work_id' thing is basically wipe
> the entire history when we're at the bottom of a context.
Sorry, but I do not understand what you are trying to say.
What I was trying to do with the 'work_id' is to distinguish between
different works, which will be used to check if history locks were held
in the same context as a release one.
> Which is a useful operation, but should arguably also be done on the
> return to userspace path. Any historical lock from before the current
> syscall is irrelevant.
Sorry. Could you explain it more?
>
> (And we should not be returning to userspace with locks held anyway --
> lockdep already has a check for that).
Yes right. We should not be returning to userspace without reporting it
in that case.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 01:49:06PM +0100, Peter Zijlstra wrote: > On Tue, Feb 28, 2017 at 01:45:07PM +0100, Peter Zijlstra wrote: > > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > > > + /* > > > + * struct held_lock does not have an indicator whether in nmi. > > > + */ > > > + int nmi; > > > > Do we really need this? Lockdep doesn't really know about NMI context, > > so its weird to now partially introduce it. > > That is, see how nmi_enter() includes lockdep_off(). Indeed. OK. I will fix it.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 01:49:06PM +0100, Peter Zijlstra wrote: > On Tue, Feb 28, 2017 at 01:45:07PM +0100, Peter Zijlstra wrote: > > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > > > + /* > > > + * struct held_lock does not have an indicator whether in nmi. > > > + */ > > > + int nmi; > > > > Do we really need this? Lockdep doesn't really know about NMI context, > > so its weird to now partially introduce it. > > That is, see how nmi_enter() includes lockdep_off(). Indeed. OK. I will fix it.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 02:17:07PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 04:49:00PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> >
> > > +struct cross_lock {
> > > + /*
> > > + * When more than one acquisition of crosslocks are overlapped,
> > > + * we do actual commit only when ref == 0.
> > > + */
> > > + atomic_t ref;
> >
> > That comment doesn't seem right, should that be: ref != 0 ?
> > Also; would it not be much clearer to call this: nr_blocked, or waiters
> > or something along those lines, because that is what it appears to be.
Honestly, I forgot why I introduced the ref.. I will remove the ref next
spin, and handle waiters in another way.
Thank you,
Byungchul
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Mar 01, 2017 at 02:17:07PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 04:49:00PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> >
> > > +struct cross_lock {
> > > + /*
> > > + * When more than one acquisition of crosslocks are overlapped,
> > > + * we do actual commit only when ref == 0.
> > > + */
> > > + atomic_t ref;
> >
> > That comment doesn't seem right, should that be: ref != 0 ?
> > Also; would it not be much clearer to call this: nr_blocked, or waiters
> > or something along those lines, because that is what it appears to be.
Honestly, I forgot why I introduced the ref.. I will remove the ref next
spin, and handle waiters in another way.
Thank you,
Byungchul
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 02:40:18PM +0100, Peter Zijlstra wrote:
> > +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;
> > +}
>
> So I'm still struggling with prev_gen_id; is it an optimization or is it
> required for correctness?
It's an optimization, but very essential and important optimization.
in hlocks[]
A gen_id (4) --+
| previous gen_id
B gen_id (3) <-+
C gen_id (3)
D gen_id (2)
oldest -> E gen_id (1)
in xhlocks[]
^ A gen_id (4) prev_gen_id (3: B's gen id)
| B gen_id (3) prev_gen_id (3: C's gen id)
| C gen_id (3) prev_gen_id (2: D's gen id)
| D gen_id (2) prev_gen_id (1: E's gen id)
| E gen_id (1) prev_gen_id (NA)
Let's consider the case that the gen id of xlock to commit is 3.
In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
'B -> A' which are already generated by original lockdep.
I use the prev_gen_id to avoid adding this kind of redundant
dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
means that the previous lock in hlocks[] is able to handle the
dependency on its commit stage.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 02:40:18PM +0100, Peter Zijlstra wrote:
> > +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;
> > +}
>
> So I'm still struggling with prev_gen_id; is it an optimization or is it
> required for correctness?
It's an optimization, but very essential and important optimization.
in hlocks[]
A gen_id (4) --+
| previous gen_id
B gen_id (3) <-+
C gen_id (3)
D gen_id (2)
oldest -> E gen_id (1)
in xhlocks[]
^ A gen_id (4) prev_gen_id (3: B's gen id)
| B gen_id (3) prev_gen_id (3: C's gen id)
| C gen_id (3) prev_gen_id (2: D's gen id)
| D gen_id (2) prev_gen_id (1: E's gen id)
| E gen_id (1) prev_gen_id (NA)
Let's consider the case that the gen id of xlock to commit is 3.
In this case, it's engough to generate 'the xlock -> C'. 'the xlock -> B'
and 'the xlock -> A' are unnecessary since it's covered by 'C -> B' and
'B -> A' which are already generated by original lockdep.
I use the prev_gen_id to avoid adding this kind of redundant
dependencies. In other words, xhlock->prev_gen_id >= xlock->hlock.gen_id
means that the previous lock in hlocks[] is able to handle the
dependency on its commit stage.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
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.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
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.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 04:49:00PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
>
> > +struct cross_lock {
> > + /*
> > +* When more than one acquisition of crosslocks are overlapped,
> > +* we do actual commit only when ref == 0.
> > +*/
> > + atomic_t ref;
>
> That comment doesn't seem right, should that be: ref != 0 ?
> Also; would it not be much clearer to call this: nr_blocked, or waiters
> or something along those lines, because that is what it appears to be.
>
> > + /*
> > +* Seperate hlock instance. This will be used at commit step.
> > +*
> > +* TODO: Use a smaller data structure containing only necessary
> > +* data. However, we should make lockdep code able to handle the
> > +* smaller one first.
> > +*/
> > + struct held_lockhlock;
> > +};
>
> > +static int add_xlock(struct held_lock *hlock)
> > +{
> > + struct cross_lock *xlock;
> > + unsigned int gen_id;
> > +
> > + if (!depend_after(hlock))
> > + return 1;
> > +
> > + if (!graph_lock())
> > + return 0;
> > +
> > + xlock = &((struct lockdep_map_cross *)hlock->instance)->xlock;
> > +
> > + /*
> > +* When acquisitions for a xlock are overlapped, we use
> > +* a reference counter to handle it.
>
> Handle what!? That comment is near empty.
I will add more comment so that it can fully descibe.
>
> > +*/
> > + if (atomic_inc_return(>ref) > 1)
> > + goto unlock;
>
> So you set the xlock's generation only once, to the oldest blocking-on
> relation, which makes sense, you want to be able to related to all
> historical locks since.
>
> > +
> > + gen_id = (unsigned int)atomic_inc_return(_gen_id);
> > + xlock->hlock = *hlock;
> > + xlock->hlock.gen_id = gen_id;
> > +unlock:
> > + graph_unlock();
> > + return 1;
> > +}
>
> > +void lock_commit_crosslock(struct lockdep_map *lock)
> > +{
> > + struct cross_lock *xlock;
> > + unsigned long flags;
> > +
> > + if (!current->xhlocks)
> > + return;
> > +
> > + if (unlikely(current->lockdep_recursion))
> > + return;
> > +
> > + raw_local_irq_save(flags);
> > + check_flags(flags);
> > + current->lockdep_recursion = 1;
> > +
> > + if (unlikely(!debug_locks))
> > + return;
> > +
> > + if (!graph_lock())
> > + return;
> > +
> > + xlock = &((struct lockdep_map_cross *)lock)->xlock;
> > + if (atomic_read(>ref) > 0 && !commit_xhlocks(xlock))
>
> You terminate with graph_lock() held.
Oops. What did I do? I'll fix it.
>
> Also, I think you can do the atomic_read() outside of graph lock, to
> avoid taking graph_lock when its 0.
I'll do that if possible after thinking more.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 04:49:00PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
>
> > +struct cross_lock {
> > + /*
> > +* When more than one acquisition of crosslocks are overlapped,
> > +* we do actual commit only when ref == 0.
> > +*/
> > + atomic_t ref;
>
> That comment doesn't seem right, should that be: ref != 0 ?
> Also; would it not be much clearer to call this: nr_blocked, or waiters
> or something along those lines, because that is what it appears to be.
>
> > + /*
> > +* Seperate hlock instance. This will be used at commit step.
> > +*
> > +* TODO: Use a smaller data structure containing only necessary
> > +* data. However, we should make lockdep code able to handle the
> > +* smaller one first.
> > +*/
> > + struct held_lockhlock;
> > +};
>
> > +static int add_xlock(struct held_lock *hlock)
> > +{
> > + struct cross_lock *xlock;
> > + unsigned int gen_id;
> > +
> > + if (!depend_after(hlock))
> > + return 1;
> > +
> > + if (!graph_lock())
> > + return 0;
> > +
> > + xlock = &((struct lockdep_map_cross *)hlock->instance)->xlock;
> > +
> > + /*
> > +* When acquisitions for a xlock are overlapped, we use
> > +* a reference counter to handle it.
>
> Handle what!? That comment is near empty.
I will add more comment so that it can fully descibe.
>
> > +*/
> > + if (atomic_inc_return(>ref) > 1)
> > + goto unlock;
>
> So you set the xlock's generation only once, to the oldest blocking-on
> relation, which makes sense, you want to be able to related to all
> historical locks since.
>
> > +
> > + gen_id = (unsigned int)atomic_inc_return(_gen_id);
> > + xlock->hlock = *hlock;
> > + xlock->hlock.gen_id = gen_id;
> > +unlock:
> > + graph_unlock();
> > + return 1;
> > +}
>
> > +void lock_commit_crosslock(struct lockdep_map *lock)
> > +{
> > + struct cross_lock *xlock;
> > + unsigned long flags;
> > +
> > + if (!current->xhlocks)
> > + return;
> > +
> > + if (unlikely(current->lockdep_recursion))
> > + return;
> > +
> > + raw_local_irq_save(flags);
> > + check_flags(flags);
> > + current->lockdep_recursion = 1;
> > +
> > + if (unlikely(!debug_locks))
> > + return;
> > +
> > + if (!graph_lock())
> > + return;
> > +
> > + xlock = &((struct lockdep_map_cross *)lock)->xlock;
> > + if (atomic_read(>ref) > 0 && !commit_xhlocks(xlock))
>
> You terminate with graph_lock() held.
Oops. What did I do? I'll fix it.
>
> Also, I think you can do the atomic_read() outside of graph lock, to
> avoid taking graph_lock when its 0.
I'll do that if possible after thinking more.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> + /*
> + * 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.
> + *
> + * TODO: We can also add dependencies between two acquisitions
> + * of different work_id, if they don't cause a sleep so make
> + * the worker stalled.
> + */
> + unsigned intwork_id;
> +/*
> + * 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++;
> +}
So what you're trying to do with that 'work_id' thing is basically wipe
the entire history when we're at the bottom of a context.
Which is a useful operation, but should arguably also be done on the
return to userspace path. Any historical lock from before the current
syscall is irrelevant.
(And we should not be returning to userspace with locks held anyway --
lockdep already has a check for that).
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> + /*
> + * 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.
> + *
> + * TODO: We can also add dependencies between two acquisitions
> + * of different work_id, if they don't cause a sleep so make
> + * the worker stalled.
> + */
> + unsigned intwork_id;
> +/*
> + * 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++;
> +}
So what you're trying to do with that 'work_id' thing is basically wipe
the entire history when we're at the bottom of a context.
Which is a useful operation, but should arguably also be done on the
return to userspace path. Any historical lock from before the current
syscall is irrelevant.
(And we should not be returning to userspace with locks held anyway --
lockdep already has a check for that).
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
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.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
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.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> +struct cross_lock {
> + /*
> + * When more than one acquisition of crosslocks are overlapped,
> + * we do actual commit only when ref == 0.
> + */
> + atomic_t ref;
That comment doesn't seem right, should that be: ref != 0 ?
Also; would it not be much clearer to call this: nr_blocked, or waiters
or something along those lines, because that is what it appears to be.
> + /*
> + * Seperate hlock instance. This will be used at commit step.
> + *
> + * TODO: Use a smaller data structure containing only necessary
> + * data. However, we should make lockdep code able to handle the
> + * smaller one first.
> + */
> + struct held_lockhlock;
> +};
> +static int add_xlock(struct held_lock *hlock)
> +{
> + struct cross_lock *xlock;
> + unsigned int gen_id;
> +
> + if (!depend_after(hlock))
> + return 1;
> +
> + if (!graph_lock())
> + return 0;
> +
> + xlock = &((struct lockdep_map_cross *)hlock->instance)->xlock;
> +
> + /*
> + * When acquisitions for a xlock are overlapped, we use
> + * a reference counter to handle it.
Handle what!? That comment is near empty.
> + */
> + if (atomic_inc_return(>ref) > 1)
> + goto unlock;
So you set the xlock's generation only once, to the oldest blocking-on
relation, which makes sense, you want to be able to related to all
historical locks since.
> +
> + gen_id = (unsigned int)atomic_inc_return(_gen_id);
> + xlock->hlock = *hlock;
> + xlock->hlock.gen_id = gen_id;
> +unlock:
> + graph_unlock();
> + return 1;
> +}
> +void lock_commit_crosslock(struct lockdep_map *lock)
> +{
> + struct cross_lock *xlock;
> + unsigned long flags;
> +
> + if (!current->xhlocks)
> + return;
> +
> + if (unlikely(current->lockdep_recursion))
> + return;
> +
> + raw_local_irq_save(flags);
> + check_flags(flags);
> + current->lockdep_recursion = 1;
> +
> + if (unlikely(!debug_locks))
> + return;
> +
> + if (!graph_lock())
> + return;
> +
> + xlock = &((struct lockdep_map_cross *)lock)->xlock;
> + if (atomic_read(>ref) > 0 && !commit_xhlocks(xlock))
You terminate with graph_lock() held.
Also, I think you can do the atomic_read() outside of graph lock, to
avoid taking graph_lock when its 0.
> + return;
> +
> + graph_unlock();
> + current->lockdep_recursion = 0;
> + raw_local_irq_restore(flags);
> +}
> +EXPORT_SYMBOL_GPL(lock_commit_crosslock);
> +
> +/*
> + * return 0: Need to do normal release operation.
> + * return 1: Done. No more release ops is needed.
> + */
> +static int lock_release_crosslock(struct lockdep_map *lock)
> +{
> + if (cross_lock(lock)) {
> + atomic_dec(&((struct lockdep_map_cross *)lock)->xlock.ref);
> + return 1;
> + }
> + return 0;
> +}
> +
> +static void cross_init(struct lockdep_map *lock, int cross)
> +{
> + if (cross)
> + atomic_set(&((struct lockdep_map_cross *)lock)->xlock.ref, 0);
> +
> + lock->cross = cross;
> +}
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> +struct cross_lock {
> + /*
> + * When more than one acquisition of crosslocks are overlapped,
> + * we do actual commit only when ref == 0.
> + */
> + atomic_t ref;
That comment doesn't seem right, should that be: ref != 0 ?
Also; would it not be much clearer to call this: nr_blocked, or waiters
or something along those lines, because that is what it appears to be.
> + /*
> + * Seperate hlock instance. This will be used at commit step.
> + *
> + * TODO: Use a smaller data structure containing only necessary
> + * data. However, we should make lockdep code able to handle the
> + * smaller one first.
> + */
> + struct held_lockhlock;
> +};
> +static int add_xlock(struct held_lock *hlock)
> +{
> + struct cross_lock *xlock;
> + unsigned int gen_id;
> +
> + if (!depend_after(hlock))
> + return 1;
> +
> + if (!graph_lock())
> + return 0;
> +
> + xlock = &((struct lockdep_map_cross *)hlock->instance)->xlock;
> +
> + /*
> + * When acquisitions for a xlock are overlapped, we use
> + * a reference counter to handle it.
Handle what!? That comment is near empty.
> + */
> + if (atomic_inc_return(>ref) > 1)
> + goto unlock;
So you set the xlock's generation only once, to the oldest blocking-on
relation, which makes sense, you want to be able to related to all
historical locks since.
> +
> + gen_id = (unsigned int)atomic_inc_return(_gen_id);
> + xlock->hlock = *hlock;
> + xlock->hlock.gen_id = gen_id;
> +unlock:
> + graph_unlock();
> + return 1;
> +}
> +void lock_commit_crosslock(struct lockdep_map *lock)
> +{
> + struct cross_lock *xlock;
> + unsigned long flags;
> +
> + if (!current->xhlocks)
> + return;
> +
> + if (unlikely(current->lockdep_recursion))
> + return;
> +
> + raw_local_irq_save(flags);
> + check_flags(flags);
> + current->lockdep_recursion = 1;
> +
> + if (unlikely(!debug_locks))
> + return;
> +
> + if (!graph_lock())
> + return;
> +
> + xlock = &((struct lockdep_map_cross *)lock)->xlock;
> + if (atomic_read(>ref) > 0 && !commit_xhlocks(xlock))
You terminate with graph_lock() held.
Also, I think you can do the atomic_read() outside of graph lock, to
avoid taking graph_lock when its 0.
> + return;
> +
> + graph_unlock();
> + current->lockdep_recursion = 0;
> + raw_local_irq_restore(flags);
> +}
> +EXPORT_SYMBOL_GPL(lock_commit_crosslock);
> +
> +/*
> + * return 0: Need to do normal release operation.
> + * return 1: Done. No more release ops is needed.
> + */
> +static int lock_release_crosslock(struct lockdep_map *lock)
> +{
> + if (cross_lock(lock)) {
> + atomic_dec(&((struct lockdep_map_cross *)lock)->xlock.ref);
> + return 1;
> + }
> + return 0;
> +}
> +
> +static void cross_init(struct lockdep_map *lock, int cross)
> +{
> + if (cross)
> + atomic_set(&((struct lockdep_map_cross *)lock)->xlock.ref, 0);
> +
> + lock->cross = cross;
> +}
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> + /*
> + * If the previous in held_locks can create a proper dependency
> + * with a target crosslock, then we can skip commiting this,
> + * since "the target crosslock -> the previous lock" and
> + * "the previous lock -> this lock" can cover the case. So we
> + * keep the previous's gen_id to make the decision.
> + */
> + unsigned intprev_gen_id;
> +static void add_xhlock(struct held_lock *hlock, unsigned int prev_gen_id)
> +{
> + struct hist_lock *xhlock;
> +
> + xhlock = alloc_xhlock();
> +
> + /* Initialize hist_lock's members */
> + xhlock->hlock = *hlock;
> + xhlock->nmi = !!(preempt_count() & NMI_MASK);
> + /*
> + * prev_gen_id is used to skip adding dependency at commit step,
> + * when the previous lock in held_locks can do that instead.
> + */
> + xhlock->prev_gen_id = prev_gen_id;
> + xhlock->work_id = current->work_id;
> +
> + xhlock->trace.nr_entries = 0;
> + xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> + xhlock->trace.entries = xhlock->trace_entries;
> + xhlock->trace.skip = 3;
> + save_stack_trace(>trace);
> +}
> +static void check_add_xhlock(struct held_lock *hlock)
> +{
> + struct held_lock *prev;
> + struct held_lock *start;
> + unsigned int gen_id;
> + unsigned int gen_id_invalid;
> +
> + if (!current->xhlocks || !depend_before(hlock))
> + return;
> +
> + gen_id = (unsigned int)atomic_read(_gen_id);
> + /*
> + * gen_id_invalid must be too old to be valid. That means
> + * current hlock should not be skipped but should be
> + * considered at commit step.
> + */
> + gen_id_invalid = gen_id - (UINT_MAX / 4);
> + start = current->held_locks;
> +
> + for (prev = hlock - 1; prev >= start &&
> + !depend_before(prev); prev--);
> +
> + if (prev < start)
> + add_xhlock(hlock, gen_id_invalid);
> + else if (prev->gen_id != gen_id)
> + add_xhlock(hlock, prev->gen_id);
> +}
> +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;
> +}
So I'm still struggling with prev_gen_id; is it an optimization or is it
required for correctness?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> + /*
> + * If the previous in held_locks can create a proper dependency
> + * with a target crosslock, then we can skip commiting this,
> + * since "the target crosslock -> the previous lock" and
> + * "the previous lock -> this lock" can cover the case. So we
> + * keep the previous's gen_id to make the decision.
> + */
> + unsigned intprev_gen_id;
> +static void add_xhlock(struct held_lock *hlock, unsigned int prev_gen_id)
> +{
> + struct hist_lock *xhlock;
> +
> + xhlock = alloc_xhlock();
> +
> + /* Initialize hist_lock's members */
> + xhlock->hlock = *hlock;
> + xhlock->nmi = !!(preempt_count() & NMI_MASK);
> + /*
> + * prev_gen_id is used to skip adding dependency at commit step,
> + * when the previous lock in held_locks can do that instead.
> + */
> + xhlock->prev_gen_id = prev_gen_id;
> + xhlock->work_id = current->work_id;
> +
> + xhlock->trace.nr_entries = 0;
> + xhlock->trace.max_entries = MAX_XHLOCK_TRACE_ENTRIES;
> + xhlock->trace.entries = xhlock->trace_entries;
> + xhlock->trace.skip = 3;
> + save_stack_trace(>trace);
> +}
> +static void check_add_xhlock(struct held_lock *hlock)
> +{
> + struct held_lock *prev;
> + struct held_lock *start;
> + unsigned int gen_id;
> + unsigned int gen_id_invalid;
> +
> + if (!current->xhlocks || !depend_before(hlock))
> + return;
> +
> + gen_id = (unsigned int)atomic_read(_gen_id);
> + /*
> + * gen_id_invalid must be too old to be valid. That means
> + * current hlock should not be skipped but should be
> + * considered at commit step.
> + */
> + gen_id_invalid = gen_id - (UINT_MAX / 4);
> + start = current->held_locks;
> +
> + for (prev = hlock - 1; prev >= start &&
> + !depend_before(prev); prev--);
> +
> + if (prev < start)
> + add_xhlock(hlock, gen_id_invalid);
> + else if (prev->gen_id != gen_id)
> + add_xhlock(hlock, prev->gen_id);
> +}
> +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;
> +}
So I'm still struggling with prev_gen_id; is it an optimization or is it
required for correctness?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
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?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
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?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 02:35:21PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 28, 2017 at 10:28:20PM +0900, Byungchul Park wrote:
> > On Tue, Feb 28, 2017 at 02:05:13PM +0100, Peter Zijlstra wrote:
> > > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > > +#define MAX_XHLOCKS_NR 64UL
> > >
> > > > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > > + if (tsk->xhlocks) {
> > > > + void *tmp = tsk->xhlocks;
> > > > + /* Disable crossrelease for current */
> > > > + tsk->xhlocks = NULL;
> > > > + vfree(tmp);
> > > > + }
> > > > +#endif
> > >
> > > > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > > + p->xhlock_idx = 0;
> > > > + p->xhlock_idx_soft = 0;
> > > > + p->xhlock_idx_hard = 0;
> > > > + p->xhlock_idx_nmi = 0;
> > > > + p->xhlocks = vzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR);
> > >
> > > I don't think we need vmalloc for this now.
> >
> > Really? When is a better time to do it?
> >
> > I think the time creating a task is the best time to initialize it. No?
>
> The place is fine, but I would use kmalloc() now (and subsequently kfree
> on the other end) for the allocation. Its not _that_ large anymore,
> right?
Did you mean that? OK, I will do it.
Thank you.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 02:35:21PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 28, 2017 at 10:28:20PM +0900, Byungchul Park wrote:
> > On Tue, Feb 28, 2017 at 02:05:13PM +0100, Peter Zijlstra wrote:
> > > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > > +#define MAX_XHLOCKS_NR 64UL
> > >
> > > > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > > + if (tsk->xhlocks) {
> > > > + void *tmp = tsk->xhlocks;
> > > > + /* Disable crossrelease for current */
> > > > + tsk->xhlocks = NULL;
> > > > + vfree(tmp);
> > > > + }
> > > > +#endif
> > >
> > > > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > > + p->xhlock_idx = 0;
> > > > + p->xhlock_idx_soft = 0;
> > > > + p->xhlock_idx_hard = 0;
> > > > + p->xhlock_idx_nmi = 0;
> > > > + p->xhlocks = vzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR);
> > >
> > > I don't think we need vmalloc for this now.
> >
> > Really? When is a better time to do it?
> >
> > I think the time creating a task is the best time to initialize it. No?
>
> The place is fine, but I would use kmalloc() now (and subsequently kfree
> on the other end) for the allocation. Its not _that_ large anymore,
> right?
Did you mean that? OK, I will do it.
Thank you.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 01:45:07PM +0100, Peter Zijlstra wrote: > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > > + /* > > +* struct held_lock does not have an indicator whether in nmi. > > +*/ > > + int nmi; > > Do we really need this? Lockdep doesn't really know about NMI context, > so its weird to now partially introduce it. That is, see how nmi_enter() includes lockdep_off().
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 01:45:07PM +0100, Peter Zijlstra wrote: > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > > + /* > > +* struct held_lock does not have an indicator whether in nmi. > > +*/ > > + int nmi; > > Do we really need this? Lockdep doesn't really know about NMI context, > so its weird to now partially introduce it. That is, see how nmi_enter() includes lockdep_off().
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 10:28:20PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 02:05:13PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > +#define MAX_XHLOCKS_NR 64UL
> >
> > > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > + if (tsk->xhlocks) {
> > > + void *tmp = tsk->xhlocks;
> > > + /* Disable crossrelease for current */
> > > + tsk->xhlocks = NULL;
> > > + vfree(tmp);
> > > + }
> > > +#endif
> >
> > > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > + p->xhlock_idx = 0;
> > > + p->xhlock_idx_soft = 0;
> > > + p->xhlock_idx_hard = 0;
> > > + p->xhlock_idx_nmi = 0;
> > > + p->xhlocks = vzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR);
> >
> > I don't think we need vmalloc for this now.
>
> Really? When is a better time to do it?
>
> I think the time creating a task is the best time to initialize it. No?
The place is fine, but I would use kmalloc() now (and subsequently kfree
on the other end) for the allocation. Its not _that_ large anymore,
right?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 10:28:20PM +0900, Byungchul Park wrote:
> On Tue, Feb 28, 2017 at 02:05:13PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > > +#define MAX_XHLOCKS_NR 64UL
> >
> > > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > + if (tsk->xhlocks) {
> > > + void *tmp = tsk->xhlocks;
> > > + /* Disable crossrelease for current */
> > > + tsk->xhlocks = NULL;
> > > + vfree(tmp);
> > > + }
> > > +#endif
> >
> > > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > > + p->xhlock_idx = 0;
> > > + p->xhlock_idx_soft = 0;
> > > + p->xhlock_idx_hard = 0;
> > > + p->xhlock_idx_nmi = 0;
> > > + p->xhlocks = vzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR);
> >
> > I don't think we need vmalloc for this now.
>
> Really? When is a better time to do it?
>
> I think the time creating a task is the best time to initialize it. No?
The place is fine, but I would use kmalloc() now (and subsequently kfree
on the other end) for the allocation. Its not _that_ large anymore,
right?
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 02:05:13PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > +#define MAX_XHLOCKS_NR 64UL
>
> > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > + if (tsk->xhlocks) {
> > + void *tmp = tsk->xhlocks;
> > + /* Disable crossrelease for current */
> > + tsk->xhlocks = NULL;
> > + vfree(tmp);
> > + }
> > +#endif
>
> > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > + p->xhlock_idx = 0;
> > + p->xhlock_idx_soft = 0;
> > + p->xhlock_idx_hard = 0;
> > + p->xhlock_idx_nmi = 0;
> > + p->xhlocks = vzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR);
>
> I don't think we need vmalloc for this now.
Really? When is a better time to do it?
I think the time creating a task is the best time to initialize it. No?
>
> > + p->work_id = 0;
> > +#endif
>
> > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > + if (p->xhlocks) {
> > + void *tmp = p->xhlocks;
> > + /* Diable crossrelease for current */
> > + p->xhlocks = NULL;
> > + vfree(tmp);
> > + }
> > +#endif
>
> Second instance of the same code, which would suggest using a function
> for this. Also, with a function you can loose the #ifdeffery.
Yes. It looks much better.
Thank you very much.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Tue, Feb 28, 2017 at 02:05:13PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> > +#define MAX_XHLOCKS_NR 64UL
>
> > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > + if (tsk->xhlocks) {
> > + void *tmp = tsk->xhlocks;
> > + /* Disable crossrelease for current */
> > + tsk->xhlocks = NULL;
> > + vfree(tmp);
> > + }
> > +#endif
>
> > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > + p->xhlock_idx = 0;
> > + p->xhlock_idx_soft = 0;
> > + p->xhlock_idx_hard = 0;
> > + p->xhlock_idx_nmi = 0;
> > + p->xhlocks = vzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR);
>
> I don't think we need vmalloc for this now.
Really? When is a better time to do it?
I think the time creating a task is the best time to initialize it. No?
>
> > + p->work_id = 0;
> > +#endif
>
> > +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> > + if (p->xhlocks) {
> > + void *tmp = p->xhlocks;
> > + /* Diable crossrelease for current */
> > + p->xhlocks = NULL;
> > + vfree(tmp);
> > + }
> > +#endif
>
> Second instance of the same code, which would suggest using a function
> for this. Also, with a function you can loose the #ifdeffery.
Yes. It looks much better.
Thank you very much.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> +#define MAX_XHLOCKS_NR 64UL
> +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> + if (tsk->xhlocks) {
> + void *tmp = tsk->xhlocks;
> + /* Disable crossrelease for current */
> + tsk->xhlocks = NULL;
> + vfree(tmp);
> + }
> +#endif
> +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> + p->xhlock_idx = 0;
> + p->xhlock_idx_soft = 0;
> + p->xhlock_idx_hard = 0;
> + p->xhlock_idx_nmi = 0;
> + p->xhlocks = vzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR);
I don't think we need vmalloc for this now.
> + p->work_id = 0;
> +#endif
> +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> + if (p->xhlocks) {
> + void *tmp = p->xhlocks;
> + /* Diable crossrelease for current */
> + p->xhlocks = NULL;
> + vfree(tmp);
> + }
> +#endif
Second instance of the same code, which would suggest using a function
for this. Also, with a function you can loose the #ifdeffery.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:
> +#define MAX_XHLOCKS_NR 64UL
> +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> + if (tsk->xhlocks) {
> + void *tmp = tsk->xhlocks;
> + /* Disable crossrelease for current */
> + tsk->xhlocks = NULL;
> + vfree(tmp);
> + }
> +#endif
> +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> + p->xhlock_idx = 0;
> + p->xhlock_idx_soft = 0;
> + p->xhlock_idx_hard = 0;
> + p->xhlock_idx_nmi = 0;
> + p->xhlocks = vzalloc(sizeof(struct hist_lock) * MAX_XHLOCKS_NR);
I don't think we need vmalloc for this now.
> + p->work_id = 0;
> +#endif
> +#ifdef CONFIG_LOCKDEP_CROSSRELEASE
> + if (p->xhlocks) {
> + void *tmp = p->xhlocks;
> + /* Diable crossrelease for current */
> + p->xhlocks = NULL;
> + vfree(tmp);
> + }
> +#endif
Second instance of the same code, which would suggest using a function
for this. Also, with a function you can loose the #ifdeffery.
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.
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.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > + /* > + * struct held_lock does not have an indicator whether in nmi. > + */ > + int nmi; Do we really need this? Lockdep doesn't really know about NMI context, so its weird to now partially introduce it.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > + /* > + * struct held_lock does not have an indicator whether in nmi. > + */ > + int nmi; Do we really need this? Lockdep doesn't really know about NMI context, so its weird to now partially introduce it.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > + /* > + * 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. > + * > + * TODO: We can also add dependencies between two acquisitions > + * of different work_id, if they don't cause a sleep so make > + * the worker stalled. > + */ > + unsigned intwork_id; I don't understand... please explain.
Re: [PATCH v5 06/13] lockdep: Implement crossrelease feature
On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote: > + /* > + * 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. > + * > + * TODO: We can also add dependencies between two acquisitions > + * of different work_id, if they don't cause a sleep so make > + * the worker stalled. > + */ > + unsigned intwork_id; I don't understand... please explain.
