Re: [PATCH 4/4] coroutine: Break inclusion loop

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/15/22 07:49, Markus Armbruster wrote:
>>> linux-user/ does not use coroutines, so I'd like to avoid that it
>>> includes qemu/coroutine.h.
>> They include it even before the patch, via lockable.h.
>
> They do but there's a difference between "including lockable.h and implictly getting coroutine.h due to dependencies" and "including 
> coroutine.h when you really wanted QEMU_LOCK_GUARD()".
>
>> My patch actually enables*not*  including coroutine.h: with it applied,
>> including lockable.h no longer gets you coroutine.h as well.
>> If you include lockable.h and make use of certain macros, the compile
>> fails, and you fix it by including coroutine.h instead like pretty much
>> everything else.  Is this really too objectionable to be committed?
>
> s/certain macros/all macros/.  All you can do is qemu_lockable_lock/unlock, which is the less common usage of 
> qemu/lockable.h:
>
> - qemu_lockable_lock/unlock: used in include/qemu/seqlock.h, tests/unit/test-coroutine.c, util/qemu-coroutine-lock.c
>
> - QEMU_LOCK_GUARD and WITH_QEMU_LOCK_GUARD are used in 49 files.
>
>>>> 1) qemu/coroutine.h keeps including qemu/lockable.h
>>
>> As in my patch.
>
> That's where the similarity ends. :)
>
>>>> 2) qemu/lockable.h is modified as follows to omit the reference to CoMutex:
>>>>
>>>> diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
>>>> index 86db7cb04c9c..db59656538a4 100644
>>>> --- a/include/qemu/lockable.h
>>>> +++ b/include/qemu/lockable.h
>>>> @@ -71,9 +71,11 @@ qemu_null_lockable(void *x)
>>>>                 void *: qemu_null_lockable(x),                             \
>>>>                 QemuMutex *: qemu_make_lockable(x, QML_OBJ_(x, mutex)),    \
>>>>                 QemuRecMutex *: qemu_make_lockable(x, QML_OBJ_(x,
>>>> rec_mutex)), \
>>>> -             CoMutex *: qemu_make_lockable(x, QML_OBJ_(x, co_mutex)),   \
>>>> +             QEMU_MAKE_CO_MUTEX_LOCKABLE(x)                             \
>>>>                 QemuSpin *: qemu_make_lockable(x, QML_OBJ_(x, spin)))
>>>>
>>>> +#define QEMU_MAKE_CO_MUTEX_LOCKABLE(x)
>>>> +
>>>>    /**
>>>>     * QEMU_MAKE_LOCKABLE_NONNULL - Make a polymorphic QemuLockable
>>>>     *
>>>>
>>>> 3) the following hack is added in qemu/coroutine.h, right after including
>>>> qemu/lockable.h:
>>>>
>>>> #undef QEMU_MAKE_CO_MUTEX_LOCKABLE(x)
>>>> #define QEMU_MAKE_CO_MUTEX_LOCKABLE(x) \
>>>>                CoMutex *: qemu_make_lockable(x, QML_OBJ_(x, co_mutex)),
>>
>> Let me see...  if I include just lockable.h and make use of certain
>> (generic) macro(s), the macro(s) won't have a case for QemuMutex *.
>> Using them with a QemuMutex * argument won't compile.
>
> s/QemuMutex/CoMutex/.  That is, you get the CoMutex case only if you include qemu/coroutine.h.  Which you probably did anyway, because 
> CoMutexes are almost always embedded (not used as pointers).  In fact I suspect the above trick also makes it possible to remove CoMutex from 
> qemu/typedefs.h.
>
> Furthermore, using qemu_lockable_lock/unlock with CoMutexes still works, even if you don't include qemu/coroutine.h, just like in your patch.
>
>>>> Neither is particularly pretty, so I vote for leaving things as is with
>>>> a comment above the two #include directives.
>>
>> Inlusion loops are landmines.  Evidence: the compilation failure Phil
>> ran in, leading to his
>>      Subject: [PATCH-for-8.0] coroutine: Add missing <qemu/atomic.h> include
>>      Message-Id:<20221125175532.48858-1-philmd@linaro.org>
>> Your macro hack I find too off-putting 😄
>
> I think the macro is much better than nerfing qemu/lockable.h.

The core of the problem is that lockable.h wants to provide _Generic()
with a CoMutex case if CoMutex exists.

The solution you proposed approximates this as follows.  lockable.h
makes the case configurable, default off.  coroutine.h configures it to
on, and includes lockable.h.  Works as long as we include only
coroutine.h, or coroutine.h before lockable.h.  Falls apart if we
include lockable.h, and only then coroutine.h.

For a robust solution, we'd need to enable lockable.h to detect "this
compilation unit may use coroutines".  Could CONFIG_USB_ONLY be pressed
into service?

> Another alternative is to add a new header qemu/lockable-protos.h and move qemu_co_mutex_{lock,unlock} there (possibly other prototypes as 
> well).  Then include it from both qemu/lockable.h and qemu/coroutine.h.

Only from lockable.h, because coroutine.h gets it via lockable.h, right?

Lazy^Wpragmatic solution: move the coroutine.h parts lockable.h needs to
lockable.h.  As far as I can tell:

    typedef CoMutex (unless we keep it in typedefs.h)
    qemu_co_mutex_lock()
    qemu_co_mutex_unlock()

Could throw in

    qemu_co_mutex_init()
    qemu_co_mutex_assert_locked()

to avoid splitting the co_mutex interface.

If keeping this in lockable.h bothers you, we could create comutex.h for
it.

Can't see what adding more to the new header would buy us (other than
arguments on how to name it).  Happy to be enlightened there, of course.