[PATCH 1/3] drmgr: don't open sysfs file for each command

[Laurent Dufour]

The new __do_kernel_dlpar() API will be used in later commit to remove by
DRC Index LMB per LMB. This will avoiding opennig and closing the fd each
time.

The fd closing will now be done at the process exit time.

In addition add an optinal parameter to silently ignore some error.

Also, change the log level of the "success" message to debug to match
the previous one saying "Trying.."

Signed-off-by: Laurent Dufour <ldu...@linux.ibm.com>
---
src/drmgr/common.c | 22 +++++++++++++---------
src/drmgr/dr.h | 3 ++-
2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/drmgr/common.c b/src/drmgr/common.c
index 5e8135bcf77e..25d244cb2f57 100644
--- a/src/drmgr/common.c
+++ b/src/drmgr/common.c
@@ -1469,32 +1469,36 @@ int kernel_dlpar_exists(void)
* @param cmd command string to write to sysfs
* @returns 0 on success, !0 otherwise
*/
-int do_kernel_dlpar(const char *cmd, int cmdlen)
+int __do_kernel_dlpar(const char *cmd, int cmdlen, int silent_error)
{
- int fd, rc;
+ static int fd = -1;
+ int rc;
int my_errno;

say(DEBUG, "Initiating kernel DLPAR \"%s\"\n", cmd);

/* write to file */
- fd = open(SYSFS_DLPAR_FILE, O_WRONLY);
- if (fd <= 0) {
- say(ERROR, "Could not open %s to initiate DLPAR request\n",
- SYSFS_DLPAR_FILE);
- return -1;
+ if (fd == -1) {
+ fd = open(SYSFS_DLPAR_FILE, O_WRONLY);
+ if (fd <= 0) {
+ say(ERROR, "Could not open %s to initiate DLPAR request\n",
+ SYSFS_DLPAR_FILE);
+ return -1;
+ }
}

rc = write(fd, cmd, cmdlen);
my_errno = errno;
- close(fd);
if (rc <= 0) {
+ if (silent_error)
+ return (my_errno == 0) ? -1 : -my_errno;
/* write does not set errno for rc == 0 */
say(ERROR, "Failed to write to %s: %s\n", SYSFS_DLPAR_FILE,
(rc == 0) ? "wrote 0 bytes" : strerror(my_errno));
return -1;
}

- say(INFO, "Success\n");
+ say(DEBUG, "Success\n");
return 0;
}

diff --git a/src/drmgr/dr.h b/src/drmgr/dr.h
index f171bfea73c3..00d2fffc9919 100644
--- a/src/drmgr/dr.h
+++ b/src/drmgr/dr.h
@@ -172,5 +172,6 @@ enum drc_type to_drc_type(const char *);
int handle_prrn(void);

int kernel_dlpar_exists(void);
-int do_kernel_dlpar(const char *, int);
+int __do_kernel_dlpar(const char *, int, int);
+#define do_kernel_dlpar(c, l) __do_kernel_dlpar(c, l, 0)
#endif
--
2.29.2

[Nathan Lynch]

Laurent Dufour <ldu...@linux.ibm.com> writes:

> The new __do_kernel_dlpar() API will be used in later commit to remove by
> DRC Index LMB per LMB. This will avoiding opennig and closing the fd each
> time.
>
> The fd closing will now be done at the process exit time.

OK. Seems reasonable to minimize open/close of /sys/kernel/dlpar if
drmgr could issue thousands of individual commands (writes) against it
for a single operation.

This is a latent bug, not introduced by your change, but the condition
here should be fd < 0 or fd == -1.

> + say(ERROR, "Could not open %s to initiate DLPAR request\n",
> + SYSFS_DLPAR_FILE);
> + return -1;
> + }
> }
>
> rc = write(fd, cmd, cmdlen);
> my_errno = errno;
> - close(fd);
> if (rc <= 0) {
> + if (silent_error)
> + return (my_errno == 0) ? -1 : -my_errno;

Just to confirm I'm understanding the purpose here -- the silent_error
flag is for suppressing 'Failed to write' messages, because there will
be a use case where it will be typical for many to occur for a single
operation (e.g. removing 1TB by individual DRC index) -- correct?

Once the close(fd) call is removed I think my_errno no longer has a
purpose, so it could be removed too.

> /* write does not set errno for rc == 0 */
> say(ERROR, "Failed to write to %s: %s\n", SYSFS_DLPAR_FILE,
> (rc == 0) ? "wrote 0 bytes" : strerror(my_errno));

More system call return code confusion, again not a problem with your
patch. I don't understand why the write would return anything but cmdlen
(success) or -1 (error).

> return -1;
> }
>
> - say(INFO, "Success\n");
> + say(DEBUG, "Success\n");

