[PATCH v6 1/4] drm/xe/vf: Divide ballooning into allocation and insertion
Lis, Tomasz
tomasz.lis at intel.com
Thu Apr 3 17:26:23 UTC 2025
On 01.04.2025 19:21, Michal Wajdeczko wrote:
> Hi Tomasz,
>
> Since we use 'ballooning' concept for GGTT, please include 'GGTT' in the
> title to make it more specific
ack
> On 31.03.2025 15:21, Tomasz Lis wrote:
>> The balloon nodes used to fill areas of GGTT inaccessible for
>> a specific VF, were allocaten and inserted into GGTT within
> typo
ack
>> one function. This disallowed re-using the insertion part
>> during VF migration recovery.
> The wording "This disallowed re-using" sounds like there is a bug in
> current implementation, while all we want is to refactor the code, to
> make if more reusable, so IMO we can just drop this sentence, and just
> add in the one below that the split is for the "reuse".
I like the division between "general awareness" paragraph and "what this
patch does"
paragraph. But I will rephrase.
>> This patch separates allocation (init/fini functs) from the insertion
>> of balloons (balloon/deballoon functs). Locks are also moved to ensure
>> calls from post-migration recovery worker will not cause a deadlock.
> hmm, this new locking is not looking as robust as before ... see below
>
>> v2: Moved declarations to proper header
>>
>> Signed-off-by: Tomasz Lis<tomasz.lis at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_ggtt.c | 9 +---
>> drivers/gpu/drm/xe/xe_gt_sriov_vf.c | 64 ++++++++++++++++++++++-------
>> drivers/gpu/drm/xe/xe_gt_sriov_vf.h | 2 +
>> 3 files changed, 52 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
>> index 5fcb2b4c2c13..2d7456e37ef4 100644
>> --- a/drivers/gpu/drm/xe/xe_ggtt.c
>> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
>> @@ -452,9 +452,7 @@ int xe_ggtt_node_insert_balloon(struct xe_ggtt_node *node, u64 start, u64 end)
>> node->base.start = start;
>> node->base.size = end - start;
>>
>> - mutex_lock(&ggtt->lock);
> Functions that don't take the lock on it's own and expect lock to be
> already taken we usually name with "_locked" suffix and use explicit
>
> lockdep_assert_held
>
> to make sure it is enforced
>
> also consider whether there should be just two variants of the function
> defined in xe_ggtt:
>
> xe_ggtt_node_insert_balloon_locked()
> {
> lockdep_assert_held
> ...
> }
>
> xe_ggtt_node_insert_balloon()
> {
> mutex_lock
> xe_ggtt_node_insert_balloon_locked()
> mutex_unlock
> }
We will never use a "non-_locked" version at this level.
I think you was going for "_locked" version of
`xe_gt_sriov_vf_balloon_ggtt()`?
I don't think it makes sense from a code flow standpoint, but it is an
established convention.. will do.
>> err = drm_mm_reserve_node(&ggtt->mm, &node->base);
>> - mutex_unlock(&ggtt->lock);
>>
>> if (xe_gt_WARN(ggtt->tile->primary_gt, err,
>> "Failed to balloon GGTT %#llx-%#llx (%pe)\n",
>> @@ -477,16 +475,11 @@ void xe_ggtt_node_remove_balloon(struct xe_ggtt_node *node)
>> return;
>>
>> if (!drm_mm_node_allocated(&node->base))
>> - goto free_node;
>> + return;
>>
>> xe_ggtt_dump_node(node->ggtt, &node->base, "remove-balloon");
>>
>> - mutex_lock(&node->ggtt->lock);
>> drm_mm_remove_node(&node->base);
>> - mutex_unlock(&node->ggtt->lock);
> same here
>
> xe_ggtt_node_remove_balloon_locked()
> {
> lockdep_assert_held
> ...
> }
>
> xe_ggtt_node_remove_balloon()
> {
> mutex_lock
> xe_ggtt_node_remove_balloon_locked()
> mutex_unlock
> }
as before, wrong level.
>> -
>> -free_node:
>> - xe_ggtt_node_fini(node);
>> }
>>
>> /**
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>> index a439261bf4d7..9edbe34f45f4 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>> @@ -560,26 +560,33 @@ u64 xe_gt_sriov_vf_lmem(struct xe_gt *gt)
>> return gt->sriov.vf.self_config.lmem_size;
>> }
>>
>> -static struct xe_ggtt_node *
>> -vf_balloon_ggtt_node(struct xe_ggtt *ggtt, u64 start, u64 end)
>> +static int
>> +vf_balloon_ggtt_node(struct xe_ggtt *ggtt, struct xe_ggtt_node *node,
>> + u64 start, u64 end)
> as this function is now just a simple wrapper around
> xe_ggtt_node_insert_balloon then maybe it's not needed and caller may
> use xe_ggtt_node_insert_balloon directly?
will do.
>> {
>> - struct xe_ggtt_node *node;
>> int err;
>>
>> - node = xe_ggtt_node_init(ggtt);
>> if (IS_ERR(node))
> this runtime check shouldn't be needed any more as any node_init()
> failure shall trigger probe abort
>
>> - return node;
>> + return PTR_ERR(node);
>>
>> err = xe_ggtt_node_insert_balloon(node, start, end);
>> if (err) {
>> - xe_ggtt_node_fini(node);
>> - return ERR_PTR(err);
>> + return err;
>> }
>>
>> - return node;
>> + return 0;
>> }
>>
>> -static int vf_balloon_ggtt(struct xe_gt *gt)
>> +static void xe_gt_sriov_vf_balloon_init(struct xe_gt *gt)
> please don't use fully qualified names for static functions
> here it could be as simple as:
>
> vf_init_ggtt_balloons()
ok
>> +{
>> + struct xe_tile *tile = gt_to_tile(gt);
>> + struct xe_ggtt *ggtt = tile->mem.ggtt;
>> +
>> + tile->sriov.vf.ggtt_balloon[0] = xe_ggtt_node_init(ggtt);
>> + tile->sriov.vf.ggtt_balloon[1] = xe_ggtt_node_init(ggtt);
> failure in any of xe_ggtt_node_init() is fatal from the VF POV and shall
> trigger immediate probe abort
will do that.
> btw, maybe it's good time to add new function variant:
>
> xe_ggtt_node_init_managed()
>
> to make sure we don't forget to call xe_ggtt_node_fini() ?
I don't see the point. We do sometimes create "managed" versions (but
that word is never added as a suffix),
but it's not really an established standard. Also it doesn't fit here.
This brings no improvement.
>> +}
>> +
>> +int xe_gt_sriov_vf_balloon_ggtt(struct xe_gt *gt)
> all new public functions shall have proper kernel-doc
true.
>> {
>> struct xe_gt_sriov_vf_selfconfig *config = >->sriov.vf.self_config;
>> struct xe_tile *tile = gt_to_tile(gt);
>> @@ -589,6 +596,7 @@ static int vf_balloon_ggtt(struct xe_gt *gt)
>>
>> xe_gt_assert(gt, IS_SRIOV_VF(xe));
>> xe_gt_assert(gt, !xe_gt_is_media_type(gt));
>> + lockdep_assert_held(&tile->mem.ggtt->lock);
>>
>> if (!config->ggtt_size)
>> return -ENODATA;
>> @@ -611,7 +619,7 @@ static int vf_balloon_ggtt(struct xe_gt *gt)
>> start = xe_wopcm_size(xe);
>> end = config->ggtt_base;
>> if (end != start) {
>> - tile->sriov.vf.ggtt_balloon[0] = vf_balloon_ggtt_node(ggtt, start, end);
>> + vf_balloon_ggtt_node(ggtt, tile->sriov.vf.ggtt_balloon[0], start, end);
>> if (IS_ERR(tile->sriov.vf.ggtt_balloon[0]))
>> return PTR_ERR(tile->sriov.vf.ggtt_balloon[0]);
>> }
>> @@ -619,7 +627,7 @@ static int vf_balloon_ggtt(struct xe_gt *gt)
>> start = config->ggtt_base + config->ggtt_size;
>> end = GUC_GGTT_TOP;
>> if (end != start) {
>> - tile->sriov.vf.ggtt_balloon[1] = vf_balloon_ggtt_node(ggtt, start, end);
>> + vf_balloon_ggtt_node(ggtt, tile->sriov.vf.ggtt_balloon[1], start, end);
>> if (IS_ERR(tile->sriov.vf.ggtt_balloon[1])) {
>> xe_ggtt_node_remove_balloon(tile->sriov.vf.ggtt_balloon[0]);
>> return PTR_ERR(tile->sriov.vf.ggtt_balloon[1]);
>> @@ -629,15 +637,34 @@ static int vf_balloon_ggtt(struct xe_gt *gt)
>> return 0;
>> }
>>
>> -static void deballoon_ggtt(struct drm_device *drm, void *arg)
>> +void xe_gt_sriov_vf_deballoon_ggtt(struct xe_gt *gt)
> if this is a public function now, then kernel-doc is required
yes.
>> {
>> - struct xe_tile *tile = arg;
>> + struct xe_tile *tile = gt_to_tile(gt);
>>
>> xe_tile_assert(tile, IS_SRIOV_VF(tile_to_xe(tile)));
>> + lockdep_assert_held(&tile->mem.ggtt->lock);
> why here?
> it should be already part of the xe_ggtt_node_remove_balloon()
This is the function which will get the suffix "_locked", so the check
here is really mandatory.
Will add to sub-function as well.
>> xe_ggtt_node_remove_balloon(tile->sriov.vf.ggtt_balloon[1]);
>> xe_ggtt_node_remove_balloon(tile->sriov.vf.ggtt_balloon[0]);
>> }
>>
>> +static void xe_gt_sriov_vf_balloon_fini(struct xe_gt *gt)
> don't use fully qualified names for static functions
ok
> look around how everything else is named!
>
>> +{
>> + struct xe_tile *tile = gt_to_tile(gt);
>> +
>> + xe_ggtt_node_fini(tile->sriov.vf.ggtt_balloon[1]);
>> + xe_ggtt_node_fini(tile->sriov.vf.ggtt_balloon[0]);
>> +}
>> +
>> +static void deballoon_ggtt(struct drm_device *drm, void *arg)
>> +{
>> + struct xe_tile *tile = arg;
>> +
>> + mutex_lock(&tile->mem.ggtt->lock);
>> + xe_gt_sriov_vf_deballoon_ggtt(tile->primary_gt);
>> + mutex_unlock(&tile->mem.ggtt->lock);
>> + xe_gt_sriov_vf_balloon_fini(tile->primary_gt);
>> +}
>> +
>> /**
>> * xe_gt_sriov_vf_prepare_ggtt - Prepare a VF's GGTT configuration.
>> * @gt: the &xe_gt
>> @@ -650,14 +677,21 @@ int xe_gt_sriov_vf_prepare_ggtt(struct xe_gt *gt)
>> {
>> struct xe_tile *tile = gt_to_tile(gt);
>> struct xe_device *xe = tile_to_xe(tile);
>> + struct xe_ggtt *ggtt = tile->mem.ggtt;
>> int err;
>>
>> if (xe_gt_is_media_type(gt))
>> return 0;
>>
>> - err = vf_balloon_ggtt(gt);
>> - if (err)
>> + xe_gt_sriov_vf_balloon_init(gt);
>> +
>> + mutex_lock(&ggtt->lock);
>> + err = xe_gt_sriov_vf_balloon_ggtt(gt);
>> + mutex_unlock(&ggtt->lock);
>> + if (err) {
>> + xe_gt_sriov_vf_balloon_fini(gt);
> since you have split ballooning into two parts, both parts deserve it's
> own cleanup "actions" to avoid any mistakes in manual cleanup sequence
> like this (or we can rely on the new xe_ggtt_node_init_managed if added)
They do have that. balloon_ggtt has deballoon_ggtt, and balloon_init has
balloon_fini.
This will get clearer with modifications from previous comments applied.
-Tomasz
>> return err;
>> + }
>>
>> return drmm_add_action_or_reset(&xe->drm, deballoon_ggtt, tile);
>> }
>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
>> index ba6c5d74e326..c87b0e9c7ebc 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
>> @@ -18,6 +18,8 @@ int xe_gt_sriov_vf_query_config(struct xe_gt *gt);
>> int xe_gt_sriov_vf_connect(struct xe_gt *gt);
>> int xe_gt_sriov_vf_query_runtime(struct xe_gt *gt);
>> int xe_gt_sriov_vf_prepare_ggtt(struct xe_gt *gt);
>> +int xe_gt_sriov_vf_balloon_ggtt(struct xe_gt *gt);
>> +void xe_gt_sriov_vf_deballoon_ggtt(struct xe_gt *gt);
>> int xe_gt_sriov_vf_notify_resfix_done(struct xe_gt *gt);
>> void xe_gt_sriov_vf_migrated_event_handler(struct xe_gt *gt);
>>
More information about the dri-devel
mailing list