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

[Daniel P . Berrangé]

On Thu, Feb 22, 2018 at 01:06:33PM +0100, Peter Lieven wrote:
> Am 22.02.2018 um 13:03 schrieb Daniel P. Berrangé:
> > On Thu, Feb 22, 2018 at 01:02:05PM +0100, Peter Lieven wrote:
> >> 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 ?
> > If you have multiple -Wstack-size=$XXX  flags to GCC, I expect the last
> > one wins. So just need to double check that the per-object file CFLAGS
> > occur after the global CFLAS in the compiler args
> 
> I will check that, thanks.
> 
> When I am at it, what would be the proper replacement for char[PATH_MAX] ?

Generally code should dynamically allocate paths. If they need to sprintf
a path, then  g_strdup_printf() is the right approach.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|