On Mon, 10 Sep 2018 15:43:21 +0200, Martin Schwidefsky wrote:
> On Mon, 10 Sep 2018 15:05:05 +0200
> Jean Delvare <jdelvare@xxxxxxx> wrote:
> > 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.
Says +4 here. Might depend on the compiler (gcc 4.8.5 here). One more
function call but one less local variable.
> 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..
I'm not familiar with the driver so I can't answer that question. I am
sure it rarely does something (otherwise I guess that the kernel log
would be filled with qeth messages? I'm not sure how qeth_dbf works to
be honest.) What I can't evaluate is how frequently it is called only
to return immediately. And this is the case where my patch makes a
difference.
My reason for avoiding unnecessary calls to qeth_get_ipa_cmd_name() is
because my previous patch makes it slower than it was before. I wanted
to limit the performance impact of the change. Now if the maintainers
do not care how fast qeth_issue_ipa_msg() is and prefer to keep the code
as it is, that's fine with me, I don't even use the driver ;-)
Thanks for the review and comments,
--
Jean Delvare
SUSE L3 Support
