On 2025-11-18 at 14:23:36 GMT, Andi Shyti wrote:
> Hi Krzysztof,
>
> ...
>
> > +static struct i915_sw_fence *submit;
> > +static struct delayed_work __live_submit_work;
> > +
> > +static void __live_submit_work_handler(struct work_struct *work)
> > +{
> > + i915_sw_fence_commit(submit);
> > + heap_fence_put(submit);
> > +}
> > +
> > static struct live_active *
> > __live_active_setup(struct drm_i915_private *i915)
> > {
> > struct intel_engine_cs *engine;
> > - struct i915_sw_fence *submit;
> > struct live_active *active;
> > unsigned int count = 0;
> > int err = 0;
> >
> > + INIT_DELAYED_WORK(&__live_submit_work, __live_submit_work_handler);
> > +
> > active = __live_alloc(i915);
> > if (!active)
> > return ERR_PTR(-ENOMEM);
> > @@ -132,8 +142,7 @@ __live_active_setup(struct drm_i915_private *i915)
> > }
> >
> > out:
> > - i915_sw_fence_commit(submit);
> > - heap_fence_put(submit);
> > + schedule_delayed_work(&__live_submit_work, msecs_to_jiffies(500));
>
> Please do not do this: you are scheduling work and leaving it
> to run on its own. This can cause races with the global
> variables, and you may exit with work still pending.
>
The tests sleep waiting for the active, which only ever gets retired
after the scheduled work runs (which is precisely the point of the
change), and the global variable isn't touched in the test between
issuing the delayed work and waiting for the active. So strictly
technically speaking there shouldn't be a race happening, if that's what
you're referring to here.
I can get behind explicitly waiting for the work to finish at the end of
the test, even if for the sake of discipline. (will a plain
flush_scheduled_work() suffice here?)
> You must at least wait for the work to finish before leaving the
> test.
>
That would be the "at least", does that mean you have other suggestions
for this?
Thanks
Krzysztof
> Andi
>
> > if (err) {
> > __live_put(active);
> > active = ERR_PTR(err);
> > --
> > 2.45.2
> >
