From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: "Richardson, Bruce" <bruce.richardson@intel.com>,
Stephen Hemminger <stephen@networkplumber.org>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"marcel@redhat.com" <marcel@redhat.com>,
Changchun Ouyang <changchun.ouyang@intel.com>,
"Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index
Date: Wed, 21 Oct 2015 16:29:42 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB97725836AB38CB@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <5627B57D.7040603@intel.com>
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, October 21, 2015 4:56 PM
> To: Stephen Hemminger; Ananyev, Konstantin
> Cc: Michael S. Tsirkin; dev@dpdk.org; marcel@redhat.com; Changchun Ouyang
> Subject: Re: [dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index
>
>
>
> On 21/10/2015 16:47, Stephen Hemminger wrote:
> > On Wed, 21 Oct 2015 09:38:37 +0000
> > "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> >
> >>>> minor nits:
> >>>> * this doesn't need to be marked as always inline,
> >>>> that is as they say in English "shooting a fly with a bazooka"
> >>> Stephen:
> >>> always_inline "forces" the compiler to inline this function, like a macro.
> >>> When should it be used or is it not preferred at all?
> >> I also don't understand what's wrong with using 'always_inline' here.
> >> As I understand the author wants compiler to *always inline* that function.
> >> So seems perfectly ok to use it here.
> >> As I remember just 'inline' is sort of recommendation that compiler is free to ignore.
> >> Konstantin
> > I follow Linux/Linus advice and resist the urge to add strong inlining.
> > The compiler does a good job of deciding to inline, and many times
> > the reason it chooses for not inlining are quite good like:
> > - the code is on an unlikely branch
> > - register pressure means inlining would mean the code would be worse
Yep, that's all true, but as I remember 'inline' keyword itself doesn't force compiler to
always inline that function. It is more like a recommendation to the compiler.
Looking at any dpdk binary, there are plenty of places where function is declared as 'inline',
but compiler decided not to and followed standard function call convention for it.
Again, from C spec:
"6. A function declared with an inline function specifier is an inline function. Making a
function an inline function suggests that calls to the function be as fast as possible.138)
7. The extent to which such suggestions are effective is implementation-defined.139)
...
139) For example, an implementation might never perform inline substitution, or might only perform inline
substitutions to calls in the scope of an inline declaration."
> >
> > Therefore my rules are:
> > * only use inline for small functions. Let compiler decide on larger static funcs
As I remember function we are talking about is really small.
> > * write code where most functions are static (localized scope) where compiler
> > can decide
> > * reserve always inline for things that access hardware and would break if not inlined.
Sorry, but the latest rule looks too restrictive to me.
Don't see any reason why we all have to follow it.
BTW, as I can see there are plenty of always_inline functions inside linux kernel
(memory allocator, scheduler, etc).
> >
> On the other hand, there are cases where we know the compiler will
> likely inline, but we also know that not inlining could have a high
> performance penalty, and in that case marking as "always inline" would
> be appropriate - even though it is likely unnecessary for most
> compilers.
Yep, totally agree here.
If memory serves me right - in the past we observed few noticeable performance drops
because of that when switching from one compiler version to another.
Konstantin
In such a case, I would expect the verification check to be:
> explicitly mark the function as *not* to be inlined, and see what the
> perf drop is. If it's a noticable drop, marking as always-inline is an
> ok precaution against future compiler changes.
>
> Also, we need to remember that compilers cannot know whether a function
> is data path or not, and also whether a function will be called
> per-packet or per-burst. That's only something the programmer will know,
> and functions called per-packet on the datapath generally need to be
> inlined for performance.
>
> /Bruce
>
> /Bruce
next prev parent reply other threads:[~2015-10-21 16:30 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-21 3:48 [dpdk-dev] [PATCH v7 0/8] vhost-user multiple queues enabling Yuanhan Liu
2015-10-21 3:48 ` [dpdk-dev] [PATCH v7 1/8] vhost-user: add protocol features support Yuanhan Liu
2015-10-22 9:52 ` Xie, Huawei
2015-10-21 3:48 ` [dpdk-dev] [PATCH v7 2/8] vhost-user: add VHOST_USER_GET_QUEUE_NUM message Yuanhan Liu
2015-10-22 9:38 ` Xie, Huawei
2015-10-21 3:48 ` [dpdk-dev] [PATCH v7 3/8] vhost: vring queue setup for multiple queue support Yuanhan Liu
2015-10-21 4:45 ` Stephen Hemminger
2015-10-21 6:52 ` Yuanhan Liu
2015-10-22 9:49 ` Xie, Huawei
2015-10-22 11:30 ` Yuanhan Liu
2015-10-21 3:48 ` [dpdk-dev] [PATCH v7 4/8] vhost: rxtx: use queue id instead of constant ring index Yuanhan Liu
2015-10-21 4:43 ` Stephen Hemminger
2015-10-21 6:54 ` Yuanhan Liu
2015-10-21 7:16 ` Xie, Huawei
2015-10-21 9:38 ` Ananyev, Konstantin
2015-10-21 15:47 ` Stephen Hemminger
2015-10-21 15:52 ` Thomas Monjalon
2015-10-21 15:57 ` Bruce Richardson
2015-10-21 15:55 ` Bruce Richardson
2015-10-21 16:29 ` Ananyev, Konstantin [this message]
2015-10-21 10:31 ` Michael S. Tsirkin
2015-10-21 12:48 ` Yuanhan Liu
2015-10-21 14:26 ` Michael S. Tsirkin
2015-10-21 14:59 ` Yuanhan Liu
2015-10-22 9:49 ` Yuanhan Liu
2015-10-22 11:32 ` Michael S. Tsirkin
2015-10-22 14:07 ` Yuanhan Liu
2015-10-22 14:19 ` Michael S. Tsirkin
2015-10-23 8:02 ` Yuanhan Liu
2015-10-24 2:34 ` Flavio Leitner
2015-10-24 17:47 ` Michael S. Tsirkin
2015-10-28 20:30 ` Flavio Leitner
2015-10-28 21:12 ` Michael S. Tsirkin
2015-11-16 22:20 ` Flavio Leitner
2015-11-17 8:23 ` Michael S. Tsirkin
2015-11-17 9:24 ` Jason Wang
2015-11-17 22:49 ` Flavio Leitner
2015-10-22 7:26 ` Xie, Huawei
2015-10-21 3:48 ` [dpdk-dev] [PATCH v7 5/8] virtio: fix deadloop due to reading virtio_net_config incorrectly Yuanhan Liu
2015-10-21 3:48 ` [dpdk-dev] [PATCH v7 6/8] vhost-user: enable vhost-user multiple queue Yuanhan Liu
2015-10-21 3:48 ` [dpdk-dev] [PATCH v7 7/8] vhost: add VHOST_USER_SET_VRING_ENABLE message Yuanhan Liu
2015-10-21 3:48 ` [dpdk-dev] [PATCH v7 8/8] doc: update release note for vhost-user mq support Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 0/8] vhost-user multiple queues enabling Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 1/8] vhost-user: add protocol features support Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 2/8] vhost-user: add VHOST_USER_GET_QUEUE_NUM message Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 3/8] vhost: vring queue setup for multiple queue support Yuanhan Liu
2015-10-26 5:24 ` Tetsuya Mukawa
2015-10-26 5:42 ` Yuanhan Liu
2015-10-27 6:20 ` Tetsuya Mukawa
2015-10-27 9:17 ` Michael S. Tsirkin
2015-10-27 9:30 ` Yuanhan Liu
2015-10-27 9:42 ` Michael S. Tsirkin
2015-10-27 9:51 ` Thomas Monjalon
2015-10-27 9:55 ` Michael S. Tsirkin
2015-10-27 10:41 ` Xie, Huawei
2015-10-27 9:53 ` Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 4/8] vhost: rxtx: use queue id instead of constant ring index Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 5/8] virtio: fix deadloop due to reading virtio_net_config incorrectly Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 6/8] vhost: add VHOST_USER_SET_VRING_ENABLE message Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 7/8] vhost-user: enable vhost-user multiple queue Yuanhan Liu
2015-10-22 12:35 ` [dpdk-dev] [PATCH v8 8/8] doc: update release note for vhost-user mq support Yuanhan Liu
2015-10-26 20:22 ` Thomas Monjalon
2015-10-27 9:38 ` Yuanhan Liu
2015-10-26 1:36 ` [dpdk-dev] [PATCH v8 0/8] vhost-user multiple queues enabling Xie, Huawei
2015-10-26 3:09 ` Yuanhan Liu
2015-10-26 20:26 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2601191342CEEE43887BDE71AB97725836AB38CB@irsmsx105.ger.corp.intel.com \
--to=konstantin.ananyev@intel.com \
--cc=bruce.richardson@intel.com \
--cc=changchun.ouyang@intel.com \
--cc=dev@dpdk.org \
--cc=marcel@redhat.com \
--cc=mst@redhat.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).