Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations

From: Tomasz Figa
Date: Wed May 31 2017 - 08:46:55 EST

Next message: Jeff Layton: "[PATCH v5 15/17] fs: add a write_one_page_since"
Previous message: Jeff Layton: "[PATCH v5 16/17] ext2: convert to errseq_t based writeback error tracking"
In reply to: Marek Szyprowski: "Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations"
Next in thread: Sakari Ailus: "Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, May 31, 2017 at 9:09 PM, Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> wrote:
> Hi Tomasz,
>
>
> On 2017-05-31 08:58, Tomasz Figa wrote:
>>
>> There are multiple places where arrays or otherwise variable sized
>> buffer are allocated through V4L2 core code, including things like
>> controls, memory pages, staging buffers for ioctls and so on. Such
>> allocations can potentially require an order > 0 allocation from the
>> page allocator, which is not guaranteed to be fulfilled and is likely to
>> fail on a system with severe memory fragmentation (e.g. a system with
>> very long uptime).
>>
>> Since the memory being allocated is intended to be used by the CPU
>> exclusively, we can consider using vmalloc() as a fallback and this is
>> exactly what the recently merged kvmalloc() helpers do. A kmalloc() call
>> is still attempted, even for order > 0 allocations, but it is done
>> with __GFP_NORETRY and __GFP_NOWARN, with expectation of failing if
>> requested memory is not available instantly. Only then the vmalloc()
>> fallback is used. This should give us fast and more reliable allocations
>> even on systems with higher memory pressure and/or more fragmentation,
>> while still retaining the same performance level on systems not
>> suffering from such conditions.
>>
>> While at it, replace explicit array size calculations on changed
>> allocations with kvmalloc_array().
>>
>> Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
>> ---
>> drivers/media/v4l2-core/v4l2-async.c | 4 ++--
>> drivers/media/v4l2-core/v4l2-ctrls.c | 25
>> +++++++++++++------------
>> drivers/media/v4l2-core/v4l2-event.c | 8 +++++---
>> drivers/media/v4l2-core/v4l2-ioctl.c | 6 +++---
>> drivers/media/v4l2-core/v4l2-subdev.c | 7 ++++---
>> drivers/media/v4l2-core/videobuf2-dma-sg.c | 8 ++++----
>
>
> For vb2:
> Acked-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>

Thanks!

>
> There are also a few vmalloc calls in old videobuf (v1) framework, which
> might be converted to kvmalloc if you have a few spare minutes to take
> a look.

I was intending to convert those as well, but on the other hand I
concluded that it's some very old code, which might be difficult to
test and likely to introduce some long undiscovered regressions. If
it's desired to update those as well, I can include those changes in
the non-RFC version.

Also FYI, this is more about converting k[mzc]alloc(_array)? into
kvmalloc, so that we avoid costly high order allocations. Already
existing vmalloc calls should be fine IMHO.

