[for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[Michal Orzel]

Using dprintk results in printing additionally file name and line
number. This is something we do not want when printing regular
information unconditionally as it looks like as if there was some issue.
It also makes the logging inconsistent.

Fix this by switching to printk because this information may also be
helpful on the release builds (it would still require setting loglvl to
"info" or lower level).

Fixes: 5597f32f409c ("xen/arm: assign static shared memory to the default owner dom_io")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
Rationale for taking this patch for 4.17:
Current code results in an abnormal behavior [1] and was introduced by
the 4.17 feature (static shared memory). Even though it can only be seen
on a debug build, it should be fixed now so that we have a consistent
behavior across all the logs.

[1]:
(XEN) arch/arm/domain_build.c:847: d0: allocate static shared memory BANK 0x00000070000000-0x00000080000000.
---
 xen/arch/arm/domain_build.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 01c2aaccd82d..f47e77876a25 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -844,9 +844,9 @@ static int __init assign_shared_memory(struct domain *d,
     unsigned long nr_pages, nr_borrowers, i;
     struct page_info *page;
 
-    dprintk(XENLOG_INFO,
-            "%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
-            d, pbase, pbase + psize);
+    printk(XENLOG_INFO
+           "%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
+           d, pbase, pbase + psize);
 
     smfn = acquire_shared_memory_bank(d, pbase, psize);
     if ( mfn_eq(smfn, INVALID_MFN) )
-- 
2.25.1

Re: [for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[Julien Grall]

Hi,

On 16/09/2022 08:19, Michal Orzel wrote:
> Using dprintk results in printing additionally file name and line
> number. This is something we do not want when printing regular
> information unconditionally as it looks like as if there was some issue.
I am OK if you want to switch to a printk() but I disagree with this 
argument. dprintk() is not about error, it is about anything that 
doesn't matter in release build.

I don't think we should just switch to printk() because dprintk() add 
the line/file. There are message we don't necessarily want to have in 
release build. So dprintk(XENLOG_INFO, ...) would be right for them.

Personally, I find them useful as there no grep required and/or 
confusion (but that's a matter of taste). If it were me, I would add the 
line/file everywhere. But I understand this takes space in the binary 
(hence why this is not present in release build).

A better argument to switch to printk() is this information is useful to 
the user even outside of the debug build.

> 
> Fix this by switching to printk because this information may also be
> helpful on the release builds (it would still require setting loglvl to
> "info" or lower level).

I think we should drop XENLOG_INFO to be consistent with the other 
printk() in domain_build.c (after all this is a domain information like 
the other) or use XENLOG_INFO everywhere.

My preference will be the former because otherwise most of the 
information will not printed in release build by default.

> 
> Fixes: 5597f32f409c ("xen/arm: assign static shared memory to the default owner dom_io")

Fixes should only be used for bugs. This is not one.

> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> Rationale for taking this patch for 4.17:
> Current code results in an abnormal behavior [1] and was introduced by

It is not abnormal (see above). This is an expected behavior when you 
use dprintk().

> the 4.17 feature (static shared memory). Even though it can only be seen
> on a debug build, it should be fixed now so that we have a consistent
> behavior across all the logs.

As I wrote above, I agree this should be printed in release build. But I 
disagree with your arguments.

Cheers,

-- 
Julien Grall

Re: [for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[Michal Orzel]

Hi Julien,

On 16/09/2022 10:08, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 16/09/2022 08:19, Michal Orzel wrote:
>> Using dprintk results in printing additionally file name and line
>> number. This is something we do not want when printing regular
>> information unconditionally as it looks like as if there was some issue.
> I am OK if you want to switch to a printk() but I disagree with this
> argument. dprintk() is not about error, it is about anything that
> doesn't matter in release build.

In the vast majority of cases, dprintk is used conditionally. That is why
in the debug build you cannot spot a single line of log starting with
a file name + line number. That is why I assume this behaviorto be abnormal
compared to all the other logs.

If someone adds a printk starting with e.g. "$$$" this is also not a bad
usage of printk but would result in an inconsistent behavior.

> 
> I don't think we should just switch to printk() because dprintk() add
> the line/file. There are message we don't necessarily want to have in
> release build. So dprintk(XENLOG_INFO, ...) would be right for them.

I think this is a matter of being consistent.
We do not have a helper to add printk only for a debug build but without adding
filename/line number. That is why almost all the dprintks are used conditionally.

> 
> Personally, I find them useful as there no grep required and/or
> confusion (but that's a matter of taste). If it were me, I would add the
> line/file everywhere. But I understand this takes space in the binary
> (hence why this is not present in release build).
> 
> A better argument to switch to printk() is this information is useful to
> the user even outside of the debug build.
> 
>>
>> Fix this by switching to printk because this information may also be
>> helpful on the release builds (it would still require setting loglvl to
>> "info" or lower level).
> 
> I think we should drop XENLOG_INFO to be consistent with the other
> printk() in domain_build.c (after all this is a domain information like
> the other) or use XENLOG_INFO everywhere.
> 
> My preference will be the former because otherwise most of the
> information will not printed in release build by default.
> 
>>
>> Fixes: 5597f32f409c ("xen/arm: assign static shared memory to the default owner dom_io")
> 
> Fixes should only be used for bugs. This is not one.
> 
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> Rationale for taking this patch for 4.17:
>> Current code results in an abnormal behavior [1] and was introduced by
> 
> It is not abnormal (see above). This is an expected behavior when you
> use dprintk().

I did not mean abnormal behavior of dprintk but abnormal behavior of logging
even on debug builds. As I said before, I could not spot any message like this
booting Xen at all. This is why I took this as a reference for "normal" behavior.

> 
>> the 4.17 feature (static shared memory). Even though it can only be seen
>> on a debug build, it should be fixed now so that we have a consistent
>> behavior across all the logs.
> 
> As I wrote above, I agree this should be printed in release build. But I
> disagree with your arguments.
> 
> Cheers,
> 
> --
> Julien Grall

~Michal

Re: [for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[Jan Beulich]

On 16.09.2022 10:32, Michal Orzel wrote:
> On 16/09/2022 10:08, Julien Grall wrote:
>> On 16/09/2022 08:19, Michal Orzel wrote:
>>> Using dprintk results in printing additionally file name and line
>>> number. This is something we do not want when printing regular
>>> information unconditionally as it looks like as if there was some issue.
>> I am OK if you want to switch to a printk() but I disagree with this
>> argument. dprintk() is not about error, it is about anything that
>> doesn't matter in release build.
> 
> In the vast majority of cases, dprintk is used conditionally. That is why
> in the debug build you cannot spot a single line of log starting with
> a file name + line number. That is why I assume this behaviorto be abnormal
> compared to all the other logs.
> 
> If someone adds a printk starting with e.g. "$$$" this is also not a bad
> usage of printk but would result in an inconsistent behavior.
> 
>>
>> I don't think we should just switch to printk() because dprintk() add
>> the line/file. There are message we don't necessarily want to have in
>> release build. So dprintk(XENLOG_INFO, ...) would be right for them.
> 
> I think this is a matter of being consistent.
> We do not have a helper to add printk only for a debug build but without adding
> filename/line number. That is why almost all the dprintks are used conditionally.

FWIW I agree with Julien and I don't view the "conditional" aspect as
relevant to decide whether to use printk() or dprintk().

Jan

Re: [for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[Julien Grall]

On 16/09/2022 09:32, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 16/09/2022 10:08, Julien Grall wrote:
>>
>>
>> Hi,
>>
>> On 16/09/2022 08:19, Michal Orzel wrote:
>>> Using dprintk results in printing additionally file name and line
>>> number. This is something we do not want when printing regular
>>> information unconditionally as it looks like as if there was some issue.
>> I am OK if you want to switch to a printk() but I disagree with this
>> argument. dprintk() is not about error, it is about anything that
>> doesn't matter in release build.
> 
> In the vast majority of cases, dprintk is used conditionally. That is why
> in the debug build you cannot spot a single line of log starting with
> a file name + line number. 
> That is why I assume this behaviorto be abnormal
> compared to all the other logs.
> 
> If someone adds a printk starting with e.g. "$$$" this is also not a bad
> usage of printk but would result in an inconsistent behavior.
Every lines are different, so it is not clear what you mean by 
inconsistent here. For instance, we have quite a few lines starting with 
the subsystem (your $$$) but not all of them. Would that be inconsistent 
to you?

> 
>>
>> I don't think we should just switch to printk() because dprintk() add
>> the line/file. There are message we don't necessarily want to have in
>> release build. So dprintk(XENLOG_INFO, ...) would be right for them.
> 
> I think this is a matter of being consistent.

You can't really argue about consistency without explaining what is a 
consistent line.

As I wrote above, a message is mostly a free form. Some may use 'rc=%d' 
other 'error %d'...

Yes it would be good if all the errors are printed the same way. 
However, this needs to be a tree-wide decision rather than localized and 
something really not worth the argument.

>> Personally, I find them useful as there no grep required and/or
>> confusion (but that's a matter of taste). If it were me, I would add the
>> line/file everywhere. But I understand this takes space in the binary
>> (hence why this is not present in release build).
>>
>> A better argument to switch to printk() is this information is useful to
>> the user even outside of the debug build.
>>
>>>
>>> Fix this by switching to printk because this information may also be
>>> helpful on the release builds (it would still require setting loglvl to
>>> "info" or lower level).
>>
>> I think we should drop XENLOG_INFO to be consistent with the other
>> printk() in domain_build.c (after all this is a domain information like
>> the other) or use XENLOG_INFO everywhere.
>>
>> My preference will be the former because otherwise most of the
>> information will not printed in release build by default.
>>
>>>
>>> Fixes: 5597f32f409c ("xen/arm: assign static shared memory to the default owner dom_io")
>>
>> Fixes should only be used for bugs. This is not one.
>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>> Rationale for taking this patch for 4.17:
>>> Current code results in an abnormal behavior [1] and was introduced by
>>
>> It is not abnormal (see above). This is an expected behavior when you
>> use dprintk().
> 
> I did not mean abnormal behavior of dprintk but abnormal behavior of logging
> even on debug builds. As I said before, I could not spot any message like this
> booting Xen at all. This is why I took this as a reference for "normal" behavior.

To me "abnormal" is quite a strong word and in this situation really a 
matter of taste.

Anyway, there are way to write the commit message in a more objective 
way. Some like:

xen/arm: domain_build: Always print the static memory region

At the moment, the static memory region are only printed during debug 
build. The information could be helpful for the end user (which may not 
be the same as the person building the package). So switch to printk().

Cheers,

-- 
Julien Grall

Re: [for-4.17] xen/arm: domain_build: Do not use dprintk unconditionally

[Michal Orzel]

On 16/09/2022 12:03, Julien Grall wrote:
> 
> 
> On 16/09/2022 09:32, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 16/09/2022 10:08, Julien Grall wrote:
>>>
>>>
>>> Hi,
>>>
>>> On 16/09/2022 08:19, Michal Orzel wrote:
>>>> Using dprintk results in printing additionally file name and line
>>>> number. This is something we do not want when printing regular
>>>> information unconditionally as it looks like as if there was some issue.
>>> I am OK if you want to switch to a printk() but I disagree with this
>>> argument. dprintk() is not about error, it is about anything that
>>> doesn't matter in release build.
>>
>> In the vast majority of cases, dprintk is used conditionally. That is why
>> in the debug build you cannot spot a single line of log starting with
>> a file name + line number.
>> That is why I assume this behaviorto be abnormal
>> compared to all the other logs.
>>
>> If someone adds a printk starting with e.g. "$$$" this is also not a bad
>> usage of printk but would result in an inconsistent behavior.
> Every lines are different, so it is not clear what you mean by
> inconsistent here. For instance, we have quite a few lines starting with
> the subsystem (your $$$) but not all of them. Would that be inconsistent
> to you?
No, it would not. The consistency I refer to is that during the "normal" build
(by normal I mean without any warning/error conditions), we cannot spot a single
line in the logfile that starts with a file name and line number. Something like
this immediately catches at least my attention as such format is really meant
for either errors or debug prints (the reason why most of the dprintks are
used conditionally is because we do not have an alternative as I mentioned before).
There is no reason why someone would want to see the file/line of such informative message.

Printing filename and line number is not the same as printing subsystem name.
There must be a good reason for choosing the former. It shall either be used
during the error condition or with loglvl debug. Certainly not with XENLOG_INFO.

Anyway, if I'm the only one who does not get used to seeing such messages prepended
with file/line, then let's not spend too much time discussing what is the consistency
because it is clearly a matter of taste :)

> 
>>
>>>
>>> I don't think we should just switch to printk() because dprintk() add
>>> the line/file. There are message we don't necessarily want to have in
>>> release build. So dprintk(XENLOG_INFO, ...) would be right for them.
>>
>> I think this is a matter of being consistent.
> 
> You can't really argue about consistency without explaining what is a
> consistent line.
> 
> As I wrote above, a message is mostly a free form. Some may use 'rc=%d'
> other 'error %d'...
> 
> Yes it would be good if all the errors are printed the same way.
> However, this needs to be a tree-wide decision rather than localized and
> something really not worth the argument.
> 
>>> Personally, I find them useful as there no grep required and/or
>>> confusion (but that's a matter of taste). If it were me, I would add the
>>> line/file everywhere. But I understand this takes space in the binary
>>> (hence why this is not present in release build).
>>>
>>> A better argument to switch to printk() is this information is useful to
>>> the user even outside of the debug build.
>>>
>>>>
>>>> Fix this by switching to printk because this information may also be
>>>> helpful on the release builds (it would still require setting loglvl to
>>>> "info" or lower level).
>>>
>>> I think we should drop XENLOG_INFO to be consistent with the other
>>> printk() in domain_build.c (after all this is a domain information like
>>> the other) or use XENLOG_INFO everywhere.
>>>
>>> My preference will be the former because otherwise most of the
>>> information will not printed in release build by default.
>>>
>>>>
>>>> Fixes: 5597f32f409c ("xen/arm: assign static shared memory to the default owner dom_io")
>>>
>>> Fixes should only be used for bugs. This is not one.
>>>
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>> ---
>>>> Rationale for taking this patch for 4.17:
>>>> Current code results in an abnormal behavior [1] and was introduced by
>>>
>>> It is not abnormal (see above). This is an expected behavior when you
>>> use dprintk().
>>
>> I did not mean abnormal behavior of dprintk but abnormal behavior of logging
>> even on debug builds. As I said before, I could not spot any message like this
>> booting Xen at all. This is why I took this as a reference for "normal" behavior.
> 
> To me "abnormal" is quite a strong word and in this situation really a
> matter of taste.
> 
> Anyway, there are way to write the commit message in a more objective
> way. Some like:
> 
> xen/arm: domain_build: Always print the static memory region
> 
> At the moment, the static memory region are only printed during debug
> build. The information could be helpful for the end user (which may not
> be the same as the person building the package). So switch to printk().
> 
> Cheers,
> 
> --
> Julien Grall