| Message ID | 20220217075014.922605-2-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mm: Add an iterator to optimally walk over holes suitable for an allocation | expand |
On 17/02/2022 07:50, Vivek Kasireddy wrote: > While looking for next holes suitable for an allocation, although, > it is highly unlikely, make sure that the DECLARE_NEXT_HOLE_ADDR > macro is using a valid node before it extracts the rb_node from it. Was the need for this just a consequence of insufficient locking in the i915 patch? Regards, Tvrtko > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Christian König <christian.koenig@amd.com> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > --- > drivers/gpu/drm/drm_mm.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c > index 8257f9d4f619..499d8874e4ed 100644 > --- a/drivers/gpu/drm/drm_mm.c > +++ b/drivers/gpu/drm/drm_mm.c > @@ -389,11 +389,12 @@ first_hole(struct drm_mm *mm, > #define DECLARE_NEXT_HOLE_ADDR(name, first, last) \ > static struct drm_mm_node *name(struct drm_mm_node *entry, u64 size) \ > { \ > - struct rb_node *parent, *node = &entry->rb_hole_addr; \ > + struct rb_node *parent, *node; \ > \ > - if (!entry || RB_EMPTY_NODE(node)) \ > + if (!entry || RB_EMPTY_NODE(&entry->rb_hole_addr)) \ > return NULL; \ > \ > + node = &entry->rb_hole_addr; \ > if (usable_hole_addr(node->first, size)) { \ > node = node->first; \ > while (usable_hole_addr(node->last, size)) \
Hi Tvrtko, > > On 17/02/2022 07:50, Vivek Kasireddy wrote: > > While looking for next holes suitable for an allocation, although, > > it is highly unlikely, make sure that the DECLARE_NEXT_HOLE_ADDR > > macro is using a valid node before it extracts the rb_node from it. > > Was the need for this just a consequence of insufficient locking in the > i915 patch? [Kasireddy, Vivek] Partly, yes; but I figured since we are anyway doing if (!entry || ..), it makes sense to dereference entry and extract the rb_node after this check. Thanks, Vivek > > Regards, > > Tvrtko > > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: Christian König <christian.koenig@amd.com> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > --- > > drivers/gpu/drm/drm_mm.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c > > index 8257f9d4f619..499d8874e4ed 100644 > > --- a/drivers/gpu/drm/drm_mm.c > > +++ b/drivers/gpu/drm/drm_mm.c > > @@ -389,11 +389,12 @@ first_hole(struct drm_mm *mm, > > #define DECLARE_NEXT_HOLE_ADDR(name, first, last) \ > > static struct drm_mm_node *name(struct drm_mm_node *entry, u64 size) \ > > { \ > > - struct rb_node *parent, *node = &entry->rb_hole_addr; \ > > + struct rb_node *parent, *node; \ > > \ > > - if (!entry || RB_EMPTY_NODE(node)) \ > > + if (!entry || RB_EMPTY_NODE(&entry->rb_hole_addr)) \ > > return NULL; \ > > \ > > + node = &entry->rb_hole_addr; \ > > if (usable_hole_addr(node->first, size)) { \ > > node = node->first; \ > > while (usable_hole_addr(node->last, size)) \
On 18/02/2022 03:47, Kasireddy, Vivek wrote: > Hi Tvrtko, > >> >> On 17/02/2022 07:50, Vivek Kasireddy wrote: >>> While looking for next holes suitable for an allocation, although, >>> it is highly unlikely, make sure that the DECLARE_NEXT_HOLE_ADDR >>> macro is using a valid node before it extracts the rb_node from it. >> >> Was the need for this just a consequence of insufficient locking in the >> i915 patch? > [Kasireddy, Vivek] Partly, yes; but I figured since we are anyway doing > if (!entry || ..), it makes sense to dereference entry and extract the rb_node > after this check. Unless I am blind I don't see that it makes a difference. "&entry->rb_hole_addr" is taking an address of, which works "fine" is entry is NULL. And does not get past the !entry check for the actual de-reference via RB_EMPTY_NODE. With your patch you move that after the !entry check but still have it in the RB_EMPTY_NODE macro. Again, unless I am blind, I think just drop this patch. Regards, Tvrtko > Thanks, > Vivek > >> >> Regards, >> >> Tvrtko >> >>> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>> Cc: Christian König <christian.koenig@amd.com> >>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> >>> --- >>> drivers/gpu/drm/drm_mm.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c >>> index 8257f9d4f619..499d8874e4ed 100644 >>> --- a/drivers/gpu/drm/drm_mm.c >>> +++ b/drivers/gpu/drm/drm_mm.c >>> @@ -389,11 +389,12 @@ first_hole(struct drm_mm *mm, >>> #define DECLARE_NEXT_HOLE_ADDR(name, first, last) \ >>> static struct drm_mm_node *name(struct drm_mm_node *entry, u64 size) \ >>> { \ >>> - struct rb_node *parent, *node = &entry->rb_hole_addr; \ >>> + struct rb_node *parent, *node; \ >>> \ >>> - if (!entry || RB_EMPTY_NODE(node)) \ >>> + if (!entry || RB_EMPTY_NODE(&entry->rb_hole_addr)) \ >>> return NULL; \ >>> \ >>> + node = &entry->rb_hole_addr; \ >>> if (usable_hole_addr(node->first, size)) { \ >>> node = node->first; \ >>> while (usable_hole_addr(node->last, size)) \
Hi Tvrtko, > > On 18/02/2022 03:47, Kasireddy, Vivek wrote: > > Hi Tvrtko, > > > >> > >> On 17/02/2022 07:50, Vivek Kasireddy wrote: > >>> While looking for next holes suitable for an allocation, although, > >>> it is highly unlikely, make sure that the DECLARE_NEXT_HOLE_ADDR > >>> macro is using a valid node before it extracts the rb_node from it. > >> > >> Was the need for this just a consequence of insufficient locking in the > >> i915 patch? > > [Kasireddy, Vivek] Partly, yes; but I figured since we are anyway doing > > if (!entry || ..), it makes sense to dereference entry and extract the rb_node > > after this check. > > Unless I am blind I don't see that it makes a difference. > "&entry->rb_hole_addr" is taking an address of, which works "fine" is [Kasireddy, Vivek] Ah, didn't realize it was the same thing as offsetof(). > entry is NULL. And does not get past the !entry check for the actual > de-reference via RB_EMPTY_NODE. With your patch you move that after the > !entry check but still have it in the RB_EMPTY_NODE macro. Again, unless > I am blind, I think just drop this patch. [Kasireddy, Vivek] Sure; do you want me to send another version with this patch dropped? Or, would you be able to just merge the other two from the latest version of this series? Thanks, Vivek > > Regards, > > Tvrtko > > > > Thanks, > > Vivek > > > >> > >> Regards, > >> > >> Tvrtko > >> > >>> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > >>> Cc: Christian König <christian.koenig@amd.com> > >>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > >>> --- > >>> drivers/gpu/drm/drm_mm.c | 5 +++-- > >>> 1 file changed, 3 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c > >>> index 8257f9d4f619..499d8874e4ed 100644 > >>> --- a/drivers/gpu/drm/drm_mm.c > >>> +++ b/drivers/gpu/drm/drm_mm.c > >>> @@ -389,11 +389,12 @@ first_hole(struct drm_mm *mm, > >>> #define DECLARE_NEXT_HOLE_ADDR(name, first, last) \ > >>> static struct drm_mm_node *name(struct drm_mm_node *entry, u64 size) \ > >>> { \ > >>> - struct rb_node *parent, *node = &entry->rb_hole_addr; \ > >>> + struct rb_node *parent, *node; \ > >>> \ > >>> - if (!entry || RB_EMPTY_NODE(node)) \ > >>> + if (!entry || RB_EMPTY_NODE(&entry->rb_hole_addr)) \ > >>> return NULL; \ > >>> \ > >>> + node = &entry->rb_hole_addr; \ > >>> if (usable_hole_addr(node->first, size)) { \ > >>> node = node->first; \ > >>> while (usable_hole_addr(node->last, size)) \
On 23/02/2022 04:35, Kasireddy, Vivek wrote: > Hi Tvrtko, > >> >> On 18/02/2022 03:47, Kasireddy, Vivek wrote: >>> Hi Tvrtko, >>> >>>> >>>> On 17/02/2022 07:50, Vivek Kasireddy wrote: >>>>> While looking for next holes suitable for an allocation, although, >>>>> it is highly unlikely, make sure that the DECLARE_NEXT_HOLE_ADDR >>>>> macro is using a valid node before it extracts the rb_node from it. >>>> >>>> Was the need for this just a consequence of insufficient locking in the >>>> i915 patch? >>> [Kasireddy, Vivek] Partly, yes; but I figured since we are anyway doing >>> if (!entry || ..), it makes sense to dereference entry and extract the rb_node >>> after this check. >> >> Unless I am blind I don't see that it makes a difference. >> "&entry->rb_hole_addr" is taking an address of, which works "fine" is > [Kasireddy, Vivek] Ah, didn't realize it was the same thing as offsetof(). > >> entry is NULL. And does not get past the !entry check for the actual >> de-reference via RB_EMPTY_NODE. With your patch you move that after the >> !entry check but still have it in the RB_EMPTY_NODE macro. Again, unless >> I am blind, I think just drop this patch. > [Kasireddy, Vivek] Sure; do you want me to send another version with this > patch dropped? Or, would you be able to just merge the other two from the > latest version of this series? Please send without the first patch so we get clean set of CI results. You can use "--subject-prefix=CI" with git format-patchs and --suppress-cc=all with git send-email to avoid spamming people and let readers know the re-send is just for the purpose of getting CI results. Regards, Tvrtko
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index 8257f9d4f619..499d8874e4ed 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -389,11 +389,12 @@ first_hole(struct drm_mm *mm, #define DECLARE_NEXT_HOLE_ADDR(name, first, last) \ static struct drm_mm_node *name(struct drm_mm_node *entry, u64 size) \ { \ - struct rb_node *parent, *node = &entry->rb_hole_addr; \ + struct rb_node *parent, *node; \ \ - if (!entry || RB_EMPTY_NODE(node)) \ + if (!entry || RB_EMPTY_NODE(&entry->rb_hole_addr)) \ return NULL; \ \ + node = &entry->rb_hole_addr; \ if (usable_hole_addr(node->first, size)) { \ node = node->first; \ while (usable_hole_addr(node->last, size)) \
While looking for next holes suitable for an allocation, although, it is highly unlikely, make sure that the DECLARE_NEXT_HOLE_ADDR macro is using a valid node before it extracts the rb_node from it. Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Christian König <christian.koenig@amd.com> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> --- drivers/gpu/drm/drm_mm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)