| Message ID | 1678305762-32381-1-git-send-email-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs | expand |
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
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
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
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 >
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); >
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);
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(-)