netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jiri Pirko <jiri@resnulli.us>
To: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: davem@davemloft.net, xiyou.wangcong@gmail.com,
	eric.dumazet@gmail.com, netdev@vger.kernel.org,
	Simon Horman <simon.horman@netronome.com>,
	Benjamin LaHaise <bcrl@kvack.org>
Subject: Re: [PATCH net-next v8 2/3] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
Date: Wed, 26 Apr 2017 14:08:51 +0200	[thread overview]
Message-ID: <20170426120851.GE1867@nanopsycho.orion> (raw)
In-Reply-To: <8f1a1b14-ad9b-7840-1fa6-04f2a2e4f55d@mojatatu.com>

Wed, Apr 26, 2017 at 01:48:29PM CEST, jhs@mojatatu.com wrote:
>On 17-04-26 02:19 AM, Jiri Pirko wrote:
>> Tue, Apr 25, 2017 at 10:29:40PM CEST, jhs@mojatatu.com wrote:
>> > On 17-04-25 12:04 PM, Jiri Pirko wrote:
>[..]
>> > That is expected behavior correct?
>> > 
>> > 3 months down the road:
>> > I add two flags - bit 1 and 2.
>> > So now my valid_flags changes to bits 1, 2 and 0.
>> > 
>> > The function above will now return true for bits 0-2 but
>> > will reject if you set bit 3.
>> > 
>> > That is expected behavior, correct?
>> 
>> The same app compiled against new kernel with bits (0, 1, 2) will run with
>> this kernel good. But if you run it with older kernel, the kernel (0)
>> would refuse. Is that ok?
>> 
>
>
>Dave said that is what has to be done.
>To quote from the cover letter:
>
>--------- START QUOTE -------------
>changes since v6:
>-----------------
>
>1) DaveM:
>New rules for netlink messages. From now on we are going to start
>checking for bits that are not used and rejecting anything we dont
>understand. In the future this is going to require major changes
>to user space code (tc etc). This is just a start.
>
>To quote, David:
>"
> Again, bits you aren't using now, make sure userspace doesn't
>   set them.  And if it does, reject.
>"
>
>---------- END QUOTE -----------
>
>I am going to send the patches - if you dont like this then speak up and
>David needs to be convinced.  This is UAPI - once patches are in it is
>cast in stone and I dont mind a discussion to make sure we get it right.
>
>> Jamal, note that I never suggested having more flags in a single attr.
>> Therefore I suggested u8 to carry a single flag.
>> 
>
>Jiri, thats our main difference unless I am misunderstanding you.
>
>I believe you should squeeze as many as you can in one single attribute.
>You believe you should have only one flag per attribute.
>
>Aesthetically a u8 looks good. Performance wise it is bad when you
>have many entries to deal with.
>
>
>> You say that it has performance impact having 3 flag attrs in compare to
>> one bit flag attr. Could you please provide some numbers?
>> 
>
>I have experience with dealing with a massive amount of various dumps
>and (batch) sets and it always boils down to one thing:
>_how much data is exchanged between user and kernel_
>3 flags encoded as bits in a u32 attribute cost 64 bits.
>Encoded separately cost 3x that.
>
>Believe me, it _does make a difference_ in performance.
>
>My least favorite subsystem is bridge. The bridge code has
>tons of flags in those entries that are sent to/from kernel as u8
>attributes. It is painful.
>
>For something more recent, lets look at this commit from Ben on Flower:
>+       TCA_FLOWER_KEY_MPLS_TTL,        /* u8 - 8 bits */
>+       TCA_FLOWER_KEY_MPLS_BOS,        /* u8 - 1 bit */
>+       TCA_FLOWER_KEY_MPLS_TC,         /* u8 - 3 bits */
>+       TCA_FLOWER_KEY_MPLS_LABEL,      /* be32 - 20 bits */
>
>Yes, that looks pretty, but:
>That would have fit in one attribute with a u32. Mask attributes would
>be eliminated with a second 32 bit - all in the same singular
>attribute.
>
>Imagine if i have 1M flower entries. If you add up the mask the cost
>of these things is about 3*2*64 bits more per entry compared to putting
>the mpls info/mask in one attribute.
>At 1M entries that is a few MBs of data being exchanged.

I can do the math :) Yet still, I would like to see the numbers :)
Because I believe that is the only way to end this lenghty converstation
once and forever...


>
>> I expect that you will not be able to show the difference. And if there
>> is no difference in performance, your main argument goes away. And we
>> can do this in a nice, clear, TLV fashion.
>> 
>
>I love TLVs Jiri. But there is a difference between management and
>control. The former cares more about humans the later needs to get shit
>done faster. The extreme version of the later is using json. But you
>try to get the json guy to do setting or dumping 1M entries and you
>can take a long distance trip and come back and they are not done.
>
>I want to use TLVs but plan for optimization/performance as well.
>
>cheers,
>jamal
>
>
>

  reply	other threads:[~2017-04-26 12:08 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-25 11:54 [PATCH net-next v8 0/3] net sched actions: improve dump performance Jamal Hadi Salim
2017-04-25 11:54 ` [PATCH net-next v8 1/3] net sched actions: Use proper root attribute table for actions Jamal Hadi Salim
2017-04-25 18:42   ` Simon Horman
2017-04-25 11:54 ` [PATCH net-next v8 2/3] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch Jamal Hadi Salim
2017-04-25 12:13   ` Jiri Pirko
2017-04-25 13:01     ` Jamal Hadi Salim
2017-04-25 16:04       ` Jiri Pirko
2017-04-25 20:29         ` Jamal Hadi Salim
2017-04-26  6:19           ` Jiri Pirko
2017-04-26 11:07             ` Simon Horman
2017-04-26 13:00               ` Jamal Hadi Salim
2017-04-26 11:48             ` Jamal Hadi Salim
2017-04-26 12:08               ` Jiri Pirko [this message]
2017-04-26 13:14                 ` Jamal Hadi Salim
2017-04-26 13:56                   ` Jiri Pirko
2017-04-26 20:07                     ` Jamal Hadi Salim
2017-04-27  6:30                       ` Jiri Pirko
2017-04-28  1:22                         ` Jamal Hadi Salim
2017-04-28  7:02                           ` Jiri Pirko
2017-04-28 12:30                             ` Jamal Hadi Salim
2017-04-28 13:21                               ` Jiri Pirko
2017-04-28 13:42                                 ` Jamal Hadi Salim
2017-04-28 14:02                                   ` Jiri Pirko
2017-04-30 10:34                                 ` Jamal Hadi Salim
2017-04-26 11:02         ` Simon Horman
2017-04-26 12:46           ` Jamal Hadi Salim
2017-04-26 13:05             ` Jiri Pirko
2017-04-26 14:46               ` David Miller
2017-04-26 14:58                 ` Jiri Pirko
2017-04-28 12:21                 ` Simon Horman
2017-04-28 12:55                   ` Jamal Hadi Salim
2017-04-28 13:21                     ` Jiri Pirko
2017-04-25 11:54 ` [PATCH net-next v8 3/3] net sched actions: add time filter for action dumping Jamal Hadi Salim

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=20170426120851.GE1867@nanopsycho.orion \
    --to=jiri@resnulli.us \
    --cc=bcrl@kvack.org \
    --cc=davem@davemloft.net \
    --cc=eric.dumazet@gmail.com \
    --cc=jhs@mojatatu.com \
    --cc=netdev@vger.kernel.org \
    --cc=simon.horman@netronome.com \
    --cc=xiyou.wangcong@gmail.com \
    /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).