On Mon, 10 Sep 2018 15:05:05 +0200
Jean Delvare <jdelvare@xxxxxxx> wrote:
> 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?
Ah, ok. I missed the extra conditional in the expansion of
QETH_DBF_MESSAGE. So on the plus side we have faster execution
time for qeth_issue_ipa_msg, on the down side we now have two
function calls instead of one, +14 bytes in bloat-o-meter.
That function is rarely used, no? Then I am not sure if we are
making the right trade-off. I leave the decision to our network
maintainers..
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
