From: Vlad Buslov <vladbu@mellanox.com>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Vlad Buslov <vladbu@mellanox.com>, Jiri Pirko <jiri@resnulli.us>,
Linux Kernel Network Developers <netdev@vger.kernel.org>,
David Miller <davem@davemloft.net>,
Jamal Hadi Salim <jhs@mojatatu.com>,
Davide Caratti <dcaratti@redhat.com>,
Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
Jakub Kicinski <kuba@kernel.org>
Subject: Re: [PATCH net-next v2 0/4] Implement classifier-action terse dump mode
Date: Wed, 20 May 2020 10:33:00 +0300 [thread overview]
Message-ID: <vbftv0bvz6b.fsf@mellanox.com> (raw)
In-Reply-To: <CAM_iQpU_BvK0Cgra+-_PUrpwkX3FyLDo6r3sz=phD-dTk278pQ@mail.gmail.com>
On Tue 19 May 2020 at 21:39, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, May 19, 2020 at 2:10 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>>
>> On Mon 18 May 2020 at 21:46, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Sun, May 17, 2020 at 11:44 PM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >>
>> >>
>> >> On Sun 17 May 2020 at 22:13, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >> > On Fri, May 15, 2020 at 4:40 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >> >>
>> >> >> Output rate of current upstream kernel TC filter dump implementation if
>> >> >> relatively low (~100k rules/sec depending on configuration). This
>> >> >> constraint impacts performance of software switch implementation that
>> >> >> rely on TC for their datapath implementation and periodically call TC
>> >> >> filter dump to update rules stats. Moreover, TC filter dump output a lot
>> >> >> of static data that don't change during the filter lifecycle (filter
>> >> >> key, specific action details, etc.) which constitutes significant
>> >> >> portion of payload on resulting netlink packets and increases amount of
>> >> >> syscalls necessary to dump all filters on particular Qdisc. In order to
>> >> >> significantly improve filter dump rate this patch sets implement new
>> >> >> mode of TC filter dump operation named "terse dump" mode. In this mode
>> >> >> only parameters necessary to identify the filter (handle, action cookie,
>> >> >> etc.) and data that can change during filter lifecycle (filter flags,
>> >> >> action stats, etc.) are preserved in dump output while everything else
>> >> >> is omitted.
>> >> >>
>> >> >> Userspace API is implemented using new TCA_DUMP_FLAGS tlv with only
>> >> >> available flag value TCA_DUMP_FLAGS_TERSE. Internally, new API requires
>> >> >> individual classifier support (new tcf_proto_ops->terse_dump()
>> >> >> callback). Support for action terse dump is implemented in act API and
>> >> >> don't require changing individual action implementations.
>> >> >
>> >> > Sorry for being late.
>> >> >
>> >> > Why terse dump needs a new ops if it only dumps a subset of the
>> >> > regular dump? That is, why not just pass a boolean flag to regular
>> >> > ->dump() implementation?
>> >> >
>> >> > I guess that might break user-space ABI? At least some netlink
>> >> > attributes are not always dumped anyway, so it does not look like
>> >> > a problem?
>> >> >
>> >> > Thanks.
>> >>
>> >> Hi Cong,
>> >>
>> >> I considered adding a flag to ->dump() callback but decided against it
>> >> for following reasons:
>> >>
>> >> - It complicates fl_dump() code by adding additional conditionals. Not a
>> >> big problem but it seemed better for me to have a standalone callback
>> >> because with combined implementation it is even hard to deduce what
>> >> does terse dump actually output.
>> >
>> > This is not a problem, at least you can add a big if in fl_dump(),
>> > something like:
>> >
>> > if (terse) {
>> > // do terse dump
>> > return 0;
>> > }
>> > // normal dump
>>
>> That is what I was trying to prevent with my implementation: having big
>> "superfunctions" that implement multiple things with branching. Why not
>> just have dedicated callbacks that do exactly one thing?
>
> 1. Saving one unnecessary ops.
> 2. Easier to trace the code: all dumpings are in one implementation.
Okay, I see your point.
>
>>
>> >
>> >>
>> >> - My initial implementation just called regular dump for classifiers
>> >> that don't support terse dump, but in internal review Jiri insisted
>> >> that cls API should fail if it can't satisfy user's request and having
>> >> dedicated callback allows implementation to return an error if
>> >> classifier doesn't define ->terse_dump(). With flag approach it would
>> >> be not trivial to determine if implementation actually uses the flag.
>> >
>> > Hmm? For those not support terse dump, we can just do:
>> >
>> > if (terse)
>> > return -EOPNOTSUPP;
>> > // normal dump goes here
>> >
>> > You just have to pass 'terse' flag to all implementations and let them
>> > to decide whether to support it or not.
>>
>> But why duplicate the same code to all existing cls dump implementations
>> instead of having such check nicely implemented in cls API (via callback
>> existence or a flag)?
>
> I am confused, your fl_terse_dump() also has duplication with fl_dump()...
>
> Thanks.
I meant duplicating the "if terse not supported return -EOPNOTSUPP" in
dump callback of every classifier implementation. With current
implementation cls API handles such case by checking whether classifier
implementation has ->terse_dump() defined and returns error otherwise.
This can also be achieved by having a new classifier flag, in case we
decide to proceed with folding both dump and terse_dump into single
->dump(bool terse) callback.
next prev parent reply other threads:[~2020-05-20 7:33 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-15 11:40 [PATCH net-next v2 0/4] Implement classifier-action terse dump mode Vlad Buslov
2020-05-15 11:40 ` [PATCH net-next v2 1/4] net: sched: introduce terse dump flag Vlad Buslov
2020-05-15 11:51 ` Jiri Pirko
2020-05-15 11:40 ` [PATCH net-next v2 2/4] net: sched: implement terse dump support in act Vlad Buslov
2020-05-15 11:51 ` Jiri Pirko
2020-05-15 11:40 ` [PATCH net-next v2 3/4] net: sched: cls_flower: implement terse dump support Vlad Buslov
2020-05-15 11:40 ` [PATCH net-next v2 4/4] selftests: implement flower classifier terse dump tests Vlad Buslov
2020-05-15 17:04 ` [PATCH net-next v2 0/4] Implement classifier-action terse dump mode Jakub Kicinski
2020-05-18 6:46 ` Vlad Buslov
2020-05-15 17:25 ` David Miller
2020-05-18 6:50 ` Vlad Buslov
2020-05-17 19:13 ` Cong Wang
2020-05-18 6:44 ` Vlad Buslov
2020-05-18 18:46 ` Cong Wang
2020-05-19 9:10 ` Vlad Buslov
2020-05-19 18:39 ` Cong Wang
2020-05-20 7:33 ` Vlad Buslov [this message]
2020-05-18 15:37 ` Edward Cree
2020-05-18 18:50 ` Cong Wang
2020-05-19 9:04 ` Vlad Buslov
2020-05-19 14:30 ` Edward Cree
2020-05-19 15:17 ` Vlad Buslov
2020-05-19 18:58 ` Cong Wang
2020-05-20 7:24 ` Vlad Buslov
2020-05-22 19:33 ` Cong Wang
2020-05-25 11:38 ` Vlad Buslov
2020-05-21 14:36 ` Vlad Buslov
2020-05-21 17:02 ` Jakub Kicinski
2020-05-22 16:16 ` Vlad Buslov
2020-05-23 11:06 ` Jamal Hadi Salim
2020-05-22 19:41 ` Cong Wang
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=vbftv0bvz6b.fsf@mellanox.com \
--to=vladbu@mellanox.com \
--cc=davem@davemloft.net \
--cc=dcaratti@redhat.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=marcelo.leitner@gmail.com \
--cc=netdev@vger.kernel.org \
--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).