| Message ID | 1507038524-24587-1-git-send-email-hans.ml.holmberg@owltronix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/03/2017 04:48 PM, Hans Holmberg wrote: > From: Hans Holmberg <hans.holmberg@cnexlabs.com> > > The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to > call in atomic context (unless the call came from an interrupt) and > introduced vfree_atomic that is safe to call in atomic context. > > So, since we're holding locks when freeing line metadata, we need to > use the atomic version of vfree. > > Fix this by introducing an atomic variant of pblk_mfree and > switching to that in pblk_line_meta_free. > > Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> > --- > > The patch is for: > https://github.com/OpenChannelSSD/linux branch for-4.15/pblk > > drivers/lightnvm/pblk-init.c | 3 ++- > drivers/lightnvm/pblk.h | 8 ++++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c > index c452478..3a191a6 100644 > --- a/drivers/lightnvm/pblk-init.c > +++ b/drivers/lightnvm/pblk-init.c > @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk) > spin_lock(&l_mg->free_lock); What's the point in holding ->free_lock here? It seems like it could be just dropped. > for (i = 0; i < PBLK_DATA_LINES; i++) { > kfree(l_mg->sline_meta[i]); > - pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); > + pblk_mfree_atomic(l_mg->eline_meta[i]->buf, > + l_mg->emeta_alloc_type); > kfree(l_mg->eline_meta[i]); > } > spin_unlock(&l_mg->free_lock); > diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h > index 03965da..93f98e3 100644 > --- a/drivers/lightnvm/pblk.h > +++ b/drivers/lightnvm/pblk.h > @@ -881,6 +881,14 @@ static inline void pblk_mfree(void *ptr, int type) > vfree(ptr); > } > > +static inline void pblk_mfree_atomic(void *ptr, int type) > +{ > + if (type == PBLK_KMALLOC_META) > + kfree(ptr); > + else > + vfree_atomic(ptr); > +} > + > static inline struct nvm_rq *nvm_rq_from_c_ctx(void *c_ctx) > { > return c_ctx - sizeof(struct nvm_rq); >
> On 3 Oct 2017, at 16.07, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > > > > On 10/03/2017 04:48 PM, Hans Holmberg wrote: >> From: Hans Holmberg <hans.holmberg@cnexlabs.com> >> >> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to >> call in atomic context (unless the call came from an interrupt) and >> introduced vfree_atomic that is safe to call in atomic context. >> >> So, since we're holding locks when freeing line metadata, we need to >> use the atomic version of vfree. >> >> Fix this by introducing an atomic variant of pblk_mfree and >> switching to that in pblk_line_meta_free. >> >> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> >> --- >> >> The patch is for: >> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk >> >> drivers/lightnvm/pblk-init.c | 3 ++- >> drivers/lightnvm/pblk.h | 8 ++++++++ >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c >> index c452478..3a191a6 100644 >> --- a/drivers/lightnvm/pblk-init.c >> +++ b/drivers/lightnvm/pblk-init.c >> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk) >> spin_lock(&l_mg->free_lock); > > What's the point in holding ->free_lock here? It seems like it could be just dropped. > This lock can indeed be dropped, but the general pblk semaphore, which serializes initialization and tear down cannot. This is taken on pblk_exit(). Javier
On 10/03/2017 05:11 PM, Javier González wrote: >> On 3 Oct 2017, at 16.07, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: >> >> >> >> On 10/03/2017 04:48 PM, Hans Holmberg wrote: >>> From: Hans Holmberg <hans.holmberg@cnexlabs.com> >>> >>> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to >>> call in atomic context (unless the call came from an interrupt) and >>> introduced vfree_atomic that is safe to call in atomic context. >>> >>> So, since we're holding locks when freeing line metadata, we need to >>> use the atomic version of vfree. >>> >>> Fix this by introducing an atomic variant of pblk_mfree and >>> switching to that in pblk_line_meta_free. >>> >>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> >>> --- >>> >>> The patch is for: >>> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk >>> >>> drivers/lightnvm/pblk-init.c | 3 ++- >>> drivers/lightnvm/pblk.h | 8 ++++++++ >>> 2 files changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c >>> index c452478..3a191a6 100644 >>> --- a/drivers/lightnvm/pblk-init.c >>> +++ b/drivers/lightnvm/pblk-init.c >>> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk) >>> spin_lock(&l_mg->free_lock); >> >> What's the point in holding ->free_lock here? It seems like it could be just dropped. >> > > This lock can indeed be dropped, So, let's do this. This would be the best way to fix this. > but the general pblk semaphore, which > serializes initialization and tear down cannot. This is taken on > pblk_exit(). > But semaphore is not the problem here. We can sleep under semaphore, so it's fine. > Javier >
> On 3 Oct 2017, at 16.20, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > > > > On 10/03/2017 05:11 PM, Javier González wrote: >>> On 3 Oct 2017, at 16.07, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: >>> >>> >>> >>> On 10/03/2017 04:48 PM, Hans Holmberg wrote: >>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com> >>>> >>>> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to >>>> call in atomic context (unless the call came from an interrupt) and >>>> introduced vfree_atomic that is safe to call in atomic context. >>>> >>>> So, since we're holding locks when freeing line metadata, we need to >>>> use the atomic version of vfree. >>>> >>>> Fix this by introducing an atomic variant of pblk_mfree and >>>> switching to that in pblk_line_meta_free. >>>> >>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> >>>> --- >>>> >>>> The patch is for: >>>> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk >>>> >>>> drivers/lightnvm/pblk-init.c | 3 ++- >>>> drivers/lightnvm/pblk.h | 8 ++++++++ >>>> 2 files changed, 10 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c >>>> index c452478..3a191a6 100644 >>>> --- a/drivers/lightnvm/pblk-init.c >>>> +++ b/drivers/lightnvm/pblk-init.c >>>> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk) >>>> spin_lock(&l_mg->free_lock); >>> >>> What's the point in holding ->free_lock here? It seems like it could be just dropped. >> >> This lock can indeed be dropped, > > So, let's do this. This would be the best way to fix this. > >> but the general pblk semaphore, which >> serializes initialization and tear down cannot. This is taken on >> pblk_exit(). > > But semaphore is not the problem here. We can sleep under semaphore, so it's fine. > It seems to me like a false positive, but lockdep complains on the mentioned rw_semaphore held by pblk, and on the mutex held by the lightnvm subsystem when removing a target (dev->mlock). [ 6037.778889] BUG: sleeping function called from invalid context at mm/vmalloc.c:1492 [ 6037.786579] in_atomic(): 1, irqs_disabled(): 0, pid: 1282, name: nvme [ 6037.793050] 3 locks held by nvme/1282: [ 6037.793053] #0: (&dev->mlock){+.+.+.}, at: [<ffffffff8ddff395>] nvm_ctl_ioctl+0x3c5/0x6a0 [ 6037.793075] #1: (pblk_lock){+.+.+.}, at: [<ffffffff8de0439b>] pblk_exit+0x1b/0x100 [ 6037.793092] #2: (&(&l_mg->free_lock)->rlock){+.+...}, at: [<ffffffff8de040ea>] pblk_line_meta_free+0x8a/0x130 Any ideas? Thanks, Javier
> On 3 Oct 2017, at 16.43, Javier González <jg@lightnvm.io> wrote: > >> On 3 Oct 2017, at 16.20, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: >> >> >> >> On 10/03/2017 05:11 PM, Javier González wrote: >>>> On 3 Oct 2017, at 16.07, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: >>>> >>>> >>>> >>>> On 10/03/2017 04:48 PM, Hans Holmberg wrote: >>>>> From: Hans Holmberg <hans.holmberg@cnexlabs.com> >>>>> >>>>> The commit bf22e37a6413 ("mm: add vfree_atomic()") made vfree unsafe to >>>>> call in atomic context (unless the call came from an interrupt) and >>>>> introduced vfree_atomic that is safe to call in atomic context. >>>>> >>>>> So, since we're holding locks when freeing line metadata, we need to >>>>> use the atomic version of vfree. >>>>> >>>>> Fix this by introducing an atomic variant of pblk_mfree and >>>>> switching to that in pblk_line_meta_free. >>>>> >>>>> Signed-off-by: Hans Holmberg <hans.holmberg@cnexlabs.com> >>>>> --- >>>>> >>>>> The patch is for: >>>>> https://github.com/OpenChannelSSD/linux branch for-4.15/pblk >>>>> >>>>> drivers/lightnvm/pblk-init.c | 3 ++- >>>>> drivers/lightnvm/pblk.h | 8 ++++++++ >>>>> 2 files changed, 10 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c >>>>> index c452478..3a191a6 100644 >>>>> --- a/drivers/lightnvm/pblk-init.c >>>>> +++ b/drivers/lightnvm/pblk-init.c >>>>> @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk) >>>>> spin_lock(&l_mg->free_lock); >>>> >>>> What's the point in holding ->free_lock here? It seems like it could be just dropped. >>> >>> This lock can indeed be dropped, >> >> So, let's do this. This would be the best way to fix this. >> >>> but the general pblk semaphore, which >>> serializes initialization and tear down cannot. This is taken on >>> pblk_exit(). >> >> But semaphore is not the problem here. We can sleep under semaphore, so it's fine. > > It seems to me like a false positive, but lockdep complains on the > mentioned rw_semaphore held by pblk, and on the mutex held by the > lightnvm subsystem when removing a target (dev->mlock). > > [ 6037.778889] BUG: sleeping function called from invalid context at mm/vmalloc.c:1492 > [ 6037.786579] in_atomic(): 1, irqs_disabled(): 0, pid: 1282, name: nvme > [ 6037.793050] 3 locks held by nvme/1282: > [ 6037.793053] #0: (&dev->mlock){+.+.+.}, at: [<ffffffff8ddff395>] nvm_ctl_ioctl+0x3c5/0x6a0 > [ 6037.793075] #1: (pblk_lock){+.+.+.}, at: [<ffffffff8de0439b>] pblk_exit+0x1b/0x100 > [ 6037.793092] #2: (&(&l_mg->free_lock)->rlock){+.+...}, at: [<ffffffff8de040ea>] pblk_line_meta_free+0x8a/0x130 > > Any ideas? > Ok. When dropping ->free_lock, lockdep does not complain. It's just a misleading notification from lockdep, signalling semaphores as "held locks" when a real non sleeping lock is being taken. We will just remove ->free_lock then. Thanks Andrey. Javier
Thanks for the review Andrey, i'll send a new patch removing the lock.
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c index c452478..3a191a6 100644 --- a/drivers/lightnvm/pblk-init.c +++ b/drivers/lightnvm/pblk-init.c @@ -396,7 +396,8 @@ static void pblk_line_meta_free(struct pblk *pblk) spin_lock(&l_mg->free_lock); for (i = 0; i < PBLK_DATA_LINES; i++) { kfree(l_mg->sline_meta[i]); - pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); + pblk_mfree_atomic(l_mg->eline_meta[i]->buf, + l_mg->emeta_alloc_type); kfree(l_mg->eline_meta[i]); } spin_unlock(&l_mg->free_lock); diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h index 03965da..93f98e3 100644 --- a/drivers/lightnvm/pblk.h +++ b/drivers/lightnvm/pblk.h @@ -881,6 +881,14 @@ static inline void pblk_mfree(void *ptr, int type) vfree(ptr); } +static inline void pblk_mfree_atomic(void *ptr, int type) +{ + if (type == PBLK_KMALLOC_META) + kfree(ptr); + else + vfree_atomic(ptr); +} + static inline struct nvm_rq *nvm_rq_from_c_ctx(void *c_ctx) { return c_ctx - sizeof(struct nvm_rq);