Am 20.06.23 um 08:47 schrieb Boris Brezillon:
On Mon, 19 Jun 2023 14:29:23 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
On Mon, 19 Jun 2023 12:44:06 +0200
Christian König <christian.koenig@xxxxxxx> wrote:
Am 19.06.23 um 12:12 schrieb Boris Brezillon:
[SNIP]
Note that the drm_exec_until_all_locked() helper I introduced is taking
an expression, so in theory, you don't have to define a separate
function.
drm_exec_until_all_locked(&exec, {
/* inlined-code */
int ret;
ret = blabla()
if (ret)
goto error;
...
error:
/* return value. */
ret;
});
This being said, as soon as you have several failure paths,
it makes things a lot easier/controllable if you make it a function,
and I honestly don't think the readability would suffer from having a
function defined just above the user. My main concern with the original
approach was the risk of calling continue/break_if_contended() in the
wrong place, and also the fact you can't really externalize things to
a function if you're looking for a cleaner split. At least with
drm_exec_until_all_locked() you can do both.
Yeah, but that means that you can't use return inside your code block
and instead has to define an error label for handling "normal"
contention which is what I'm trying to avoid here.
Sorry, didn't pay attention to this particular concern. Indeed, if you
want to return inside the expression, that's a problem.
Sorry, that's wrong again. Had trouble focusing yesterday...
So, returning directly from the expression block should be perfectly
fine. The only problem is breaking out of the retry loop early and
propagating the error, but that's no more or less problematic than it
was before. We just need the drm_exec_retry_on_contention() helper you
suggested, and a drm_exec_stop() that would go to some local
__drm_exec_stop label.
int ret = 0;
ret = drm_exec_until_all_locked(exec, ({
...
ret = drm_exec_prepare_obj(exec, objA, 1);
drm_exec_retry_on_contention(exec);
if (ret)
drm_exec_stop(exec, ret);
...
ret = drm_exec_prepare_obj(exec, objB, 1);
drm_exec_retry_on_contention(exec);
if (ret)
drm_exec_stop(exec, ret);
0;
}));
Which is pretty close to the syntax you defined initially, except for
the '0;' oddity at the end, which is ugly, I admit.
Yeah and it looks like giving code blocks as macro argument is usually
also not a good idea.
How about this:
#define drm_exec_until_all_locked(exec) \
for (void *__drm_exec_retry_ptr; ({ \
__label__ __drm_exec_retry; \
__drm_exec_retry: \
__drm_exec_retry_ptr = &&__drm_exec_retry; \
drm_exec_cleanup(exec); \
});)
#define drm_exec_retry_on_contention(exec) \
if (unlikely(drm_exec_is_contended(exec))) \
goto *__drm_exec_retry_ptr
The problem is that you can't declare a label so that it dominates the
body of the loop.
But what you can do is to declare a void* which dominates the loop, then
assign the address of a label to it and when you need it use goto*.
The goto* syntax is a gcc extension, but BPF is already using that in
the upstream kernel.
It's quite a hack and I need to extend my testing a bit, but as far as I
can see this actually works.
Regards,
Christian.