And I assume this is to reduce log noise as well.

> return 0;
> }
>
> diff --git a/src/drmgr/dr.h b/src/drmgr/dr.h
> index f171bfea73c3..00d2fffc9919 100644
> --- a/src/drmgr/dr.h
> +++ b/src/drmgr/dr.h
> @@ -172,5 +172,6 @@ enum drc_type to_drc_type(const char *);
> int handle_prrn(void);
>
> int kernel_dlpar_exists(void);
> -int do_kernel_dlpar(const char *, int);
> +int __do_kernel_dlpar(const char *, int, int);
> +#define do_kernel_dlpar(c, l) __do_kernel_dlpar(c, l, 0)

I'd prefer a static inline function.

Identifiers beginning with '__' are reserved in C and we should not
introduce more of them in this project. (Yes I see there are a few
violations already, but they should be fixed.)

[Laurent Dufour]

I do agree.

>
>> + say(ERROR, "Could not open %s to initiate DLPAR request\n",
>> + SYSFS_DLPAR_FILE);
>> + return -1;
>> + }
>> }
>>
>> rc = write(fd, cmd, cmdlen);
>> my_errno = errno;
>> - close(fd);
>> if (rc <= 0) {
>> + if (silent_error)
>> + return (my_errno == 0) ? -1 : -my_errno;
>
> Just to confirm I'm understanding the purpose here -- the silent_error
> flag is for suppressing 'Failed to write' messages, because there will
> be a use case where it will be typical for many to occur for a single
> operation (e.g. removing 1TB by individual DRC index) -- correct?

That's correct, some LMBs may be removable at a time, that's not a real issue
when removing a large area LMB per LMB.

>
> Once the close(fd) call is removed I think my_errno no longer has a
> purpose, so it could be removed too.

Nice catch ! I merged 2 commits here, but missed that.

>
>> /* write does not set errno for rc == 0 */
>> say(ERROR, "Failed to write to %s: %s\n", SYSFS_DLPAR_FILE,
>> (rc == 0) ? "wrote 0 bytes" : strerror(my_errno));
>
> More system call return code confusion, again not a problem with your
> patch. I don't understand why the write would return anything but cmdlen
> (success) or -1 (error).

Hum... That's an assumption I would not like to see in userspace. The write()'s
man page is explicit about that:

If no errors are detected, or error detection is not performed, 0 will
be returned without causing any other effect.

>> return -1;
>> }
>>
>> - say(INFO, "Success\n");
>> + say(DEBUG, "Success\n");
>
> And I assume this is to reduce log noise as well.

Yes, as I mentioned in the commit's description:

Also, change the log level of the "success" message to debug to match
the previous one saying "Trying.."

>

>> return 0;
>> }
>>
>> diff --git a/src/drmgr/dr.h b/src/drmgr/dr.h
>> index f171bfea73c3..00d2fffc9919 100644
>> --- a/src/drmgr/dr.h
>> +++ b/src/drmgr/dr.h
>> @@ -172,5 +172,6 @@ enum drc_type to_drc_type(const char *);
>> int handle_prrn(void);
>>
>> int kernel_dlpar_exists(void);
>> -int do_kernel_dlpar(const char *, int);
>> +int __do_kernel_dlpar(const char *, int, int);
>> +#define do_kernel_dlpar(c, l) __do_kernel_dlpar(c, l, 0)
>
> I'd prefer a static inline function.

