| Message ID | dbbb6a2e-bde3-365a-ad72-c78cf2f35912@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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>
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
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 >
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