[RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs

[RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs

[Abhinav Kumar]

For DRM property blobs created by user mode using
drm_property_create_blob(), if the blob value needs to be updated the
only way is to destroy the previous blob and create a new one instead.

For some of the property blobs, if the size of the blob is more
than one page size, kvzalloc() can slow down system as it will first
try to allocate physically contiguous memory but upon failure will
fall back to non-contiguous (vmalloc) allocation.

If the blob property being used is bigger than one page size, in a
heavily loaded system, this causes performance issues because
some of the blobs are updated on a per-frame basis.

To mitigate the performance impact of kvzalloc(), use it only when
the size of allocation is less than a page size when creating property
blobs

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/drm_property.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index dfec479830e4..40c2a3142038 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
 	if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
 		return ERR_PTR(-EINVAL);
 
-	blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
+	if (sizeof(struct drm_property_blob) + length > PAGE_SIZE)
+		blob = vzalloc(sizeof(struct drm_property_blob)+length);
+	else
+		blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
+
 	if (!blob)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.7.4

Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs

[Ville Syrjälä]

On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote:
> For DRM property blobs created by user mode using
> drm_property_create_blob(), if the blob value needs to be updated the
> only way is to destroy the previous blob and create a new one instead.
> 
> For some of the property blobs, if the size of the blob is more
> than one page size, kvzalloc() can slow down system as it will first
> try to allocate physically contiguous memory but upon failure will
> fall back to non-contiguous (vmalloc) allocation.
> 
> If the blob property being used is bigger than one page size, in a
> heavily loaded system, this causes performance issues because
> some of the blobs are updated on a per-frame basis.
> 
> To mitigate the performance impact of kvzalloc(), use it only when
> the size of allocation is less than a page size when creating property
> blobs

Not sure how badly this will eat into the vmalloc area.

Is there no GFP flag to avoid the expensive stuff instead?

> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/drm_property.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index dfec479830e4..40c2a3142038 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
>  	if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
>  		return ERR_PTR(-EINVAL);
>  
> -	blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
> +	if (sizeof(struct drm_property_blob) + length > PAGE_SIZE)
> +		blob = vzalloc(sizeof(struct drm_property_blob)+length);
> +	else
> +		blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
> +
>  	if (!blob)
>  		return ERR_PTR(-ENOMEM);
>  
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel

Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs

[Rob Clark]

On Wed, Mar 8, 2023 at 1:23 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote:
> > For DRM property blobs created by user mode using
> > drm_property_create_blob(), if the blob value needs to be updated the
> > only way is to destroy the previous blob and create a new one instead.
> >
> > For some of the property blobs, if the size of the blob is more
> > than one page size, kvzalloc() can slow down system as it will first
> > try to allocate physically contiguous memory but upon failure will
> > fall back to non-contiguous (vmalloc) allocation.
> >
> > If the blob property being used is bigger than one page size, in a
> > heavily loaded system, this causes performance issues because
> > some of the blobs are updated on a per-frame basis.
> >
> > To mitigate the performance impact of kvzalloc(), use it only when
> > the size of allocation is less than a page size when creating property
> > blobs
>
> Not sure how badly this will eat into the vmalloc area.

Normally I wouldn't expect this to be much of a problem, but we don't
appear to restrict CREATEBLOBPROP to DRM_MASTER, which seems like it
might have been a mistake.. so perhaps we want to either restrict
CREATEBLOBPROP or put an upper threshold limit on total size of all
allocated blob props using vmalloc area?

BR,
-R

> Is there no GFP flag to avoid the expensive stuff instead?
>
> >
> > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > ---
> >  drivers/gpu/drm/drm_property.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> > index dfec479830e4..40c2a3142038 100644
> > --- a/drivers/gpu/drm/drm_property.c
> > +++ b/drivers/gpu/drm/drm_property.c
> > @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
> >       if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
> >               return ERR_PTR(-EINVAL);
> >
> > -     blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
> > +     if (sizeof(struct drm_property_blob) + length > PAGE_SIZE)
> > +             blob = vzalloc(sizeof(struct drm_property_blob)+length);
> > +     else
> > +             blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
> > +
> >       if (!blob)
> >               return ERR_PTR(-ENOMEM);
> >
> > --
> > 2.7.4
>
> --
> Ville Syrjälä
> Intel

Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs

[Ville Syrjälä]

On Wed, Mar 08, 2023 at 03:33:48PM -0800, Rob Clark wrote:
> On Wed, Mar 8, 2023 at 1:23 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote:
> > > For DRM property blobs created by user mode using
> > > drm_property_create_blob(), if the blob value needs to be updated the
> > > only way is to destroy the previous blob and create a new one instead.
> > >
> > > For some of the property blobs, if the size of the blob is more
> > > than one page size, kvzalloc() can slow down system as it will first
> > > try to allocate physically contiguous memory but upon failure will
> > > fall back to non-contiguous (vmalloc) allocation.
> > >
> > > If the blob property being used is bigger than one page size, in a
> > > heavily loaded system, this causes performance issues because
> > > some of the blobs are updated on a per-frame basis.
> > >
> > > To mitigate the performance impact of kvzalloc(), use it only when
> > > the size of allocation is less than a page size when creating property
> > > blobs
> >
> > Not sure how badly this will eat into the vmalloc area.
> 
> Normally I wouldn't expect this to be much of a problem, but we don't
> appear to restrict CREATEBLOBPROP to DRM_MASTER, which seems like it
> might have been a mistake.. so perhaps we want to either restrict
> CREATEBLOBPROP or put an upper threshold limit on total size of all
> allocated blob props using vmalloc area?

Surprisingly few kms ioctls are master-only it seems. Dunno
what the use case for all those being non-master really is.

I think blob limits in general were disussed at at various
points in the past with no conclusion. I guess it's slightly
problematic in that if you limit individual max blob size
then they just create more smaller ones. If you limit the
total size per fd they just open more fds. If you put a total
upper limit then it's just a slightly quicker DoS than
without the limit. Shrug.

> 
> BR,
> -R
> 
> > Is there no GFP flag to avoid the expensive stuff instead?
> >
> > >
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > > ---
> > >  drivers/gpu/drm/drm_property.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> > > index dfec479830e4..40c2a3142038 100644
> > > --- a/drivers/gpu/drm/drm_property.c
> > > +++ b/drivers/gpu/drm/drm_property.c
> > > @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
> > >       if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
> > > +     if (sizeof(struct drm_property_blob) + length > PAGE_SIZE)
> > > +             blob = vzalloc(sizeof(struct drm_property_blob)+length);
> > > +     else
> > > +             blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
> > > +
> > >       if (!blob)
> > >               return ERR_PTR(-ENOMEM);
> > >
> > > --
> > > 2.7.4
> >
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs

[Abhinav Kumar]

Hi Ville

Thanks for the comments.

On 3/8/2023 4:10 PM, Ville Syrjälä wrote:
> On Wed, Mar 08, 2023 at 03:33:48PM -0800, Rob Clark wrote:
>> On Wed, Mar 8, 2023 at 1:23 PM Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>>>
>>> On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote:
>>>> For DRM property blobs created by user mode using
>>>> drm_property_create_blob(), if the blob value needs to be updated the
>>>> only way is to destroy the previous blob and create a new one instead.
>>>>
>>>> For some of the property blobs, if the size of the blob is more
>>>> than one page size, kvzalloc() can slow down system as it will first
>>>> try to allocate physically contiguous memory but upon failure will
>>>> fall back to non-contiguous (vmalloc) allocation.
>>>>
>>>> If the blob property being used is bigger than one page size, in a
>>>> heavily loaded system, this causes performance issues because
>>>> some of the blobs are updated on a per-frame basis.
>>>>
>>>> To mitigate the performance impact of kvzalloc(), use it only when
>>>> the size of allocation is less than a page size when creating property
>>>> blobs
>>>
>>> Not sure how badly this will eat into the vmalloc area.
>>

The reason we had the PAGE_SIZE check to use vzalloc() was specifically 
to limit the cases which will be affected by this.

The percentage of blobs having a size more than a PAGE_SIZE will be the 
ones for which we will use vzalloc() which is actually good anyway since 
it cases of heavy memory fragmentation, kvzalloc() will fallback to vmalloc.

That percentage should have been very less to begin with otherwise 
others would have already hit this issue and even those will only 
benefit from this change in our opinion.

For most of the existing blobs, then this change should not affect and 
for those which it does, it should only benefit (like MSM).

>> Normally I wouldn't expect this to be much of a problem, but we don't
>> appear to restrict CREATEBLOBPROP to DRM_MASTER, which seems like it
>> might have been a mistake.. so perhaps we want to either restrict
>> CREATEBLOBPROP or put an upper threshold limit on total size of all
>> allocated blob props using vmalloc area?
> 
> Surprisingly few kms ioctls are master-only it seems. Dunno
> what the use case for all those being non-master really is.
> 
> I think blob limits in general were disussed at at various
> points in the past with no conclusion. I guess it's slightly
> problematic in that if you limit individual max blob size
> then they just create more smaller ones. If you limit the
> total size per fd they just open more fds. If you put a total
> upper limit then it's just a slightly quicker DoS than
> without the limit. Shrug.
> 
>>
>> BR,
>> -R
>>
>>> Is there no GFP flag to avoid the expensive stuff instead?
>>>
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_property.c | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
>>>> index dfec479830e4..40c2a3142038 100644
>>>> --- a/drivers/gpu/drm/drm_property.c
>>>> +++ b/drivers/gpu/drm/drm_property.c
>>>> @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
>>>>        if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
>>>>                return ERR_PTR(-EINVAL);
>>>>
>>>> -     blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
>>>> +     if (sizeof(struct drm_property_blob) + length > PAGE_SIZE)
>>>> +             blob = vzalloc(sizeof(struct drm_property_blob)+length);
>>>> +     else
>>>> +             blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
>>>> +
>>>>        if (!blob)
>>>>                return ERR_PTR(-ENOMEM);
>>>>
>>>> --
>>>> 2.7.4
>>>
>>> --
>>> Ville Syrjälä
>>> Intel
> 

Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs

[kernel test robot]

Hi Abhinav,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on drm-misc/drm-misc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abhinav-Kumar/drm-property-use-vzalloc-instead-of-kvzalloc-for-large-blobs/20230309-040340
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/1678305762-32381-1-git-send-email-quic_abhinavk%40quicinc.com
patch subject: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230309/202303090639.go7OZ9Q2-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/92cee5833068676c0ad28b238175260686a3c741
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Abhinav-Kumar/drm-property-use-vzalloc-instead-of-kvzalloc-for-large-blobs/20230309-040340
        git checkout 92cee5833068676c0ad28b238175260686a3c741
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/gpu/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303090639.go7OZ9Q2-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_property.c: In function 'drm_property_create_blob':
   drivers/gpu/drm/drm_property.c:565:24: error: implicit declaration of function 'vzalloc'; did you mean 'kvzalloc'? [-Werror=implicit-function-declaration]
     565 |                 blob = vzalloc(sizeof(struct drm_property_blob)+length);
         |                        ^~~~~~~
         |                        kvzalloc
>> drivers/gpu/drm/drm_property.c:565:22: warning: assignment to 'struct drm_property_blob *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     565 |                 blob = vzalloc(sizeof(struct drm_property_blob)+length);
         |                      ^
   cc1: some warnings being treated as errors


vim +565 drivers/gpu/drm/drm_property.c

   539	
   540	/**
   541	 * drm_property_create_blob - Create new blob property
   542	 * @dev: DRM device to create property for
   543	 * @length: Length to allocate for blob data
   544	 * @data: If specified, copies data into blob
   545	 *
   546	 * Creates a new blob property for a specified DRM device, optionally
   547	 * copying data. Note that blob properties are meant to be invariant, hence the
   548	 * data must be filled out before the blob is used as the value of any property.
   549	 *
   550	 * Returns:
   551	 * New blob property with a single reference on success, or an ERR_PTR
   552	 * value on failure.
   553	 */
   554	struct drm_property_blob *
   555	drm_property_create_blob(struct drm_device *dev, size_t length,
   556				 const void *data)
   557	{
   558		struct drm_property_blob *blob;
   559		int ret;
   560	
   561		if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
   562			return ERR_PTR(-EINVAL);
   563	
   564		if (sizeof(struct drm_property_blob) + length > PAGE_SIZE)
 > 565			blob = vzalloc(sizeof(struct drm_property_blob)+length);
   566		else
   567			blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
   568	
   569		if (!blob)
   570			return ERR_PTR(-ENOMEM);
   571	
   572		/* This must be explicitly initialised, so we can safely call list_del
   573		 * on it in the removal handler, even if it isn't in a file list. */
   574		INIT_LIST_HEAD(&blob->head_file);
   575		blob->data = (void *)blob + sizeof(*blob);
   576		blob->length = length;
   577		blob->dev = dev;
   578	
   579		if (data)
   580			memcpy(blob->data, data, length);
   581	
   582		ret = __drm_mode_object_add(dev, &blob->base, DRM_MODE_OBJECT_BLOB,
   583					    true, drm_property_free_blob);
   584		if (ret) {
   585			kvfree(blob);
   586			return ERR_PTR(-EINVAL);
   587		}
   588	
   589		mutex_lock(&dev->mode_config.blob_lock);
   590		list_add_tail(&blob->head_global,
   591		              &dev->mode_config.property_blob_list);
   592		mutex_unlock(&dev->mode_config.blob_lock);
   593	
   594		return blob;
   595	}
   596	EXPORT_SYMBOL(drm_property_create_blob);
   597	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs

[Dmitry Baryshkov]

On 08/03/2023 22:02, Abhinav Kumar wrote:
> For DRM property blobs created by user mode using
> drm_property_create_blob(), if the blob value needs to be updated the
> only way is to destroy the previous blob and create a new one instead.
> 
> For some of the property blobs, if the size of the blob is more
> than one page size, kvzalloc() can slow down system as it will first
> try to allocate physically contiguous memory but upon failure will
> fall back to non-contiguous (vmalloc) allocation.
> 
> If the blob property being used is bigger than one page size, in a
> heavily loaded system, this causes performance issues because
> some of the blobs are updated on a per-frame basis.
> 
> To mitigate the performance impact of kvzalloc(), use it only when
> the size of allocation is less than a page size when creating property
> blobs
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/drm_property.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index dfec479830e4..40c2a3142038 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
>   	if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
>   		return ERR_PTR(-EINVAL);
>   
> -	blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
> +	if (sizeof(struct drm_property_blob) + length > PAGE_SIZE)
> +		blob = vzalloc(sizeof(struct drm_property_blob)+length);
> +	else
> +		blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
> +

Seeing the same expression repeated three times in a row is a bad sign.
Also, I think in the else branch you can use kzalloc directly, kvzalloc 
will end up there anyway.

>   	if (!blob)
>   		return ERR_PTR(-ENOMEM);
>   

-- 
With best wishes
Dmitry