From: Juergen Gross <jgross@suse.com>
To: Demi Marie Obenour <demi@invisiblethingslab.com>,
Stefano Stabellini <sstabellini@kernel.org>,
Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Cc: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
stable@vger.kernel.org
Subject: Re: [PATCH] xen: speed up grant-table reclaim
Date: Tue, 14 Feb 2023 08:51:09 +0100 [thread overview]
Message-ID: <763838a9-acd5-b330-6165-6c288973d51c@suse.com> (raw)
In-Reply-To: <Y+qlPYi20cP0yXnE@itl-email>
[-- Attachment #1.1.1: Type: text/plain, Size: 4668 bytes --]
On 13.02.23 22:01, Demi Marie Obenour wrote:
> On Mon, Feb 13, 2023 at 10:26:11AM +0100, Juergen Gross wrote:
>> On 07.02.23 03:10, Demi Marie Obenour wrote:
>>> When a grant entry is still in use by the remote domain, Linux must put
>>> it on a deferred list. Normally, this list is very short, because
>>> the PV network and block protocols expect the backend to unmap the grant
>>> first. However, Qubes OS's GUI protocol is subject to the constraints
>>> of the X Window System, and as such winds up with the frontend unmapping
>>> the window first. As a result, the list can grow very large, resulting
>>> in a massive memory leak and eventual VM freeze.
>>>
>>> Fix this problem by bumping the number of entries that the VM will
>>> attempt to free at each iteration to 10000. This is an ugly hack that
>>> may well make a denial of service easier, but for Qubes OS that is less
>>> bad than the problem Qubes OS users are facing today.
>>
>>> There really
>>> needs to be a way for a frontend to be notified when the backend has
>>> unmapped the grants.
>>
>> Please remove this sentence from the commit message, or move it below the
>> "---" marker.
>
> Will fix in v2.
>
>> There are still some flag bits unallocated in struct grant_entry_v1 or
>> struct grant_entry_header. You could suggest some patches for Xen to use
>> one of the bits as a marker to get an event from the hypervisor if a
>> grant with such a bit set has been unmapped.
>
> That is indeed a good idea. There are other problems with the grant
> interface as well, but those can be dealt with later.
>
>> I have no idea, whether such an interface would be accepted by the
>> maintainers, though.
>>
>>> Additionally, a module parameter is provided to
>>> allow tuning the reclaim speed.
>>>
>>> The code previously used printk(KERN_DEBUG) whenever it had to defer
>>> reclaiming a page because the grant was still mapped. This resulted in
>>> a large volume of log messages that bothered users. Use pr_debug
>>> instead, which suppresses the messages by default. Developers can
>>> enable them using the dynamic debug mechanism.
>>>
>>> Fixes: QubesOS/qubes-issues#7410 (memory leak)
>>> Fixes: QubesOS/qubes-issues#7359 (excessive logging)
>>> Fixes: 569ca5b3f94c ("xen/gnttab: add deferred freeing logic")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
>>> ---
>>> Anyone have suggestions for improving the grant mechanism? Argo isn't
>>> a good option, as in the GUI protocol there are substantial performance
>>> wins to be had by using true shared memory. Resending as I forgot the
>>> Signed-off-by on the first submission. Sorry about that.
>>>
>>> drivers/xen/grant-table.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
>>> index 5c83d41..2c2faa7 100644
>>> --- a/drivers/xen/grant-table.c
>>> +++ b/drivers/xen/grant-table.c
>>> @@ -355,14 +355,20 @@
>>> static void gnttab_handle_deferred(struct timer_list *);
>>> static DEFINE_TIMER(deferred_timer, gnttab_handle_deferred);
>>> +static atomic64_t deferred_count;
>>> +static atomic64_t leaked_count;
>>> +static unsigned int free_per_iteration = 10000;
>>
>> As you are adding a kernel parameter to change this value, please set the
>> default to a value not potentially causing any DoS problems. Qubes OS can
>> still use a higher value then.
>
> Do you have any suggestions? I don’t know if this is actually a DoS
> concern anymore. Shrinking the interval between iterations would be.
Why don't you use today's value of 10 for the default?
>
>>> +
>>> static void gnttab_handle_deferred(struct timer_list *unused)
>>> {
>>> - unsigned int nr = 10;
>>> + unsigned int nr = READ_ONCE(free_per_iteration);
>>
>> I don't see why you are needing READ_ONCE() here.
>
> free_per_iteration can be concurrently modified via sysfs.
My remark was based on the wrong assumption that ignore_limit could be
dropped.
>
>>> + const bool ignore_limit = nr == 0;
>>
>> I don't think you need this extra variable, if you would ...
>>
>>> struct deferred_entry *first = NULL;
>>> unsigned long flags;
>>> + size_t freed = 0;
>>> spin_lock_irqsave(&gnttab_list_lock, flags);
>>> - while (nr--) {
>>> + while ((ignore_limit || nr--) && !list_empty(&deferred_list)) {
>>
>> ... change this to "while ((!nr || nr--) ...".
>
> nr-- evaluates to the old value of nr, so "while ((!nr | nr--)) ..." is
> an infinite loop.
Ah, right.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
next prev parent reply other threads:[~2023-02-14 7:51 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-07 2:10 [PATCH] xen: speed up grant-table reclaim Demi Marie Obenour
2023-02-13 9:26 ` Juergen Gross
2023-02-13 21:01 ` Demi Marie Obenour
2023-02-14 7:51 ` Juergen Gross [this message]
2023-02-14 15:08 ` Demi Marie Obenour
-- strict thread matches above, loose matches on Subject: below --
2023-06-10 15:32 Demi Marie Obenour
2023-06-12 6:27 ` Juergen Gross
2023-06-12 6:45 ` Juergen Gross
2023-06-12 20:09 ` Demi Marie Obenour
2023-06-13 6:45 ` Juergen Gross
2023-06-13 15:11 ` Demi Marie Obenour
2023-06-19 13:23 ` Marek Marczykowski-Górecki
2023-02-07 1:38 Demi Marie Obenour
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 \
--in-reply-to=763838a9-acd5-b330-6165-6c288973d51c@suse.com \
--to=jgross@suse.com \
--cc=demi@invisiblethingslab.com \
--cc=linux-kernel@vger.kernel.org \
--cc=marmarek@invisiblethingslab.com \
--cc=oleksandr_tyshchenko@epam.com \
--cc=sstabellini@kernel.org \
--cc=stable@vger.kernel.org \
--cc=xen-devel@lists.xenproject.org \
/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;
as well as URLs for NNTP newsgroup(s).