* [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
@ 2018-05-16 8:36 Jie Wang
2018-05-16 11:14 ` WangJie (Pluto)
2018-05-16 11:18 ` WangJie (Pluto)
0 siblings, 2 replies; 6+ messages in thread
From: Jie Wang @ 2018-05-16 8:36 UTC (permalink / raw)
To: stefanha, peterx, famz, eblake
Cc: qemu-devel, qemu-block, eric.fangyi, wu.wubin, wangjie88
When we call addIOThread, the epollfd created in aio_context_setup,
but not close it in the process of delIOThread, so the epollfd will leak.
Signed-off-by: Jie Wang <wangjie88@huawei.com>
---
include/block/aio.h | 8 ++++++++
util/aio-posix.c | 9 +++++++++
util/aio-win32.c | 4 ++++
util/async.c | 1 +
4 files changed, 22 insertions(+)
diff --git a/include/block/aio.h b/include/block/aio.h
index a1d6b9e..ae6f354 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -555,6 +555,14 @@ static inline bool in_aio_context_home_thread(AioContext *ctx)
void aio_context_setup(AioContext *ctx);
/**
+ * aio_context_destroy:
+ * @ctx: the aio context
+ *
+ * Destroy the aio context.
+ */
+void aio_context_destroy(AioContext *ctx);
+
+/**
* aio_context_set_poll_params:
* @ctx: the aio context
* @max_ns: how long to busy poll for, in nanoseconds
diff --git a/util/aio-posix.c b/util/aio-posix.c
index d8f0cb4..0ade2c7 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -713,6 +713,15 @@ void aio_context_setup(AioContext *ctx)
#endif
}
+void aio_context_destroy(AioContext *ctx)
+{
+#ifdef CONFIG_EPOLL_CREATE1
+ if (ctx->epollfd >= 0) {
+ close(ctx->epollfd);
+ }
+#endif
+}
+
void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
int64_t grow, int64_t shrink, Error **errp)
{
diff --git a/util/aio-win32.c b/util/aio-win32.c
index a67b00c..e676a8d 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -407,6 +407,10 @@ void aio_context_setup(AioContext *ctx)
{
}
+void aio_context_destroy(AioContext *ctx)
+{
+}
+
void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
int64_t grow, int64_t shrink, Error **errp)
{
diff --git a/util/async.c b/util/async.c
index 4dd9d95..03f6278 100644
--- a/util/async.c
+++ b/util/async.c
@@ -298,6 +298,7 @@ aio_ctx_finalize(GSource *source)
qemu_rec_mutex_destroy(&ctx->lock);
qemu_lockcnt_destroy(&ctx->list_lock);
timerlistgroup_deinit(&ctx->tlg);
+ aio_context_destroy(ctx);
}
static GSourceFuncs aio_source_funcs = {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
2018-05-16 8:36 [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread Jie Wang
@ 2018-05-16 11:14 ` WangJie (Pluto)
2018-05-16 11:43 ` Peter Xu
2018-05-16 11:18 ` WangJie (Pluto)
1 sibling, 1 reply; 6+ messages in thread
From: WangJie (Pluto) @ 2018-05-16 11:14 UTC (permalink / raw)
To: peterx
Cc: stefanha, famz, eblake, qemu-devel, qemu-block, eric.fangyi,
wu.wubin, wangjie88
Hi, Peter Xu:
If call aio_epoll_disable() here, aio_epoll_disable() will return before close ctx->epollfd,
Because the ctx->epoll_enabled is false in the moment.
In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
so ctx->epoll_enabled have no chance to set true.
On 2018/5/16 16:36, Jie Wang wrote:
> +void aio_context_destroy(AioContext *ctx)
> +{
> +#ifdef CONFIG_EPOLL_CREATE1
> + if (ctx->epollfd >= 0) {
> + close(ctx->epollfd);
> + }
> +#endif
> +}
> +
> void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> int64_t grow, int64_t shrink, Error **errp)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
2018-05-16 8:36 [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread Jie Wang
2018-05-16 11:14 ` WangJie (Pluto)
@ 2018-05-16 11:18 ` WangJie (Pluto)
1 sibling, 0 replies; 6+ messages in thread
From: WangJie (Pluto) @ 2018-05-16 11:18 UTC (permalink / raw)
To: stefanha, peterx, famz, eblake
Cc: qemu-devel, qemu-block, eric.fangyi, wu.wubin
Hi, Peter Xu:
If call aio_epoll_disable() in aio_context_destroy, aio_epoll_disable() will return before close(ctx->epollfd),
Because the ctx->epoll_enabled is false in the moment.
In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
so ctx->epoll_enabled have no chance to set true.
On 2018/5/16 16:36, Jie Wang wrote:
> When we call addIOThread, the epollfd created in aio_context_setup,
> but not close it in the process of delIOThread, so the epollfd will leak.
>
> Signed-off-by: Jie Wang <wangjie88@huawei.com>
> ---
> include/block/aio.h | 8 ++++++++
> util/aio-posix.c | 9 +++++++++
> util/aio-win32.c | 4 ++++
> util/async.c | 1 +
> 4 files changed, 22 insertions(+)
>
> diff --git a/include/block/aio.h b/include/block/aio.h
> index a1d6b9e..ae6f354 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -555,6 +555,14 @@ static inline bool in_aio_context_home_thread(AioContext *ctx)
> void aio_context_setup(AioContext *ctx);
>
> /**
> + * aio_context_destroy:
> + * @ctx: the aio context
> + *
> + * Destroy the aio context.
> + */
> +void aio_context_destroy(AioContext *ctx);
> +
> +/**
> * aio_context_set_poll_params:
> * @ctx: the aio context
> * @max_ns: how long to busy poll for, in nanoseconds
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index d8f0cb4..0ade2c7 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -713,6 +713,15 @@ void aio_context_setup(AioContext *ctx)
> #endif
> }
>
> +void aio_context_destroy(AioContext *ctx)
> +{
> +#ifdef CONFIG_EPOLL_CREATE1
> + if (ctx->epollfd >= 0) {
> + close(ctx->epollfd);
> + }
> +#endif
> +}
> +
> void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> int64_t grow, int64_t shrink, Error **errp)
> {
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index a67b00c..e676a8d 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -407,6 +407,10 @@ void aio_context_setup(AioContext *ctx)
> {
> }
>
> +void aio_context_destroy(AioContext *ctx)
> +{
> +}
> +
> void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> int64_t grow, int64_t shrink, Error **errp)
> {
> diff --git a/util/async.c b/util/async.c
> index 4dd9d95..03f6278 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -298,6 +298,7 @@ aio_ctx_finalize(GSource *source)
> qemu_rec_mutex_destroy(&ctx->lock);
> qemu_lockcnt_destroy(&ctx->list_lock);
> timerlistgroup_deinit(&ctx->tlg);
> + aio_context_destroy(ctx);
> }
>
> static GSourceFuncs aio_source_funcs = {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
2018-05-16 11:14 ` WangJie (Pluto)
@ 2018-05-16 11:43 ` Peter Xu
2018-05-17 1:42 ` WangJie (Pluto)
2018-05-17 2:26 ` Fam Zheng
0 siblings, 2 replies; 6+ messages in thread
From: Peter Xu @ 2018-05-16 11:43 UTC (permalink / raw)
To: WangJie (Pluto)
Cc: stefanha, famz, eblake, qemu-devel, qemu-block, eric.fangyi, wu.wubin
On Wed, May 16, 2018 at 07:14:53PM +0800, WangJie (Pluto) wrote:
> Hi, Peter Xu:
> If call aio_epoll_disable() here, aio_epoll_disable() will return before close ctx->epollfd,
> Because the ctx->epoll_enabled is false in the moment.
> In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
> so ctx->epoll_enabled have no chance to set true.
I see that epoll_available will only be set if epollfd != -1, so it
seems to me to make more sense if we swap the two variables in
aio_epoll_disable(), from current version:
static void aio_epoll_disable(AioContext *ctx)
{
ctx->epoll_available = false;
if (!ctx->epoll_enabled) {
return;
}
ctx->epoll_enabled = false;
close(ctx->epollfd);
}
To:
static void aio_epoll_disable(AioContext *ctx)
{
ctx->epoll_enabled = false;
if (!ctx->epoll_available) {
return;
}
ctx->epoll_available = false;
close(ctx->epollfd);
}
What do you think? And Fam?
>
> On 2018/5/16 16:36, Jie Wang wrote:
> > +void aio_context_destroy(AioContext *ctx)
> > +{
> > +#ifdef CONFIG_EPOLL_CREATE1
> > + if (ctx->epollfd >= 0) {
> > + close(ctx->epollfd);
> > + }
> > +#endif
> > +}
> > +
> > void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> > int64_t grow, int64_t shrink, Error **errp)
>
--
Peter Xu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
2018-05-16 11:43 ` Peter Xu
@ 2018-05-17 1:42 ` WangJie (Pluto)
2018-05-17 2:26 ` Fam Zheng
1 sibling, 0 replies; 6+ messages in thread
From: WangJie (Pluto) @ 2018-05-17 1:42 UTC (permalink / raw)
To: Peter Xu
Cc: stefanha, famz, eblake, qemu-devel, qemu-block, eric.fangyi, wu.wubin
I agree, wait for a reply from Fam
On 2018/5/16 19:43, Peter Xu wrote:
> On Wed, May 16, 2018 at 07:14:53PM +0800, WangJie (Pluto) wrote:
>> Hi, Peter Xu:
>> If call aio_epoll_disable() here, aio_epoll_disable() will return before close ctx->epollfd,
>> Because the ctx->epoll_enabled is false in the moment.
>> In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
>> so ctx->epoll_enabled have no chance to set true.
>
> I see that epoll_available will only be set if epollfd != -1, so it
> seems to me to make more sense if we swap the two variables in
> aio_epoll_disable(), from current version:
>
> static void aio_epoll_disable(AioContext *ctx)
> {
> ctx->epoll_available = false;
> if (!ctx->epoll_enabled) {
> return;
> }
> ctx->epoll_enabled = false;
> close(ctx->epollfd);
> }
>
> To:
>
> static void aio_epoll_disable(AioContext *ctx)
> {
> ctx->epoll_enabled = false;
> if (!ctx->epoll_available) {
> return;
> }
> ctx->epoll_available = false;
> close(ctx->epollfd);
> }
>
> What do you think? And Fam?
>
>>
>> On 2018/5/16 16:36, Jie Wang wrote:
>>> +void aio_context_destroy(AioContext *ctx)
>>> +{
>>> +#ifdef CONFIG_EPOLL_CREATE1
>>> + if (ctx->epollfd >= 0) {
>>> + close(ctx->epollfd);
>>> + }
>>> +#endif
>>> +}
>>> +
>>> void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
>>> int64_t grow, int64_t shrink, Error **errp)
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread
2018-05-16 11:43 ` Peter Xu
2018-05-17 1:42 ` WangJie (Pluto)
@ 2018-05-17 2:26 ` Fam Zheng
1 sibling, 0 replies; 6+ messages in thread
From: Fam Zheng @ 2018-05-17 2:26 UTC (permalink / raw)
To: Peter Xu
Cc: WangJie (Pluto),
stefanha, eblake, qemu-devel, qemu-block, eric.fangyi, wu.wubin
On Wed, 05/16 19:43, Peter Xu wrote:
> On Wed, May 16, 2018 at 07:14:53PM +0800, WangJie (Pluto) wrote:
> > Hi, Peter Xu:
> > If call aio_epoll_disable() here, aio_epoll_disable() will return before close ctx->epollfd,
> > Because the ctx->epoll_enabled is false in the moment.
> > In the process of addIOThread, aio_context_setup created epoll without call aio_epoll_try_enable,
> > so ctx->epoll_enabled have no chance to set true.
>
> I see that epoll_available will only be set if epollfd != -1, so it
> seems to me to make more sense if we swap the two variables in
> aio_epoll_disable(), from current version:
>
> static void aio_epoll_disable(AioContext *ctx)
> {
> ctx->epoll_available = false;
> if (!ctx->epoll_enabled) {
> return;
> }
> ctx->epoll_enabled = false;
> close(ctx->epollfd);
> }
>
> To:
>
> static void aio_epoll_disable(AioContext *ctx)
> {
> ctx->epoll_enabled = false;
> if (!ctx->epoll_available) {
> return;
> }
> ctx->epoll_available = false;
> close(ctx->epollfd);
> }
>
> What do you think? And Fam?
Looks good.
Fam
>
> >
> > On 2018/5/16 16:36, Jie Wang wrote:
> > > +void aio_context_destroy(AioContext *ctx)
> > > +{
> > > +#ifdef CONFIG_EPOLL_CREATE1
> > > + if (ctx->epollfd >= 0) {
> > > + close(ctx->epollfd);
> > > + }
> > > +#endif
> > > +}
> > > +
> > > void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> > > int64_t grow, int64_t shrink, Error **errp)
> >
>
> --
> Peter Xu
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-17 2:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-16 8:36 [Qemu-devel] [PATCH v4] iothread: fix epollfd leak in the process of delIOThread Jie Wang
2018-05-16 11:14 ` WangJie (Pluto)
2018-05-16 11:43 ` Peter Xu
2018-05-17 1:42 ` WangJie (Pluto)
2018-05-17 2:26 ` Fam Zheng
2018-05-16 11:18 ` WangJie (Pluto)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.