[PATCH] fix no output boot_time event "TLS initialization"

[Wang Yu]

before patch boot with --bootchart, "TLS initialization"
is not output
...
disk read (real mode): 50.71ms, (+50.71ms)
.init functions: 89.20ms, (+5.48ms)
SMP launched: 90.56ms, (+1.36ms)
VFS initialized: 95.76ms, (+5.20ms)
...
after patch,
...
disk read (real mode): 50.71ms, (+50.71ms)
TLS initialization: 83.72ms, (+33.01ms)
.init functions: 89.20ms, (+5.48ms)
SMP launched: 90.56ms, (+1.36ms)
VFS initialized: 95.76ms, (+5.20ms)
...
Signed-off-by: Wang Yu <yuw...@linux.alibaba.com>
---
include/osv/boot.hh | 4 ++--
loader.cc | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/osv/boot.hh b/include/osv/boot.hh
index eb97cc9..ec5301c 100644
--- a/include/osv/boot.hh
+++ b/include/osv/boot.hh
@@ -15,13 +15,13 @@ public:
void print_chart();
time_element arrays[16];
friend void arch_setup_free_memory();
-private:
// Can we keep it at 0 and let the initial two users increment it? No, we
// cannot. The reason is that the code that *parses* those fields run
// relatively late (the code that takes the measure is so early it cannot
// call this one directly. Therefore, the measurements would appear in the
// middle of the list, and we want to preserve order.
- int _event = 2;
+ int _event = 3;
+private:

void print_one_time(int index);
double to_msec(u64 time);
diff --git a/loader.cc b/loader.cc
index f6cbd4d..0d97151 100644
--- a/loader.cc
+++ b/loader.cc
@@ -109,6 +109,7 @@ void premain()
}

setup_tls(inittab);
+ boot_time._event=2;
boot_time.event("TLS initialization");
for (auto init = inittab.start; init < inittab.start + inittab.count; ++init) {
(*init)();
--
1.8.3.1

[Wang Yu]

[Waldek Kozaczuk]

Nice catch.

Even though the patch addresses the bug it also makes _event member variable public and therefore weakens encapsulation. I wonder if we can address this bug along these lines:

diff --git a/arch/x64/arch-setup.cc b/arch/x64/arch-setup.cc

index 325c26a..e4c1ab0 100644

--- a/arch/x64/arch-setup.cc

+++ b/arch/x64/arch-setup.cc

@@ -134,11 +134,11 @@ void arch_setup_free_memory()

     u64 time;

     time = omb.tsc_init_hi;

     time = (time << 32) | omb.tsc_init;

-    boot_time.arrays[0] = { "", time };

+    boot_time.event(0, "", time );

 

     time = omb.tsc_disk_done_hi;

     time = (time << 32) | omb.tsc_disk_done;

-    boot_time.arrays[1] = { "disk read (real mode)", time };

+    boot_time.event(1, "disk read (real mode)", time );

 

     auto c = processor::cpuid(0x80000000);

     if (c.a >= 0x80000008) {

diff --git a/core/chart.cc b/core/chart.cc

index 86153a2..59311a9 100644

--- a/core/chart.cc

+++ b/core/chart.cc

@@ -20,8 +20,18 @@ void boot_time_chart::print_one_time(int index)

 

 void boot_time_chart::event(const char *str)

 {

-    arrays[_event].str  = str;

-    arrays[_event++].stamp = processor::ticks();

+    event(_event++, str, processor::ticks());

+}

+

+void boot_time_chart::event(int event_idx, const char *str)

+{

+    event(event_idx, str, processor::ticks());

+}

+

+void boot_time_chart::event(int event_idx, const char *str, u64 stamp)

+{

+    arrays[event_idx].str = str;

+    arrays[event_idx].stamp = stamp;

 }

 

 void boot_time_chart::print_chart()

diff --git a/include/osv/boot.hh b/include/osv/boot.hh

index eb97cc9..391ad8f 100644

--- a/include/osv/boot.hh

+++ b/include/osv/boot.hh

@@ -12,16 +12,17 @@ public:

 class boot_time_chart {

 public:

     void event(const char *str);

+    void event(int event_idx, const char *str);

+    void event(int event_idx, const char *str, u64 stamp);

     void print_chart();

-    time_element arrays[16];

-    friend void arch_setup_free_memory();

 private:

     // Can we keep it at 0 and let the initial two users increment it?  No, we

     // cannot. The reason is that the code that *parses* those fields run

     // relatively late (the code that takes the measure is so early it cannot

     // call this one directly. Therefore, the measurements would appear in the

     // middle of the list, and we want to preserve order.

-    int _event = 2;

+    int _event = 3;

+    time_element arrays[16];

 

     void print_one_time(int index);

     double to_msec(u64 time);

diff --git a/loader.cc b/loader.cc

index f6cbd4d..de1d772 100644

--- a/loader.cc

+++ b/loader.cc

@@ -109,7 +109,8 @@ void premain()

     }

 

     setup_tls(inittab);

-    boot_time.event("TLS initialization");

+    debug("After TLS initialization");

+    boot_time.event(2, "TLS initialization");

     for (auto init = inittab.start; init < inittab.start + inittab.count; ++init) {

         (*init)();

     }

Waldek

[Wang Yu]

no one review?


在 18/4/9 下午8:37, Wang Yu 写道:

[Waldek Kozaczuk]

Hi.

I reviewed this and other patch and replied on the the mailing list. Have my replies not reached you ?

Waldek

Sent from my iPhone

> --
> You received this message because you are subscribed to a topic in the Google Groups "OSv Development" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/osv-dev/XWBt6oDA6_4/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to osv-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Wang Yu]

在 18/4/12 上午10:22, Waldek Kozaczuk 写道:
> Hi.
>
> I reviewed this and other patch and replied on the the mailing list. Have my replies not reached you ？.
I haven't recieved it, maybe i am not in the mail list, and you re send
reviewed mail to me?

[Waldek Kozaczuk]

Including the author of the patch. 

Sent from my iPhone

[Wang Yu]

在 18/4/12 上午10:50, Waldek Kozaczuk 写道:

Including the author of the patch. 

Sent from my iPhone

On Apr 10, 2018, at 23:44, Waldek Kozaczuk <jwkoz...@gmail.com> wrote:

Nice catch.

Even though the patch addresses the bug it also makes _event member variable public and therefore weakens encapsulation. I wonder if we can address this bug along these lines:

i agree it, thanks for your review,
then i commit v2 patch, and add reviewer Waldek Kozaczuk <jwkoz...@gmail.com>
ok?

[Waldek Kozaczuk]

Sound good. Thanks. 

Sent from my iPhone