>
>
>> 6 files changed, 31 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c
>> b/drivers/media/v4l2-core/v4l2-async.c
>> index 96cc733f35ef..2d2d9f1f8831 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -204,7 +204,7 @@ void v4l2_async_notifier_unregister(struct
>> v4l2_async_notifier *notifier)
>> if (!notifier->v4l2_dev)
>> return;
>> - dev = kmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
>> + dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
>> if (!dev) {
>> dev_err(notifier->v4l2_dev->dev,
>> "Failed to allocate device cache!\n");
>> @@ -260,7 +260,7 @@ void v4l2_async_notifier_unregister(struct
>> v4l2_async_notifier *notifier)
>> }
>> put_device(d);
>> }
>> - kfree(dev);
>> + kvfree(dev);
>> notifier->v4l2_dev = NULL;
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index ec42872d11cf..88025527c67e 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -1745,8 +1745,9 @@ int v4l2_ctrl_handler_init_class(struct
>> v4l2_ctrl_handler *hdl,
>> INIT_LIST_HEAD(&hdl->ctrls);
>> INIT_LIST_HEAD(&hdl->ctrl_refs);
>> hdl->nr_of_buckets = 1 + nr_of_controls_hint / 8;
>> - hdl->buckets = kcalloc(hdl->nr_of_buckets,
>> sizeof(hdl->buckets[0]),
>> - GFP_KERNEL);
>> + hdl->buckets = kvmalloc_array(hdl->nr_of_buckets,
>> + sizeof(hdl->buckets[0]),
>> + GFP_KERNEL | __GFP_ZERO);
>> hdl->error = hdl->buckets ? 0 : -ENOMEM;
>> return hdl->error;
>> }
>> @@ -1773,9 +1774,9 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler
>> *hdl)
>> list_del(&ctrl->node);
>> list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs,
>> node)
>> list_del(&sev->node);
>> - kfree(ctrl);
>> + kvfree(ctrl);
>> }
>> - kfree(hdl->buckets);
>> + kvfree(hdl->buckets);
>> hdl->buckets = NULL;
>> hdl->cached = NULL;
>> hdl->error = 0;
>> @@ -2022,7 +2023,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
>> v4l2_ctrl_handler *hdl,
>> is_array)
>> sz_extra += 2 * tot_ctrl_size;
>> - ctrl = kzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
>> + ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
>> if (ctrl == NULL) {
>> handler_set_err(hdl, -ENOMEM);
>> return NULL;
>> @@ -2071,7 +2072,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct
>> v4l2_ctrl_handler *hdl,
>> }
>> if (handler_new_ref(hdl, ctrl)) {
>> - kfree(ctrl);
>> + kvfree(ctrl);
>> return NULL;
>> }
>> mutex_lock(hdl->lock);
>> @@ -2824,8 +2825,8 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>> struct v4l2_ext_controls *cs
>> return class_check(hdl, cs->which);
>> if (cs->count > ARRAY_SIZE(helper)) {
>> - helpers = kmalloc_array(cs->count, sizeof(helper[0]),
>> - GFP_KERNEL);
>> + helpers = kvmalloc_array(cs->count, sizeof(helper[0]),
>> + GFP_KERNEL);
>> if (helpers == NULL)
>> return -ENOMEM;
>> }
>> @@ -2877,7 +2878,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>> struct v4l2_ext_controls *cs
>> }
>> if (cs->count > ARRAY_SIZE(helper))
>> - kfree(helpers);
>> + kvfree(helpers);
>> return ret;
>> }
>> EXPORT_SYMBOL(v4l2_g_ext_ctrls);
>> @@ -3079,8 +3080,8 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
>> struct v4l2_ctrl_handler *hdl,
>> return class_check(hdl, cs->which);
>> if (cs->count > ARRAY_SIZE(helper)) {
>> - helpers = kmalloc_array(cs->count, sizeof(helper[0]),
>> - GFP_KERNEL);
>> + helpers = kvmalloc_array(cs->count, sizeof(helper[0]),
>> + GFP_KERNEL);
>> if (!helpers)
>> return -ENOMEM;
>> }
>> @@ -3157,7 +3158,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
>> struct v4l2_ctrl_handler *hdl,
>> }
>> if (cs->count > ARRAY_SIZE(helper))
>> - kfree(helpers);
>> + kvfree(helpers);
>> return ret;
>> }
>> diff --git a/drivers/media/v4l2-core/v4l2-event.c
>> b/drivers/media/v4l2-core/v4l2-event.c
>> index a75df6cb141f..5f072ef8ff57 100644
>> --- a/drivers/media/v4l2-core/v4l2-event.c
>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>> @@ -24,6 +24,7 @@
>> #include <linux/sched.h>
>> #include <linux/slab.h>
>> #include <linux/export.h>
>> +#include <linux/mm.h>
>> static unsigned sev_pos(const struct v4l2_subscribed_event *sev,
>> unsigned idx)
>> {
>> @@ -214,7 +215,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>> if (elems < 1)
>> elems = 1;
>> - sev = kzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
>> GFP_KERNEL);
>> + sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
>> + GFP_KERNEL);
>> if (!sev)
>> return -ENOMEM;
>> for (i = 0; i < elems; i++)
>> @@ -232,7 +234,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> if (found_ev) {
>> - kfree(sev);
>> + kvfree(sev);
>> return 0; /* Already listening */
>> }
>> @@ -304,7 +306,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>> if (sev && sev->ops && sev->ops->del)
>> sev->ops->del(sev);
>> - kfree(sev);
>> + kvfree(sev);
>> return 0;
>> }
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
>> b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index e5a2187381db..098e8be36ea6 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2811,7 +2811,7 @@ video_usercopy(struct file *file, unsigned int cmd,
>> unsigned long arg,
>> parg = sbuf;
>> } else {
>> /* too big to allocate from stack */
>> - mbuf = kmalloc(_IOC_SIZE(cmd), GFP_KERNEL);
>> + mbuf = kvmalloc(_IOC_SIZE(cmd), GFP_KERNEL);
>> if (NULL == mbuf)
>> return -ENOMEM;
>> parg = mbuf;
>> @@ -2858,7 +2858,7 @@ video_usercopy(struct file *file, unsigned int cmd,
>> unsigned long arg,
>> * array) fits into sbuf (so that mbuf will still remain
>> * unused up to here).
>> */
>> - mbuf = kmalloc(array_size, GFP_KERNEL);
>> + mbuf = kvmalloc(array_size, GFP_KERNEL);
>> err = -ENOMEM;
>> if (NULL == mbuf)
>> goto out_array_args;
>> @@ -2901,7 +2901,7 @@ video_usercopy(struct file *file, unsigned int cmd,
>> unsigned long arg,
>> }
>> out:
>> - kfree(mbuf);
>> + kvfree(mbuf);
>> return err;
>> }
>> EXPORT_SYMBOL(video_usercopy);
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>> b/drivers/media/v4l2-core/v4l2-subdev.c
>> index da78497ae5ed..053d06bb407d 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -577,13 +577,14 @@ v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd)
>> if (!sd->entity.num_pads)
>> return NULL;
>> - cfg = kcalloc(sd->entity.num_pads, sizeof(*cfg), GFP_KERNEL);
>> + cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg),
>> + GFP_KERNEL | __GFP_ZERO);
>> if (!cfg)
>> return NULL;
>> ret = v4l2_subdev_call(sd, pad, init_cfg, cfg);
>> if (ret < 0 && ret != -ENOIOCTLCMD) {
>> - kfree(cfg);
>> + kvfree(cfg);
>> return NULL;
>> }
>> @@ -593,7 +594,7 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config);
>> void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg)
>> {
>> - kfree(cfg);
>> + kvfree(cfg);
>> }
>> EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config);
>> #endif /* CONFIG_MEDIA_CONTROLLER */
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c
>> b/drivers/media/v4l2-core/videobuf2-dma-sg.c
>> index 8e8798a74760..5defa1f22ca2 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
>> @@ -120,8 +120,8 @@ static void *vb2_dma_sg_alloc(struct device *dev,
>> unsigned long dma_attrs,
>> buf->num_pages = size >> PAGE_SHIFT;
>> buf->dma_sgt = &buf->sg_table;
>> - buf->pages = kzalloc(buf->num_pages * sizeof(struct page *),
>> - GFP_KERNEL);
>> + buf->pages = kvmalloc_array(buf->num_pages, sizeof(struct page *),
>> + GFP_KERNEL | __GFP_ZERO);
>> if (!buf->pages)
>> goto fail_pages_array_alloc;
>> @@ -165,7 +165,7 @@ static void *vb2_dma_sg_alloc(struct device *dev,
>> unsigned long dma_attrs,
>> while (num_pages--)
>> __free_page(buf->pages[num_pages]);
>> fail_pages_alloc:
>> - kfree(buf->pages);
>> + kvfree(buf->pages);
>> fail_pages_array_alloc:
>> kfree(buf);
>> return ERR_PTR(-ENOMEM);
>> @@ -187,7 +187,7 @@ static void vb2_dma_sg_put(void *buf_priv)
>> sg_free_table(buf->dma_sgt);
>> while (--i >= 0)
>> __free_page(buf->pages[i]);
>> - kfree(buf->pages);
>> + kvfree(buf->pages);
>> put_device(buf->dev);
>> kfree(buf);
>> }
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Next message: Jeff Layton: "[PATCH v5 15/17] fs: add a write_one_page_since"
Previous message: Jeff Layton: "[PATCH v5 16/17] ext2: convert to errseq_t based writeback error tracking"
In reply to: Marek Szyprowski: "Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations"
Next in thread: Sakari Ailus: "Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]