Re: [PATCH] net/filter: Optimize filter_send to coroutine

["Rao, Lei" <lei.rao@intel.com>]

On 12/24/2021 6:07 PM, lizhijian@fujitsu.com wrote:
> 
> 
> On 24/12/2021 10:37, Rao, Lei wrote:
>> This patch is to improve the logic of QEMU main thread sleep code in
>> qemu_chr_write_buffer() where it can be blocked and can't run other
>> coroutines during COLO IO stress test.
>>
>> Our approach is to put filter_send() in a coroutine. In this way,
>> filter_send() will call qemu_coroutine_yield() in qemu_co_sleep_ns(),
>> so that it can be scheduled out and QEMU main thread has opportunity to
>> run other tasks.
>>
>> Signed-off-by: Lei Rao <lei.rao@intel.com>
>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> ---
>>    net/filter-mirror.c | 67 ++++++++++++++++++++++++++++++++++++---------
>>    1 file changed, 54 insertions(+), 13 deletions(-)
>>
>> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
>> index f20240cc9f..1e9f8b6216 100644
>> --- a/net/filter-mirror.c
>> +++ b/net/filter-mirror.c
>> @@ -20,6 +20,7 @@
>>    #include "chardev/char-fe.h"
>>    #include "qemu/iov.h"
>>    #include "qemu/sockets.h"
>> +#include "block/aio-wait.h"
>>    
>>    #define TYPE_FILTER_MIRROR "filter-mirror"
>>    typedef struct MirrorState MirrorState;
>> @@ -42,20 +43,21 @@ struct MirrorState {
>>        bool vnet_hdr;
>>    };
>>    
>> -static int filter_send(MirrorState *s,
>> -                       const struct iovec *iov,
>> -                       int iovcnt)
>> +typedef struct FilterSendCo {
>> +    MirrorState *s;
>> +    char *buf;
>> +    ssize_t size;
>> +    bool done;
>> +    int ret;
>> +} FilterSendCo;
>> +
>> +static int _filter_send(MirrorState *s,
>> +                       char *buf,
>> +                       ssize_t size)
>>    {
>>        NetFilterState *nf = NETFILTER(s);
>>        int ret = 0;
>> -    ssize_t size = 0;
>>        uint32_t len = 0;
>> -    char *buf;
>> -
>> -    size = iov_size(iov, iovcnt);
>> -    if (!size) {
>> -        return 0;
>> -    }
>>    
>>        len = htonl(size);
>>        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
>> @@ -80,10 +82,7 @@ static int filter_send(MirrorState *s,
>>            }
>>        }
>>    
>> -    buf = g_malloc(size);
>> -    iov_to_buf(iov, iovcnt, 0, buf, size);
>>        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
>> -    g_free(buf);
>>        if (ret != size) {
>>            goto err;
>>        }
>> @@ -94,6 +93,48 @@ err:
>>        return ret < 0 ? ret : -EIO;
>>    }
>>    
>> +static void coroutine_fn filter_send_co(void *opaque)
>> +{
>> +    FilterSendCo *data = opaque;
>> +
>> +    data->ret = _filter_send(data->s, data->buf, data->size);
>> +    data->done = true;
>> +    g_free(data->buf);
>> +    aio_wait_kick();
>> +}
>> +
>> +static int filter_send(MirrorState *s,
>> +                       const struct iovec *iov,
>> +                       int iovcnt)
>> +{
>> +    ssize_t size = iov_size(iov, iovcnt);
>> +    char *buf = NULL;
>> +
>> +    if (!size) {
>> +        return 0;
>> +    }
>> +
>> +    buf = g_malloc(size);
>> +    iov_to_buf(iov, iovcnt, 0, buf, size);
>> +
>> +    FilterSendCo data = {
>> +        .s = s,
>> +        .size = size,
>> +        .buf = buf,
>> +        .ret = 0,
>> +    };
>> +
>> +    Coroutine *co = qemu_coroutine_create(filter_send_co, &data);
> 
> BTW, does qemu/old gcc complaint such coding style ?
> 
> int a;
> a = foo()
> int b = a;

There are a lot of codes of this style in QEMU.
It is written that we need at least GCC v7.4 to compile QEMU in the configure file.
So, I think it is no problem.

> 
> 
> 
>> +    qemu_coroutine_enter(co);
>> +
>> +    while (!data.done) {
>> +        aio_poll(qemu_get_aio_context(), true);
>> +    }
>> +
>> +    return data.ret;
>> +
> redundant  newline

will be changed in V2.

Thanks,
Lei

> 
> Otherwise,
> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
> 
> 
> 
>> +}
>> +
>>    static void redirector_to_filter(NetFilterState *nf,
>>                                     const uint8_t *buf,
>>                                     int len)