* [PATCH] trace: Fix incorrect number of pages used for trace metadata
@ 2016-09-29 13:53 Igor Druzhinin
2016-09-30 14:46 ` George Dunlap
0 siblings, 1 reply; 7+ messages in thread
From: Igor Druzhinin @ 2016-09-29 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: george.dunlap, Igor Druzhinin
As long as t_info_first_offset is calculated in uint32_t offsets we need to
multiply it by sizeof(uint32_t) in order to get the right number of pages
for trace metadata. Not doing that makes it impossible to read the trace
buffer correctly from userspace for some corner cases.
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
xen/common/trace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/common/trace.c b/xen/common/trace.c
index f651cf3..a9563cc 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -149,7 +149,7 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
}
t_info_words = num_online_cpus() * pages * sizeof(uint32_t);
- t_info_pages = PFN_UP(t_info_first_offset + t_info_words);
+ t_info_pages = PFN_UP(t_info_first_offset * sizeof(uint32_t) + t_info_words);
printk(XENLOG_INFO "xentrace: requesting %u t_info pages "
"for %u trace pages on %u cpus\n",
t_info_pages, pages, num_online_cpus());
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata
2016-09-29 13:53 [PATCH] trace: Fix incorrect number of pages used for trace metadata Igor Druzhinin
@ 2016-09-30 14:46 ` George Dunlap
2016-09-30 14:48 ` Andrew Cooper
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: George Dunlap @ 2016-09-30 14:46 UTC (permalink / raw)
To: Igor Druzhinin, xen-devel
Cc: george.dunlap, Andrew Cooper, Olaf Hering, Jan Beulich
[-- Attachment #1: Type: text/plain, Size: 909 bytes --]
On 29/09/16 14:53, Igor Druzhinin wrote:
> As long as t_info_first_offset is calculated in uint32_t offsets we need to
> multiply it by sizeof(uint32_t) in order to get the right number of pages
> for trace metadata. Not doing that makes it impossible to read the trace
> buffer correctly from userspace for some corner cases.
>
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
correct formula to calculate t_info_pages". But that one was presumably
written (and Acked by me) because the variable name there,
t_info_first_offset, is confusing.
The other mistake in fbf96e6 is that before t_info_words was actually
denominated in words; but after it's denominated in bytes (which is
again confusing).
What about something like the attached instead? This should fix your
problem while making the code clearer.
-George
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-xen-trace-Fix-trace-metadata-page-count-calculation-.patch --]
[-- Type: text/x-diff; name="0001-xen-trace-Fix-trace-metadata-page-count-calculation-.patch", Size: 1806 bytes --]
From b0252cec4a96fea28faa85c47ab0f5d5997444ad Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 30 Sep 2016 15:42:56 +0100
Subject: [PATCH] xen/trace: Fix trace metadata page count calculation (revert
fbf96e6)
Changeset fbf96e6, "xentrace: correct formula to calculate
t_info_pages", broke the trace metadata page count calculation, by
mistaking t_info_first_offset as denominated in bytes, when in fact it
is denominated in words (uint32_t).
Effectively revert that change, and put a comment there to reduce the
chance that someone will make that mistake in the future.
Spotted-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Olaf Hering <olaf@aepfle.de>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
xen/common/trace.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/xen/common/trace.c b/xen/common/trace.c
index f651cf3..2f4ecca 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -148,8 +148,12 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
pages = max_pages;
}
- t_info_words = num_online_cpus() * pages * sizeof(uint32_t);
- t_info_pages = PFN_UP(t_info_first_offset + t_info_words);
+ /*
+ * NB this calculation is correct, because t_info_first_offset is
+ * in words, not bytes, not bytes
+ */
+ t_info_words = num_online_cpus() * pages + t_info_first_offset;
+ t_info_pages = PFN_UP(t_info_words * sizeof(uint32_t));
printk(XENLOG_INFO "xentrace: requesting %u t_info pages "
"for %u trace pages on %u cpus\n",
t_info_pages, pages, num_online_cpus());
--
2.1.4
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata
2016-09-30 14:46 ` George Dunlap
@ 2016-09-30 14:48 ` Andrew Cooper
2016-09-30 14:48 ` George Dunlap
2016-09-30 16:04 ` Igor Druzhinin
2 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2016-09-30 14:48 UTC (permalink / raw)
To: George Dunlap, Igor Druzhinin, xen-devel
Cc: george.dunlap, Olaf Hering, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 2867 bytes --]
On 30/09/16 15:46, George Dunlap wrote:
> On 29/09/16 14:53, Igor Druzhinin wrote:
>> > As long as t_info_first_offset is calculated in uint32_t offsets we need to
>> > multiply it by sizeof(uint32_t) in order to get the right number of pages
>> > for trace metadata. Not doing that makes it impossible to read the trace
>> > buffer correctly from userspace for some corner cases.
>> >
>> > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
> correct formula to calculate t_info_pages". But that one was presumably
> written (and Acked by me) because the variable name there,
> t_info_first_offset, is confusing.
>
> The other mistake in fbf96e6 is that before t_info_words was actually
> denominated in words; but after it's denominated in bytes (which is
> again confusing).
>
> What about something like the attached instead? This should fix your
> problem while making the code clearer.
>
> -George
>
>
>
> From b0252cec4a96fea28faa85c47ab0f5d5997444ad Mon Sep 17 00:00:00 2001
> From: George Dunlap <george.dunlap@citrix.com>
> Date: Fri, 30 Sep 2016 15:42:56 +0100
> Subject: [PATCH] xen/trace: Fix trace metadata page count calculation (revert
> fbf96e6)
>
> Changeset fbf96e6, "xentrace: correct formula to calculate
> t_info_pages", broke the trace metadata page count calculation, by
> mistaking t_info_first_offset as denominated in bytes, when in fact it
> is denominated in words (uint32_t).
>
> Effectively revert that change, and put a comment there to reduce the
> chance that someone will make that mistake in the future.
>
> Spotted-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Olaf Hering <olaf@aepfle.de>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
> xen/common/trace.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/trace.c b/xen/common/trace.c
> index f651cf3..2f4ecca 100644
> --- a/xen/common/trace.c
> +++ b/xen/common/trace.c
> @@ -148,8 +148,12 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
> pages = max_pages;
> }
>
> - t_info_words = num_online_cpus() * pages * sizeof(uint32_t);
> - t_info_pages = PFN_UP(t_info_first_offset + t_info_words);
> + /*
> + * NB this calculation is correct, because t_info_first_offset is
> + * in words, not bytes, not bytes
s/, not bytes$/./
~Andrew
> + */
> + t_info_words = num_online_cpus() * pages + t_info_first_offset;
> + t_info_pages = PFN_UP(t_info_words * sizeof(uint32_t));
> printk(XENLOG_INFO "xentrace: requesting %u t_info pages "
> "for %u trace pages on %u cpus\n",
> t_info_pages, pages, num_online_cpus());
> -- 2.1.4
[-- Attachment #1.2: Type: text/html, Size: 4969 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata
2016-09-30 14:46 ` George Dunlap
2016-09-30 14:48 ` Andrew Cooper
@ 2016-09-30 14:48 ` George Dunlap
2016-09-30 16:04 ` Igor Druzhinin
2 siblings, 0 replies; 7+ messages in thread
From: George Dunlap @ 2016-09-30 14:48 UTC (permalink / raw)
To: Igor Druzhinin, xen-devel
Cc: george.dunlap, Andrew Cooper, Olaf Hering, Jan Beulich
On 30/09/16 15:46, George Dunlap wrote:
> On 29/09/16 14:53, Igor Druzhinin wrote:
>> As long as t_info_first_offset is calculated in uint32_t offsets we need to
>> multiply it by sizeof(uint32_t) in order to get the right number of pages
>> for trace metadata. Not doing that makes it impossible to read the trace
>> buffer correctly from userspace for some corner cases.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>
> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
> correct formula to calculate t_info_pages". But that one was presumably
> written (and Acked by me) because the variable name there,
> t_info_first_offset, is confusing.
>
> The other mistake in fbf96e6 is that before t_info_words was actually
> denominated in words; but after it's denominated in bytes (which is
> again confusing).
>
> What about something like the attached instead? This should fix your
> problem while making the code clearer.
Obviously that comment shouldn't include the second "not bytes". We're
not writing folk songs here...
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata
2016-09-30 14:46 ` George Dunlap
2016-09-30 14:48 ` Andrew Cooper
2016-09-30 14:48 ` George Dunlap
@ 2016-09-30 16:04 ` Igor Druzhinin
2016-09-30 16:12 ` George Dunlap
2 siblings, 1 reply; 7+ messages in thread
From: Igor Druzhinin @ 2016-09-30 16:04 UTC (permalink / raw)
To: George Dunlap, xen-devel
Cc: george.dunlap, Andrew Cooper, Olaf Hering, Jan Beulich
On 30/09/16 15:46, George Dunlap wrote:
> On 29/09/16 14:53, Igor Druzhinin wrote:
>> As long as t_info_first_offset is calculated in uint32_t offsets we need to
>> multiply it by sizeof(uint32_t) in order to get the right number of pages
>> for trace metadata. Not doing that makes it impossible to read the trace
>> buffer correctly from userspace for some corner cases.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>
> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
> correct formula to calculate t_info_pages". But that one was presumably
> written (and Acked by me) because the variable name there,
> t_info_first_offset, is confusing.
>
> The other mistake in fbf96e6 is that before t_info_words was actually
> denominated in words; but after it's denominated in bytes (which is
> again confusing).
>
> What about something like the attached instead? This should fix your
> problem while making the code clearer.
>
> -George
>
>
Reviewed-by: Igor Druzhinin <igor.druzhinin@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata
2016-09-30 16:04 ` Igor Druzhinin
@ 2016-09-30 16:12 ` George Dunlap
2016-10-04 9:59 ` Igor Druzhinin
0 siblings, 1 reply; 7+ messages in thread
From: George Dunlap @ 2016-09-30 16:12 UTC (permalink / raw)
To: Igor Druzhinin, xen-devel
Cc: george.dunlap, Andrew Cooper, Olaf Hering, Jan Beulich
On 30/09/16 17:04, Igor Druzhinin wrote:
> On 30/09/16 15:46, George Dunlap wrote:
>> On 29/09/16 14:53, Igor Druzhinin wrote:
>>> As long as t_info_first_offset is calculated in uint32_t offsets we
>>> need to
>>> multiply it by sizeof(uint32_t) in order to get the right number of
>>> pages
>>> for trace metadata. Not doing that makes it impossible to read the trace
>>> buffer correctly from userspace for some corner cases.
>>>
>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>
>> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
>> correct formula to calculate t_info_pages". But that one was presumably
>> written (and Acked by me) because the variable name there,
>> t_info_first_offset, is confusing.
>>
>> The other mistake in fbf96e6 is that before t_info_words was actually
>> denominated in words; but after it's denominated in bytes (which is
>> again confusing).
>>
>> What about something like the attached instead? This should fix your
>> problem while making the code clearer.
>>
>> -George
>>
>>
>
> Reviewed-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Thanks. Any chance I could get a Tested-by as well? :-)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata
2016-09-30 16:12 ` George Dunlap
@ 2016-10-04 9:59 ` Igor Druzhinin
0 siblings, 0 replies; 7+ messages in thread
From: Igor Druzhinin @ 2016-10-04 9:59 UTC (permalink / raw)
To: George Dunlap, xen-devel
Cc: george.dunlap, Andrew Cooper, Olaf Hering, Jan Beulich
Checked that.
Tested-by: Igor Druzhinin <igor.druzhinin@citrix.com>
On 30/09/16 17:12, George Dunlap wrote:
> On 30/09/16 17:04, Igor Druzhinin wrote:
>> On 30/09/16 15:46, George Dunlap wrote:
>>> On 29/09/16 14:53, Igor Druzhinin wrote:
>>>> As long as t_info_first_offset is calculated in uint32_t offsets we
>>>> need to
>>>> multiply it by sizeof(uint32_t) in order to get the right number of
>>>> pages
>>>> for trace metadata. Not doing that makes it impossible to read the trace
>>>> buffer correctly from userspace for some corner cases.
>>>>
>>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>>
>>> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
>>> correct formula to calculate t_info_pages". But that one was presumably
>>> written (and Acked by me) because the variable name there,
>>> t_info_first_offset, is confusing.
>>>
>>> The other mistake in fbf96e6 is that before t_info_words was actually
>>> denominated in words; but after it's denominated in bytes (which is
>>> again confusing).
>>>
>>> What about something like the attached instead? This should fix your
>>> problem while making the code clearer.
>>>
>>> -George
>>>
>>>
>>
>> Reviewed-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>
> Thanks. Any chance I could get a Tested-by as well? :-)
>
> -George
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-10-04 9:59 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-29 13:53 [PATCH] trace: Fix incorrect number of pages used for trace metadata Igor Druzhinin
2016-09-30 14:46 ` George Dunlap
2016-09-30 14:48 ` Andrew Cooper
2016-09-30 14:48 ` George Dunlap
2016-09-30 16:04 ` Igor Druzhinin
2016-09-30 16:12 ` George Dunlap
2016-10-04 9:59 ` Igor Druzhinin
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.