* [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free @ 2019-01-22 10:15 hans 2019-01-22 19:48 ` Matias Bjørling 2019-01-24 13:19 ` Javier González 0 siblings, 2 replies; 5+ messages in thread From: hans @ 2019-01-22 10:15 UTC (permalink / raw) To: Matias Bjorling; +Cc: javier, linux-block, linux-kernel, Hans Holmberg From: Hans Holmberg <hans.holmberg@cnexlabs.com> pblk_line_meta_free might sleep (it can end up calling vfree, depending on how we allocate lba lists), and this can lead to a BUG() if we wake up on a different cpu and release the lock. As there is no point of grabbing the free lock when pblk has shut down, remove the lock. Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> --- drivers/lightnvm/pblk-init.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c index f9a3e47b6a93..eb0135c77805 100644 --- a/drivers/lightnvm/pblk-init.c +++ b/drivers/lightnvm/pblk-init.c @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk) struct pblk_line *line; int i; - spin_lock(&l_mg->free_lock); for (i = 0; i < l_mg->nr_lines; i++) { line = &pblk->lines[i]; pblk_line_free(line); pblk_line_meta_free(l_mg, line); } - spin_unlock(&l_mg->free_lock); pblk_line_mg_free(pblk); -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free 2019-01-22 10:15 [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free hans @ 2019-01-22 19:48 ` Matias Bjørling 2019-01-24 13:19 ` Javier González 1 sibling, 0 replies; 5+ messages in thread From: Matias Bjørling @ 2019-01-22 19:48 UTC (permalink / raw) To: hans; +Cc: javier, linux-block, linux-kernel, Hans Holmberg On 1/22/19 11:15 AM, hans@owltronix.com wrote: > From: Hans Holmberg <hans.holmberg@cnexlabs.com> > > pblk_line_meta_free might sleep (it can end up calling vfree, depending > on how we allocate lba lists), and this can lead to a BUG() > if we wake up on a different cpu and release the lock. > > As there is no point of grabbing the free lock when pblk has shut down, > remove the lock. > > Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> > --- > drivers/lightnvm/pblk-init.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c > index f9a3e47b6a93..eb0135c77805 100644 > --- a/drivers/lightnvm/pblk-init.c > +++ b/drivers/lightnvm/pblk-init.c > @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk) > struct pblk_line *line; > int i; > > - spin_lock(&l_mg->free_lock); > for (i = 0; i < l_mg->nr_lines; i++) { > line = &pblk->lines[i]; > > pblk_line_free(line); > pblk_line_meta_free(l_mg, line); > } > - spin_unlock(&l_mg->free_lock); > > pblk_line_mg_free(pblk); > > Thanks Hans. Applied for 5.1. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free 2019-01-22 10:15 [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free hans 2019-01-22 19:48 ` Matias Bjørling @ 2019-01-24 13:19 ` Javier González 2019-01-25 9:15 ` Hans Holmberg 1 sibling, 1 reply; 5+ messages in thread From: Javier González @ 2019-01-24 13:19 UTC (permalink / raw) To: Hans Holmberg Cc: Matias Bjørling, linux-block, linux-kernel, Hans Holmberg [-- Attachment #1: Type: text/plain, Size: 1325 bytes --] > On 22 Jan 2019, at 11.15, hans@owltronix.com wrote: > > From: Hans Holmberg <hans.holmberg@cnexlabs.com> > > pblk_line_meta_free might sleep (it can end up calling vfree, depending > on how we allocate lba lists), and this can lead to a BUG() > if we wake up on a different cpu and release the lock. > > As there is no point of grabbing the free lock when pblk has shut down, > remove the lock. > > Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> > --- > drivers/lightnvm/pblk-init.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c > index f9a3e47b6a93..eb0135c77805 100644 > --- a/drivers/lightnvm/pblk-init.c > +++ b/drivers/lightnvm/pblk-init.c > @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk) > struct pblk_line *line; > int i; > > - spin_lock(&l_mg->free_lock); > for (i = 0; i < l_mg->nr_lines; i++) { > line = &pblk->lines[i]; > > pblk_line_free(line); > pblk_line_meta_free(l_mg, line); > } > - spin_unlock(&l_mg->free_lock); > > pblk_line_mg_free(pblk); > > -- > 2.17.1 Can you add a comment too indicating that this is only safe on a single threaded shutdown? Otherwise the patch looks good. Reviewed-by: Javier González <javier@javigon.com> [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free 2019-01-24 13:19 ` Javier González @ 2019-01-25 9:15 ` Hans Holmberg 2019-01-25 9:37 ` Javier González 0 siblings, 1 reply; 5+ messages in thread From: Hans Holmberg @ 2019-01-25 9:15 UTC (permalink / raw) To: Javier González Cc: Matias Bjørling, linux-block, Linux Kernel Mailing List, Hans Holmberg On Thu, Jan 24, 2019 at 2:19 PM Javier González <javier@javigon.com> wrote: > > > On 22 Jan 2019, at 11.15, hans@owltronix.com wrote: > > > > From: Hans Holmberg <hans.holmberg@cnexlabs.com> > > > > pblk_line_meta_free might sleep (it can end up calling vfree, depending > > on how we allocate lba lists), and this can lead to a BUG() > > if we wake up on a different cpu and release the lock. > > > > As there is no point of grabbing the free lock when pblk has shut down, > > remove the lock. > > > > Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> > > --- > > drivers/lightnvm/pblk-init.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c > > index f9a3e47b6a93..eb0135c77805 100644 > > --- a/drivers/lightnvm/pblk-init.c > > +++ b/drivers/lightnvm/pblk-init.c > > @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk) > > struct pblk_line *line; > > int i; > > > > - spin_lock(&l_mg->free_lock); > > for (i = 0; i < l_mg->nr_lines; i++) { > > line = &pblk->lines[i]; > > > > pblk_line_free(line); > > pblk_line_meta_free(l_mg, line); > > } > > - spin_unlock(&l_mg->free_lock); > > > > pblk_line_mg_free(pblk); > > > > -- > > 2.17.1 > > Can you add a comment too indicating that this is only safe on a single > threaded shutdown? To be able to free the lines, we need have stopped anything accessing the lines first. That seems obvious to me. It would be nice to make a pass over the code and document pblk's locking(and other concurrency handling, like the line krefs) though. Thanks, Hans > > Otherwise the patch looks good. > > Reviewed-by: Javier González <javier@javigon.com> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free 2019-01-25 9:15 ` Hans Holmberg @ 2019-01-25 9:37 ` Javier González 0 siblings, 0 replies; 5+ messages in thread From: Javier González @ 2019-01-25 9:37 UTC (permalink / raw) To: Hans Holmberg Cc: Matias Bjørling, linux-block, Linux Kernel Mailing List, Hans Holmberg [-- Attachment #1: Type: text/plain, Size: 2767 bytes --] > On 25 Jan 2019, at 10.15, Hans Holmberg <hans.ml.holmberg@owltronix.com> wrote: > > On Thu, Jan 24, 2019 at 2:19 PM Javier González <javier@javigon.com> wrote: >>> On 22 Jan 2019, at 11.15, hans@owltronix.com wrote: >>> >>> From: Hans Holmberg <hans.holmberg@cnexlabs.com> >>> >>> pblk_line_meta_free might sleep (it can end up calling vfree, depending >>> on how we allocate lba lists), and this can lead to a BUG() >>> if we wake up on a different cpu and release the lock. >>> >>> As there is no point of grabbing the free lock when pblk has shut down, >>> remove the lock. >>> >>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> >>> --- >>> drivers/lightnvm/pblk-init.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c >>> index f9a3e47b6a93..eb0135c77805 100644 >>> --- a/drivers/lightnvm/pblk-init.c >>> +++ b/drivers/lightnvm/pblk-init.c >>> @@ -584,14 +584,12 @@ static void pblk_lines_free(struct pblk *pblk) >>> struct pblk_line *line; >>> int i; >>> >>> - spin_lock(&l_mg->free_lock); >>> for (i = 0; i < l_mg->nr_lines; i++) { >>> line = &pblk->lines[i]; >>> >>> pblk_line_free(line); >>> pblk_line_meta_free(l_mg, line); >>> } >>> - spin_unlock(&l_mg->free_lock); >>> >>> pblk_line_mg_free(pblk); >>> >>> -- >>> 2.17.1 >> >> Can you add a comment too indicating that this is only safe on a single >> threaded shutdown? > > To be able to free the lines, we need have stopped anything accessing > the lines first. That seems obvious to me. > The reason I mention this is that there is assumptions on the shutdown logic that the line freeup will be single threaded - you can see that we do not lock pblk_line_mg_free() either, for the same reason as you are removing this lock. We can do this is only because we have a single open line at the time (which in close down we fill up in parallel to speed up the process BTW). If we have more open lines, it would be desirable to close them in parallel. At this point we either have a sync point when they are all closed and then free, or we allow them to free themselves. In the second case, locking will be necessary. IMHO, a comment does not hurt. > It would be nice to make a pass over the code and document pblk's > locking(and other concurrency handling, like the line krefs) though. > True that - krefs specially would deserve some documentation. Let me see if I can allocate some time the following weeks to write up some documentation. > Thanks, > Hans > >> Otherwise the patch looks good. >> >> Reviewed-by: Javier González <javier@javigon.com> [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-25 9:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-22 10:15 [PATCH] lightnvm: pblk: stop taking the free lock in in pblk_lines_free hans 2019-01-22 19:48 ` Matias Bjørling 2019-01-24 13:19 ` Javier González 2019-01-25 9:15 ` Hans Holmberg 2019-01-25 9:37 ` Javier González
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.