RFR : 8217786: Provide virtualization related info in the hs_error file on linux s390x
David Holmes
david.holmes at oracle.com
Wed Jan 30 05:49:01 UTC 2019
Hi Matthias,
Thanks for reworking this.
On 30/01/2019 2:56 am, Baesken, Matthias wrote:
> Hello, I added a break to avoid potential printing lines multiple times,
> and removed the comment line :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8217786.3/
A couple of minor comments:
src/hotspot/os/linux/os_linux.cpp
+ while (keywords_to_match[i]) {
Style nit: avoid implicit booleans, explicitly check != NULL
+ void os::Linux::print_virtualization_info(outputStream* st) {
Don't you want an initial print of some introductory text eg:
"Virtualization Information"
No need for updated webrev.
Thanks,
David
-----
> Best regards, Matthias
>
> *From:*Thomas Stüfe <thomas.stuefe at gmail.com>
> *Sent:* Dienstag, 29. Januar 2019 15:22
> *To:* Baesken, Matthias <matthias.baesken at sap.com>
> *Cc:* David Holmes <david.holmes at oracle.com>; hotspot-dev at openjdk.java.net
> *Subject:* Re: RFR : 8217786: Provide virtualization related info in the
> hs_error file on linux s390x
>
> Hi Matthias,
> + if (strncmp(line, keywords_to_match[i],
> strlen(keywords_to_match[i])) == 0) {
> + st->print("%s", line);
> + }
>
> you should break here otherwise a line containing multiple keywords will
> be printed multiple times.
>
> + // the LPAR / CPUs / VM - related infos usually come in blocks
>
> This comment can be removed.
>
> +#if defined(S390)
>
> I still do not like the arch specific code here, but for now I can live
> with it. Should this section grow and cover other architectures as well,
> we should fan out into os_linux_<cpu>.cpp.
>
> Thanks, Thomas
>
> On Tue, Jan 29, 2019 at 12:22 PM Baesken, Matthias
> <matthias.baesken at sap.com <mailto:matthias.baesken at sap.com>> wrote:
>
> Hello here is a 2nd webrev :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8217786.2/
>
> * Introduced static bool
> print_matching_lines_from_sysinfo_file(outputStream* st, const
> char* keywords_to_match[])
> * Moved call to Linux-only print_os_info
>
> Best regards, Matthias
>
> *From:*Thomas Stüfe <thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>>
> *Sent:* Dienstag, 29. Januar 2019 09:23
> *To:* Baesken, Matthias <matthias.baesken at sap.com
> <mailto:matthias.baesken at sap.com>>; David Holmes
> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>>
> *Cc:* hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>
> *Subject:* Re: RFR : 8217786: Provide virtualization related info in
> the hs_error file on linux s390x
>
> I'm still unhappy with that solution, since we have fanned out this
> coding for all architectures into the architecture independent
> os_linux.cpp. A generic "Show matching lines from given file" would
> be a better (slimmer, better reusable) solution IMHO.
>
> Side note: Could you please exchange strstr() .. with strncmp()
> since you require the start of the string to match. So no reason to
> parse the whole line if the start does not match.
>
> Cheers, Thomas
>
> On Tue, Jan 29, 2019 at 9:03 AM Baesken, Matthias
> <matthias.baesken at sap.com <mailto:matthias.baesken at sap.com>> wrote:
>
>
> >
> > No I was thinking more about just adding the virtualization
> info to an
> > existing step like print_os_info or print_cpu_info.
> >
>
> Hi David , print_cpu_info does not sound like a great fit .
> Some info like
>
> LPAR Number: 14
> LPAR Characteristics: Shared
> LPAR Name: VM12
>
> Does not really belong there .
>
> print_os_info looks better , it already contains
> "container_info" on Linux, so I think this might fit .
>
>
> Best regards, Matthias
>
>
> > -----Original Message-----
> > From: David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>
> > Sent: Dienstag, 29. Januar 2019 05:17
> > To: Baesken, Matthias <matthias.baesken at sap.com
> <mailto:matthias.baesken at sap.com>>; 'hotspot-
> > dev at openjdk.java.net <mailto:dev at openjdk.java.net>'
> <hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>>
> > Subject: Re: RFR : 8217786: Provide virtualization related
> info in the hs_error
> > file on linux s390x
> >
> > On 28/01/2019 10:23 pm, Baesken, Matthias wrote:
> > >>
> > >> Can't you include this information in an existing section
> of the error
> > >> processing code instead of adding a new function that is empty
> > >> everywhere except Linux?
> > >>
> > >
> > > Hi David , do you mean something like
> > >
> > >
> > > #if defined(S390)
> > >
> > > STEP("printing virtualization info")
> > > ...
> > >
> > > #endif
> >
> > No I was thinking more about just adding the virtualization
> info to an
> > existing step like print_os_info or print_cpu_info.
> >
> > Cheers,
> > David
> > -----
> >
> > > in vmError.cpp ?
> > >
> > > I thought about doing this.
> > >
> > >
> > > But on the other hand , the now still empty
> > os::pd_print_virtualization_info in platforms != linux
> > > might fill over time ( we could add [at least for
> some platforms] other
> > virtualization related info ).
> > >
> > >
> > > Best regards, Matthias
> > >
> > >
> > >> -----Original Message-----
> > >> From: David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>
> > >> Sent: Montag, 28. Januar 2019 12:35
> > >> To: Baesken, Matthias <matthias.baesken at sap.com
> <mailto:matthias.baesken at sap.com>>; 'hotspot-
> > >> dev at openjdk.java.net <mailto:dev at openjdk.java.net>'
> <hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>>
> > >> Subject: Re: RFR : 8217786: Provide virtualization related
> info in the
> > hs_error
> > >> file on linux s390x
> > >>
> > >> Hi Matthias,
> > >>
> > >> On 28/01/2019 6:48 pm, Baesken, Matthias wrote:
> > >>> Hello, please review this change ; it adds
> virtualization related info in
> > the
> > >> hs_error file on linux s390x .
> > >>
> > >> Can't you include this information in an existing section
> of the error
> > >> processing code instead of adding a new function that is empty
> > >> everywhere except Linux?
> > >>
> > >> Thanks,
> > >> David
> > >>
> > >>> On linux s390x, we usually (always?) run in
> virtualized environments
> > >> (LPAR and/or z/VM / KVM ).
> > >>>
> > >>> It is helpful for instance in support cases to get some
> information about
> > the
> > >> virtualized environment in the hs_error file .
> > >>> A lot of info can be taken from the /proc/sysinfo file on
> linux s390x .
> > >>>
> > >>>
> > >>> Bug/webrev :
> > >>>
> > >>> https://bugs.openjdk.java.net/browse/JDK-8217786
> > >>>
> > >>>
> > >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8217786.1/
> > >>>
> > >>>
> > >>>
> > >>> Best regards, Matthias
> > >>>
>
More information about the hotspot-dev
mailing list