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? 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..
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
