| Message ID | 1402543307-29800-3-git-send-email-iamjoonsoo.kim@lge.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Joonsoo Kim <iamjoonsoo.kim@lge.com> writes: > We should free memory for bitmap when we find zone mis-match, > otherwise this memory will leak. > > Additionally, I copy code comment from ppc kvm's cma code to notify > why we need to check zone mis-match. > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > > diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c > index bd0bb81..fb0cdce 100644 > --- a/drivers/base/dma-contiguous.c > +++ b/drivers/base/dma-contiguous.c > @@ -177,14 +177,24 @@ static int __init cma_activate_area(struct cma *cma) > base_pfn = pfn; > for (j = pageblock_nr_pages; j; --j, pfn++) { > WARN_ON_ONCE(!pfn_valid(pfn)); > + /* > + * alloc_contig_range requires the pfn range > + * specified to be in the same zone. Make this > + * simple by forcing the entire CMA resv range > + * to be in the same zone. > + */ > if (page_zone(pfn_to_page(pfn)) != zone) > - return -EINVAL; > + goto err; > } > init_cma_reserved_pageblock(pfn_to_page(base_pfn)); > } while (--i); > > mutex_init(&cma->lock); > return 0; > + > +err: > + kfree(cma->bitmap); > + return -EINVAL; > } > > static struct cma cma_areas[MAX_CMA_AREAS]; > -- > 1.7.9.5
On Thu, Jun 12, 2014 at 12:21:39PM +0900, Joonsoo Kim wrote: > We should free memory for bitmap when we find zone mis-match, > otherwise this memory will leak. Then, -stable stuff? > > Additionally, I copy code comment from ppc kvm's cma code to notify > why we need to check zone mis-match. > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c > index bd0bb81..fb0cdce 100644 > --- a/drivers/base/dma-contiguous.c > +++ b/drivers/base/dma-contiguous.c > @@ -177,14 +177,24 @@ static int __init cma_activate_area(struct cma *cma) > base_pfn = pfn; > for (j = pageblock_nr_pages; j; --j, pfn++) { > WARN_ON_ONCE(!pfn_valid(pfn)); > + /* > + * alloc_contig_range requires the pfn range > + * specified to be in the same zone. Make this > + * simple by forcing the entire CMA resv range > + * to be in the same zone. > + */ > if (page_zone(pfn_to_page(pfn)) != zone) > - return -EINVAL; > + goto err; At a first glance, I thought it would be better to handle such error before activating. So when I see the registration code(ie, dma_contiguous_revere_area), I realized it is impossible because we didn't set up zone yet. :( If so, when we detect to fail here, it would be better to report more meaningful error message like what was successful zone and what is new zone and failed pfn number? > } > init_cma_reserved_pageblock(pfn_to_page(base_pfn)); > } while (--i); > > mutex_init(&cma->lock); > return 0; > + > +err: > + kfree(cma->bitmap); > + return -EINVAL; > } > > static struct cma cma_areas[MAX_CMA_AREAS]; > -- > 1.7.9.5
On Thu, Jun 12, 2014 at 02:25:43PM +0900, Minchan Kim wrote: > On Thu, Jun 12, 2014 at 12:21:39PM +0900, Joonsoo Kim wrote: > > We should free memory for bitmap when we find zone mis-match, > > otherwise this memory will leak. > > Then, -stable stuff? I don't think so. This is just possible leak candidate, so we don't need to push this to stable tree. > > > > > Additionally, I copy code comment from ppc kvm's cma code to notify > > why we need to check zone mis-match. > > > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > > diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c > > index bd0bb81..fb0cdce 100644 > > --- a/drivers/base/dma-contiguous.c > > +++ b/drivers/base/dma-contiguous.c > > @@ -177,14 +177,24 @@ static int __init cma_activate_area(struct cma *cma) > > base_pfn = pfn; > > for (j = pageblock_nr_pages; j; --j, pfn++) { > > WARN_ON_ONCE(!pfn_valid(pfn)); > > + /* > > + * alloc_contig_range requires the pfn range > > + * specified to be in the same zone. Make this > > + * simple by forcing the entire CMA resv range > > + * to be in the same zone. > > + */ > > if (page_zone(pfn_to_page(pfn)) != zone) > > - return -EINVAL; > > + goto err; > > At a first glance, I thought it would be better to handle such error > before activating. > So when I see the registration code(ie, dma_contiguous_revere_area), > I realized it is impossible because we didn't set up zone yet. :( > > If so, when we detect to fail here, it would be better to report more > meaningful error message like what was successful zone and what is > new zone and failed pfn number? What I want to do in early phase of this patchset is to make cma code on DMA APIs similar to ppc kvm's cma code. ppc kvm's cma code already has this error handling logic, so I make this patch. If we think that we need more things, we can do that on general cma code after merging this patchset. Thanks.
On 06/12/2014 02:02 PM, Joonsoo Kim wrote: > On Thu, Jun 12, 2014 at 02:25:43PM +0900, Minchan Kim wrote: >> On Thu, Jun 12, 2014 at 12:21:39PM +0900, Joonsoo Kim wrote: >>> We should free memory for bitmap when we find zone mis-match, >>> otherwise this memory will leak. >> >> Then, -stable stuff? > > I don't think so. This is just possible leak candidate, so we don't > need to push this to stable tree. > >> >>> >>> Additionally, I copy code comment from ppc kvm's cma code to notify >>> why we need to check zone mis-match. >>> >>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> >>> >>> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c >>> index bd0bb81..fb0cdce 100644 >>> --- a/drivers/base/dma-contiguous.c >>> +++ b/drivers/base/dma-contiguous.c >>> @@ -177,14 +177,24 @@ static int __init cma_activate_area(struct cma *cma) >>> base_pfn = pfn; >>> for (j = pageblock_nr_pages; j; --j, pfn++) { >>> WARN_ON_ONCE(!pfn_valid(pfn)); >>> + /* >>> + * alloc_contig_range requires the pfn range >>> + * specified to be in the same zone. Make this >>> + * simple by forcing the entire CMA resv range >>> + * to be in the same zone. >>> + */ >>> if (page_zone(pfn_to_page(pfn)) != zone) >>> - return -EINVAL; >>> + goto err; >> >> At a first glance, I thought it would be better to handle such error >> before activating. >> So when I see the registration code(ie, dma_contiguous_revere_area), >> I realized it is impossible because we didn't set up zone yet. :( >> >> If so, when we detect to fail here, it would be better to report more >> meaningful error message like what was successful zone and what is >> new zone and failed pfn number? > > What I want to do in early phase of this patchset is to make cma code > on DMA APIs similar to ppc kvm's cma code. ppc kvm's cma code already > has this error handling logic, so I make this patch. > > If we think that we need more things, we can do that on general cma code > after merging this patchset. > Yeah, I also like the idea. After all, this patchset aims to a general CMA management, we could improve more after this patchset. So Acked-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
On Thu, Jun 12 2014, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote: > We should free memory for bitmap when we find zone mis-match, > otherwise this memory will leak. > > Additionally, I copy code comment from ppc kvm's cma code to notify > why we need to check zone mis-match. > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> Reviewed-by: Michal Nazarewicz <mina86@mina86.com> > diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c > index bd0bb81..fb0cdce 100644 > --- a/drivers/base/dma-contiguous.c > +++ b/drivers/base/dma-contiguous.c > @@ -177,14 +177,24 @@ static int __init cma_activate_area(struct cma *cma) > base_pfn = pfn; > for (j = pageblock_nr_pages; j; --j, pfn++) { > WARN_ON_ONCE(!pfn_valid(pfn)); > + /* > + * alloc_contig_range requires the pfn range > + * specified to be in the same zone. Make this > + * simple by forcing the entire CMA resv range > + * to be in the same zone. > + */ > if (page_zone(pfn_to_page(pfn)) != zone) > - return -EINVAL; > + goto err; > } > init_cma_reserved_pageblock(pfn_to_page(base_pfn)); > } while (--i); > > mutex_init(&cma->lock); > return 0; > + > +err: > + kfree(cma->bitmap); > + return -EINVAL; > }
diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c index bd0bb81..fb0cdce 100644 --- a/drivers/base/dma-contiguous.c +++ b/drivers/base/dma-contiguous.c @@ -177,14 +177,24 @@ static int __init cma_activate_area(struct cma *cma) base_pfn = pfn; for (j = pageblock_nr_pages; j; --j, pfn++) { WARN_ON_ONCE(!pfn_valid(pfn)); + /* + * alloc_contig_range requires the pfn range + * specified to be in the same zone. Make this + * simple by forcing the entire CMA resv range + * to be in the same zone. + */ if (page_zone(pfn_to_page(pfn)) != zone) - return -EINVAL; + goto err; } init_cma_reserved_pageblock(pfn_to_page(base_pfn)); } while (--i); mutex_init(&cma->lock); return 0; + +err: + kfree(cma->bitmap); + return -EINVAL; } static struct cma cma_areas[MAX_CMA_AREAS];
We should free memory for bitmap when we find zone mis-match, otherwise this memory will leak. Additionally, I copy code comment from ppc kvm's cma code to notify why we need to check zone mis-match. Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>