Fair enough.

>
> Identifiers beginning with '__' are reserved in C and we should not
> introduce more of them in this project. (Yes I see there are a few
> violations already, but they should be fixed.)

Where does that statement, saying that '__' is reserved in C, come from?
This is very usual to declare __ prefixed function when underlying it is
proposing additional parameters that are not needed for most of the callers.

[Nathan Lynch]

Laurent Dufour <ldu...@linux.ibm.com> writes:
> Le 10/12/2020 à 05:53, Nathan Lynch a écrit :
>> Laurent Dufour <ldu...@linux.ibm.com> writes:
>>> /* write does not set errno for rc == 0 */
>>> say(ERROR, "Failed to write to %s: %s\n", SYSFS_DLPAR_FILE,
>>> (rc == 0) ? "wrote 0 bytes" : strerror(my_errno));
>>
>> More system call return code confusion, again not a problem with your
>> patch. I don't understand why the write would return anything but cmdlen
>> (success) or -1 (error).
>
> Hum... That's an assumption I would not like to see in userspace. The write()'s
> man page is explicit about that:
>
> If no errors are detected, or error detection is not performed, 0 will
> be returned without causing any other effect.

I see, thanks.

>>> - say(INFO, "Success\n");
>>> + say(DEBUG, "Success\n");
>>
>> And I assume this is to reduce log noise as well.
>
> Yes, as I mentioned in the commit's description:
>
> Also, change the log level of the "success" message to debug to match
> the previous one saying "Trying.."

This describes what the change does, and leaves the reasoning somewhat
implied, I think. I needed to think about why it would be good to match
the other log statement and just wanted to confirm.

>>> diff --git a/src/drmgr/dr.h b/src/drmgr/dr.h
>>> index f171bfea73c3..00d2fffc9919 100644
>>> --- a/src/drmgr/dr.h
>>> +++ b/src/drmgr/dr.h
>>> @@ -172,5 +172,6 @@ enum drc_type to_drc_type(const char *);
>>> int handle_prrn(void);
>>>
>>> int kernel_dlpar_exists(void);
>>> -int do_kernel_dlpar(const char *, int);
>>> +int __do_kernel_dlpar(const char *, int, int);
>>> +#define do_kernel_dlpar(c, l) __do_kernel_dlpar(c, l, 0)
>>

>> Identifiers beginning with '__' are reserved in C and we should not
>> introduce more of them in this project. (Yes I see there are a few
>> violations already, but they should be fixed.)
>
> Where does that statement, saying that '__' is reserved in C, come from?
> This is very usual to declare __ prefixed function when underlying it is
> proposing additional parameters that are not needed for most of the callers.

Yes, it's an entrenched practice in the Linux kernel and it's a useful
convention. Nevertheless:

"All identifiers that begin with an underscore and either an uppercase
letter or another underscore are always reserved for any use."

https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

The risk of nonconformance is low, but it's easy to avoid, so...
something like this?

int do_kernel_dlpar_common(const char *cmd, unsigned int len, int silent);

static inline int do_kernel_dlpar(const char *cmd, unsigned int len) {
return do_kernel_dlpar_common(cmd, 0);
}

static inline int try_kernel_dlpar(const char *cmd, unsigned int len) {
return do_kernel_dlpar_flags(cmd, 1);
}

[Laurent Dufour]

