public inbox for [email protected]
 help / color / mirror / Atom feed
From: Pavel Begunkov <[email protected]>
To: Jens Axboe <[email protected]>, [email protected]
Subject: Re: [PATCH 1/1] io_uring: fix leaks on IOPOLL and CQE_SKIP
Date: Sat, 16 Apr 2022 09:39:32 +0100	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 4/15/22 23:53, Jens Axboe wrote:
> On 4/15/22 4:41 PM, Pavel Begunkov wrote:
>> On 4/15/22 23:03, Jens Axboe wrote:
>>> On 4/15/22 3:05 PM, Pavel Begunkov wrote:
>>>> On 4/12/22 17:46, Jens Axboe wrote:
>>>>> On 4/12/22 10:41 AM, Jens Axboe wrote:
>>>>>> On 4/12/22 10:24 AM, Pavel Begunkov wrote:
>>>>>>> If all completed requests in io_do_iopoll() were marked with
>>>>>>> REQ_F_CQE_SKIP, we'll not only skip CQE posting but also
>>>>>>> io_free_batch_list() leaking memory and resources.
>>>>>>>
>>>>>>> Move @nr_events increment before REQ_F_CQE_SKIP check. We'll potentially
>>>>>>> return the value greater than the real one, but iopolling will deal with
>>>>>>> it and the userspace will re-iopoll if needed. In anyway, I don't think
>>>>>>> there are many use cases for REQ_F_CQE_SKIP + IOPOLL.
>>>>>>
>>>>>> Ah good catch - yes probably not much practical concern, as the lack of
>>>>>> ordering for file IO means that CQE_SKIP isn't really useful for that
>>>>>> scenario.
>>>>>
>>>>> One potential snag is with the change we're now doing
>>>>> io_cqring_ev_posted_iopoll() even if didn't post an event. Again
>>>>> probably not a practical concern, but it is theoretically a violation
>>>>> if an eventfd is used.
>>>> Looks this didn't get applied. Are you concerned about eventfd?
>>>
>>> Yep, was hoping to get a reply back, so just deferred it for now.
>>>
>>>> Is there any good reason why the userspace can't tolerate spurious
>>>> eventfd events? Because I don't think we should care this case
>>>
>>> I always forget the details on that, but we've had cases like this in
>>> the past where some applications assume that if they got N eventfd
>>> events, then are are also N events in the ring. Which granted is a bit
>>> odd, but it does also make some sense. Why would you have more eventfd
>>> events posted than events?
>>
>> For the same reason why it can get less eventfd events than there are
>> CQEs, as for me it's only a communication channel but not a
>> replacement for completion events.
> 
> That part is inherently racy in that we might get some CQEs while we
> respond to the initial eventfd notifications. But I'm totally agreeing
> with you, and it doesn't seem like a big deal to me.
> 
>> Ok, we don't want to break old applications, but it's a new most
>> probably not widely used feature, and we can say that the userspace
>> has to handle spurious eventfd.
> 
> If I were to guess, I'd say it's probably epoll + eventfd conversions.
> But it should just be made explicit. Since events reaped and checked

Didn't get it, what should be made explicit? Do you mean documenting
that there might be spurious eventfd events or something else?

> happen differently anyway, it seems like a bad assumption to make that
> eventfd notifications == events available.

-- 
Pavel Begunkov

  parent reply	other threads:[~2022-04-16  8:40 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-12 16:24 [PATCH 1/1] io_uring: fix leaks on IOPOLL and CQE_SKIP Pavel Begunkov
2022-04-12 16:41 ` Jens Axboe
2022-04-12 16:46   ` Jens Axboe
2022-04-15 21:05     ` Pavel Begunkov
2022-04-15 22:03       ` Jens Axboe
2022-04-15 22:41         ` Pavel Begunkov
2022-04-15 22:53           ` Jens Axboe
2022-04-15 23:51             ` Jens Axboe
2022-04-16  8:36               ` Pavel Begunkov
2022-04-16  8:39             ` Pavel Begunkov [this message]
2022-04-16 13:23               ` Jens Axboe
2022-04-16  8:34           ` Pavel Begunkov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox