Hi Martin,
Thanks for the quick reply.
On Mon, 2018-09-10 at 11:28 +0200, Martin Schwidefsky wrote:
> On Mon, 10 Sep 2018 11:11:03 +0200
> Jean Delvare <jdelvare@xxxxxxx> wrote:
>
> > While it is generally a good practice to store the result of a lookup
> > function in a local variable if you need it more than once, in this
> > specific case it is not such a good idea because both uses are
> > conditional (one depends on an error, the other on debugging). On top
> > of that, they are mutually exclusive, so in the worst case the result
> > of the lookup function will only be used once anyway.
> >
> > So better call qeth_get_ipa_cmd_name() where it is needed, so that in
> > the best (and most frequent) case it is never called at all. It
> > should improve the qeth driver performance a little bit.
> >
> > Signed-off-by: Jean Delvare <jdelvare@xxxxxxx>
> > Cc: Julian Wiedmann <jwi@xxxxxxxxxxxxx>
> > Cc: Ursula Braun <ubraun@xxxxxxxxxxxxx>
> > Cc: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
> > Cc: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
> > ---
> > Note: build-tested only.
> >
> > drivers/s390/net/qeth_core_main.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > --- linux-4.19-rc2.orig/drivers/s390/net/qeth_core_main.c 2018-09-08 15:19:47.225337133 +0200
> > +++ linux-4.19-rc2/drivers/s390/net/qeth_core_main.c 2018-09-10 10:41:15.552350012 +0200
> > @@ -609,18 +609,19 @@ static void qeth_put_reply(struct qeth_r
> > static void qeth_issue_ipa_msg(struct qeth_ipa_cmd *cmd, int rc,
> > struct qeth_card *card)
> > {
> > - const char *ipa_name;
> > int com = cmd->hdr.command;
> > - ipa_name = qeth_get_ipa_cmd_name(com);
> > +
> > if (rc)
> > QETH_DBF_MESSAGE(2, "IPA: %s(x%X) for %s/%s returned "
> > "x%X \"%s\"\n",
> > - ipa_name, com, dev_name(&card->gdev->dev),
> > + qeth_get_ipa_cmd_name(com), com,
> > + dev_name(&card->gdev->dev),
> > QETH_CARD_IFNAME(card), rc,
> > qeth_get_ipa_msg(rc));
> > else
> > QETH_DBF_MESSAGE(5, "IPA: %s(x%X) for %s/%s succeeded\n",
> > - ipa_name, com, dev_name(&card->gdev->dev),
> > + qeth_get_ipa_cmd_name(com), com,
> > + dev_name(&card->gdev->dev),
> > QETH_CARD_IFNAME(card));
> > }
> >
> >
>
> This does not make sense to me. "com" is read at the beginning of the function,
> the ipa_name is dependent on com only, no?
Correct, but I'm not sure where you are going with that. I do agree
that qeth_get_ipa_cmd_name(com) will return the same thing in both
branches, is that was your point. My proposal is not making any
functional changes, it only aims at avoiding useless function calls.
> Moving the call to qeth_get_ipa_cmd_name
> to the two statements of the if-then-else does not change a thing, in particular
> if you have an intelligent compiler it will promptly undo the indented effect of
> your patch..
I can't see how it could or would even want to. I know that the
compiler can reorder independent instructions for optimization
purposes, but as far as I know it is not allowed to execute functions
in anticipation "just in case" - the compiler has no idea what side
effects calling a function can have. Plus, all the benefits of the
optimization would potentially be lost or even negated by the cost of
the function call.
If you look at the preprocessed code with my patch applied (reformatted
because the output of gcc -E was barely readable), it looks like:
static void qeth_issue_ipa_msg(struct qeth_ipa_cmd *cmd, int rc,
struct qeth_card *card)
{
int com = cmd->hdr.command;
if (rc) {
debug_entry_t *__ret;
debug_info_t *__id = qeth_dbf[QETH_DBF_MSG].id;
int __level = 2;
if ((!__id) || (__level > __id->level))
__ret = ((void *)0);
else
__ret = __debug_sprintf_event(__id, __level,
"IPA: %s(x%X) for %s/%s returned x%X \"%s\"\n",
qeth_get_ipa_cmd_name(com), com,
dev_name(&card->gdev->dev),
(((card)->dev)? (card)->dev->name : ""),
rc, qeth_get_ipa_msg(rc));
} else {
debug_entry_t *__ret;
debug_info_t *__id = qeth_dbf[QETH_DBF_MSG].id;
int __level = 5;
if ((!__id) || (__level > __id->level))
__ret = ((void *)0);
else
__ret = __debug_sprintf_event(__id, __level,
"IPA: %s(x%X) for %s/%s succeeded\n",
qeth_get_ipa_cmd_name(com), com,
dev_name(&card->gdev->dev),
(((card)->dev)? (card)->dev->name : ""));
}
}
As you can see, each call to qeth_get_ipa_cmd_name() is behind a
conditional. Gcc simply can't call them if the branch in question is
not taken. The preprocessed code also illustrates how the sprintfs (and
thus the calls to qeth_get_ipa_cmd_name) are skipped in most cases,
which is what makes my proposed optimization valuable.
The fact that gcc does not move or merge the calls
to qeth_get_ipa_cmd_name() is further confirmed by inspecting the
assembly output of gcc -S. With my patch applied, there are two calling
sites for qeth_get_ipa_cmd_name() in qeth_issue_ipa_msg():
18127 .type qeth_issue_ipa_msg.isra.43, @function
18128 qeth_issue_ipa_msg.isra.43:
18186 brasl %r14,qeth_get_ipa_cmd_name@PLT
18252 brasl %r14,qeth_get_ipa_cmd_name@PLT
Are you convinced now?
Thanks,
--
Jean Delvare
SUSE L3 Support