Le 10/12/2020 à 15:28, Nathan Lynch a écrit :
> Laurent Dufour <ldu...@linux.ibm.com> writes:
>> Le 10/12/2020 à 05:53, Nathan Lynch a écrit :
>>> Laurent Dufour <ldu...@linux.ibm.com> writes:
>>>> /* write does not set errno for rc == 0 */
>>>> say(ERROR, "Failed to write to %s: %s\n", SYSFS_DLPAR_FILE,
>>>> (rc == 0) ? "wrote 0 bytes" : strerror(my_errno));
>>>
>>> More system call return code confusion, again not a problem with your
>>> patch. I don't understand why the write would return anything but cmdlen
>>> (success) or -1 (error).
>>
>> Hum... That's an assumption I would not like to see in userspace. The write()'s
>> man page is explicit about that:
>>
>> If no errors are detected, or error detection is not performed, 0 will
>> be returned without causing any other effect.
>
> I see, thanks.
>
>
>>>> - say(INFO, "Success\n");
>>>> + say(DEBUG, "Success\n");
>>>
>>> And I assume this is to reduce log noise as well.
>>
>> Yes, as I mentioned in the commit's description:
>>
>> Also, change the log level of the "success" message to debug to match
>> the previous one saying "Trying.."
>
> This describes what the change does, and leaves the reasoning somewhat
> implied, I think. I needed to think about why it would be good to match
> the other log statement and just wanted to confirm.

Just because without that patch, with log levet set to INFO, you'll see in the
drmgr's output command the following lines:

Success
Success
Success
Success
Success
Success
Success
....

Not really helpful ;)

>
>
>>>> diff --git a/src/drmgr/dr.h b/src/drmgr/dr.h
>>>> index f171bfea73c3..00d2fffc9919 100644
>>>> --- a/src/drmgr/dr.h
>>>> +++ b/src/drmgr/dr.h
>>>> @@ -172,5 +172,6 @@ enum drc_type to_drc_type(const char *);
>>>> int handle_prrn(void);
>>>>
>>>> int kernel_dlpar_exists(void);
>>>> -int do_kernel_dlpar(const char *, int);
>>>> +int __do_kernel_dlpar(const char *, int, int);
>>>> +#define do_kernel_dlpar(c, l) __do_kernel_dlpar(c, l, 0)
>>>
>>> Identifiers beginning with '__' are reserved in C and we should not
>>> introduce more of them in this project. (Yes I see there are a few
>>> violations already, but they should be fixed.)
>>
>> Where does that statement, saying that '__' is reserved in C, come from?
>> This is very usual to declare __ prefixed function when underlying it is
>> proposing additional parameters that are not needed for most of the callers.
>
> Yes, it's an entrenched practice in the Linux kernel and it's a useful
> convention. Nevertheless:
>
> "All identifiers that begin with an underscore and either an uppercase
> letter or another underscore are always reserved for any use."
>
> https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

That's really interesting...
The norm this document is refering to is from 1999, has been cancelled, and
superseded by ISO/IEC 9899:2018. I can't check if that statement is still valid
in that new version (don't want to spend money for this) but seeing the amount
of code using the "__" prefix, I'm tented to say that this is no more valid.

>
> The risk of nonconformance is low, but it's easy to avoid, so...
> something like this?

Anyway, I like your proposal so let's go for a non rule violating
do_kernel_dlpar_common().

[Laurent Dufour]

The new __do_kernel_dlpar() API will be used in later commit to remove by
DRC Index LMB per LMB. This will avoiding opennig and closing the fd each
time.

The fd closing will now be done at the process exit time.

In addition add an optinal parameter to silently ignore some error.

Also, change the log level of the "success" message to debug to match
the previous one saying "Trying.."

Signed-off-by: Laurent Dufour <ldu...@linux.ibm.com>
---

src/drmgr/common.c | 30 +++++++++++++++++-------------
src/drmgr/dr.h | 6 +++++-
2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/src/drmgr/common.c b/src/drmgr/common.c
index 5e8135bcf77e..341777250feb 100644
--- a/src/drmgr/common.c
+++ b/src/drmgr/common.c
@@ -1462,39 +1462,43 @@ int kernel_dlpar_exists(void)
}

/**
- * do_kernel_dlpar
+ * do_kernel_dlpar_common
* @brief Use the in-kernel dlpar capabilities to perform the requested
* dlpar operation.
*

* @param cmd command string to write to sysfs

+ * @silent_error if not 0, error is not reported, it's up to the caller

* @returns 0 on success, !0 otherwise
*/
-int do_kernel_dlpar(const char *cmd, int cmdlen)

+int do_kernel_dlpar_common(const char *cmd, int cmdlen, int silent_error)
{
- int fd, rc;
- int my_errno;

+ static int fd = -1;
+ int rc;

say(DEBUG, "Initiating kernel DLPAR \"%s\"\n", cmd);

/* write to file */
- fd = open(SYSFS_DLPAR_FILE, O_WRONLY);
- if (fd <= 0) {
- say(ERROR, "Could not open %s to initiate DLPAR request\n",
- SYSFS_DLPAR_FILE);
- return -1;
+ if (fd == -1) {
+ fd = open(SYSFS_DLPAR_FILE, O_WRONLY);

+ if (fd < 0) {
+ say(ERROR,
+ "Could not open %s to initiate DLPAR request\n",

+ SYSFS_DLPAR_FILE);
+ return -1;
+ }
}

rc = write(fd, cmd, cmdlen);

- my_errno = errno;

- close(fd);
if (rc <= 0) {
+ if (silent_error)

+ return (errno == 0) ? -1 : -errno;

/* write does not set errno for rc == 0 */
say(ERROR, "Failed to write to %s: %s\n", SYSFS_DLPAR_FILE,

- (rc == 0) ? "wrote 0 bytes" : strerror(my_errno));
+ (rc == 0) ? "wrote 0 bytes" : strerror(errno));
return -1;

}

- say(INFO, "Success\n");
+ say(DEBUG, "Success\n");

return 0;
}

diff --git a/src/drmgr/dr.h b/src/drmgr/dr.h
index f171bfea73c3..ffbcfdb15cc0 100644
--- a/src/drmgr/dr.h
+++ b/src/drmgr/dr.h
@@ -172,5 +172,9 @@ enum drc_type to_drc_type(const char *);

int handle_prrn(void);

int kernel_dlpar_exists(void);
-int do_kernel_dlpar(const char *, int);

+int do_kernel_dlpar_common(const char *, int, int);
+static inline int do_kernel_dlpar(const char *cmd, int len)
+{
+ return do_kernel_dlpar_common(cmd, len, 0);
+}
#endif
--
2.29.2

[Tyrel Datwyler]

It was still valid as of ISO/IEC 9899:2011, and unlikely to change.

[Laurent Dufour]

Correct.

To close that interesting discussion about naming rules, I'd like to remind that
the chapter 7.1.3 of the ISO/IEC 9899:2017 mentioning restriction on "_" naming
is about library, not program code. Anyway, I don't want to waste more time
arguing about that underscore case, the V2 release is no more containing _
prefixed function, and ready for review / acceptance.

[Laurent Dufour]

The new __do_kernel_dlpar() API will be used in later commit to remove by
DRC Index LMB per LMB. This will avoiding opennig and closing the fd each
time.

The fd closing will now be done at the process exit time.

In addition add an optinal parameter to silently ignore some error.

Also, change the log level of the "success" message to debug to match
the previous one saying "Trying.."

/* write does not set errno for rc == 0 */
say(ERROR, "Failed to write to %s: %s\n", SYSFS_DLPAR_FILE,

- (rc == 0) ? "wrote 0 bytes" : strerror(my_errno));
+ (rc == 0) ? "wrote 0 bytes" : strerror(errno));
return -1;

}

- say(INFO, "Success\n");
+ say(DEBUG, "Success\n");

return 0;
}

diff --git a/src/drmgr/dr.h b/src/drmgr/dr.h
index f171bfea73c3..ffbcfdb15cc0 100644
--- a/src/drmgr/dr.h
+++ b/src/drmgr/dr.h
@@ -172,5 +172,9 @@ enum drc_type to_drc_type(const char *);

int handle_prrn(void);

int kernel_dlpar_exists(void);
-int do_kernel_dlpar(const char *, int);

[Nathan Lynch]

Laurent Dufour <ldu...@linux.ibm.com> writes:
> The new __do_kernel_dlpar() API will be used in later commit to remove by
> DRC Index LMB per LMB.

__do_kernel_dlpar() should be kernel_dlpar_common() here, but otherwise
LGTM.

[Laurent Dufour]

Good catch !