Re: [Qemu-block] [Qemu-devel] Limiting coroutine stack usage

[Peter Lieven]

Am 22.02.2018 um 13:00 schrieb Daniel P. Berrangé:
> On Thu, Feb 22, 2018 at 12:51:58PM +0100, Peter Lieven wrote:
>> Am 22.02.2018 um 12:40 schrieb Daniel P. Berrangé:
>>> On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote:
>>>> Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben:
>>>>> Am 22.02.2018 um 11:57 schrieb Kevin Wolf:
>>>>>> Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben:
>>>>>>> On 20/02/2018 18:04, Peter Lieven wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I remember we discussed a long time ago to limit the stack usage of all
>>>>>>>> functions that are executed in a coroutine
>>>>>>>> context to a very low value to be able to safely limit the coroutine
>>>>>>>> stack size as well.
>>>>>>> IIRC the only issue was that hw/ide/atapi.c has mutual recursion between
>>>>>>> ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer ->
>>>>>>> ide_atapi_cmd_reply_end.
>>>>>>>
>>>>>>> But perhaps it's not an issue, somebody needs to audit the code.
>>>>>> I think John intended to get rid of the recursion sometime, but I doubt
>>>>>> he has had the time so far.
>>>>> Apart from this is is possible to define special cflags in the
>>>>> Makefile.objs just for a subdirectory? I have patches ready to make
>>>>> the block layer files and other coroutine users compile with
>>>>> -Wstack-size=2048. But I do not want to specify each file separately.
>>>> Our Makefiles have lines like this:
>>>>
>>>>     iscsi.o-cflags     := $(LIBISCSI_CFLAGS)
>>>>
>>>> I don't think there is a direct mechanism to apply cflags to a whole
>>>> directory or just to block-obj-y/block-obj-m, but just looping over them
>>>> could work. I'm not a Makefile expert at all, but after some toying with
>>>> a simple example, something like this might work:
>>>>
>>>>     $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048))
>>> You'll need it for anything block layer depends on too - so that's much
>>> of util/, crypto/ and io/ directories at least.
>>>
>>> So perhaps it would be shorter if we do the opposite - set -Wstack-size=2048
>>> globally for everything in QEMU, and then override -Wstack-size=$BIGGER
>>> for the (hopefully) few sources that have a larger stack need ?
>> I tried that already. 2048 is a strong limit for many functions.
>> It breaks already as soon as some buffer has a size of PATH_MAX, but
>> thats handleable. But there are some structs around that are very large.
> There are surprisingly few "char [PATH_MAX]" variables left in QEMU - we
> should have a final push to eliminate them regardless.
>
>> Generally, it would be a good idea to have a global limit, of course.
> We could at least put a limit on that matches the current worst case to
> prevent it getting worse than it already is.

That would be a good idea, yes.

How would you handle the override for a smaller -Wstack-usage ?

>
> Regards,
> Daniel