linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations
@ 2017-05-31  6:58 ` Tomasz Figa
  2017-05-31 12:09   ` Marek Szyprowski
  0 siblings, 1 reply; 5+ messages in thread
From: Tomasz Figa @ 2017-05-31  6:58 UTC (permalink / raw)
  To: linux-media
  Cc: linux-kernel, Mauro Carvalho Chehab, Pawel Osciak,
	Marek Szyprowski, Hans Verkuil, Laurent Pinchart, Sakari Ailus,
	Tomasz Figa

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@chromium.org>
---
 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 ++++----
 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);
 	}
-- 
2.13.0.219.gdb65acc882-goog

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations
  2017-05-31  6:58 ` [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations Tomasz Figa
@ 2017-05-31 12:09   ` Marek Szyprowski
  2017-05-31 12:46     ` Tomasz Figa
  0 siblings, 1 reply; 5+ messages in thread
From: Marek Szyprowski @ 2017-05-31 12:09 UTC (permalink / raw)
  To: Tomasz Figa, linux-media
  Cc: linux-kernel, Mauro Carvalho Chehab, Pawel Osciak, Hans Verkuil,
	Laurent Pinchart, Sakari Ailus

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@chromium.org>
> ---
>   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@samsung.com>

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.

>   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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations
  2017-05-31 12:09   ` Marek Szyprowski
@ 2017-05-31 12:46     ` Tomasz Figa
  2017-05-31 13:07       ` Sakari Ailus
  0 siblings, 1 reply; 5+ messages in thread
From: Tomasz Figa @ 2017-05-31 12:46 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Pawel Osciak,
	Hans Verkuil, Laurent Pinchart, Sakari Ailus

On Wed, May 31, 2017 at 9:09 PM, Marek Szyprowski
<m.szyprowski@samsung.com> 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@chromium.org>
>> ---
>>   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@samsung.com>

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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations
  2017-05-31 12:46     ` Tomasz Figa
@ 2017-05-31 13:07       ` Sakari Ailus
  2017-06-08 16:26         ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2017-05-31 13:07 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Marek Szyprowski, linux-media, linux-kernel,
	Mauro Carvalho Chehab, Pawel Osciak, Hans Verkuil,
	Laurent Pinchart, Sakari Ailus

Hi Tomasz,

On Wed, May 31, 2017 at 09:46:05PM +0900, Tomasz Figa wrote:
> On Wed, May 31, 2017 at 9:09 PM, Marek Szyprowski
> <m.szyprowski@samsung.com> 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@chromium.org>
> >> ---
> >>   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@samsung.com>
> 
> 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.

I think it's better to leave videobuf1 as-is. I'd rather like to see it
removed instead.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations
  2017-05-31 13:07       ` Sakari Ailus
@ 2017-06-08 16:26         ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 5+ messages in thread
From: Mauro Carvalho Chehab @ 2017-06-08 16:26 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Tomasz Figa, Marek Szyprowski, linux-media, linux-kernel,
	Mauro Carvalho Chehab, Pawel Osciak, Hans Verkuil,
	Laurent Pinchart, Sakari Ailus

Em Wed, 31 May 2017 16:07:29 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Tomasz,
> 
> On Wed, May 31, 2017 at 09:46:05PM +0900, Tomasz Figa wrote:
> > On Wed, May 31, 2017 at 9:09 PM, Marek Szyprowski
> > <m.szyprowski@samsung.com> 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@chromium.org>
> > >> ---
> > >>   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@samsung.com>  
> > 
> > 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.  
> 
> I think it's better to leave videobuf1 as-is. I'd rather like to see it
> removed instead.

Agreed.

There aren't much VB drivers anymore:

$ git grep -l VIDEOBUF_ |grep Kconfig
drivers/media/common/saa7146/Kconfig
drivers/media/pci/bt8xx/Kconfig
drivers/media/pci/cx18/Kconfig
drivers/media/platform/Kconfig
drivers/media/platform/davinci/Kconfig
drivers/media/platform/omap/Kconfig
drivers/media/usb/cx231xx/Kconfig
drivers/media/usb/tm6000/Kconfig
drivers/media/usb/zr364xx/Kconfig
drivers/media/v4l2-core/Kconfig
drivers/staging/media/atomisp/pci/Kconfig

(at platform/Kconfig, there are two drivers: via-camera and viu)

Not sure how easy/hard would be to convert those remaining ones.


Thanks,
Mauro

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-06-08 16:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20170531065846epcas5p4d4cf5a7cb2bb86fe4e9d9151fc83a896@epcas5p4.samsung.com>
2017-05-31  6:58 ` [PATCH RFC] v4l2-core: Use kvmalloc() for potentially big allocations Tomasz Figa
2017-05-31 12:09   ` Marek Szyprowski
2017-05-31 12:46     ` Tomasz Figa
2017-05-31 13:07       ` Sakari Ailus
2017-06-08 16:26         ` Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).