| Message ID | 20190122101531.30893-1-hans@owltronix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lightnvm: pblk: stop taking the free lock in in pblk_lines_free | expand |
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.
> 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>
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> >
> 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>
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);