From: Vlad Buslov <vladbu@mellanox•com>
To: Jakub Kicinski <jakub.kicinski@netronome•com>
Cc: Vlad Buslov <vladbu@mellanox•com>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"jhs@mojatatu•com" <jhs@mojatatu•com>,
"xiyou.wangcong@gmail•com" <xiyou.wangcong@gmail•com>,
"jiri@resnulli•us" <jiri@resnulli•us>,
"davem@davemloft•net" <davem@davemloft•net>
Subject: Re: [RFC PATCH net-next] net: sched: flower: refactor reoffload for concurrent access
Date: Thu, 18 Apr 2019 16:33:22 +0000 [thread overview]
Message-ID: <vbfh8avuvg3.fsf@mellanox.com> (raw)
In-Reply-To: <20190417093432.495dcda6@cakuba.netronome.com>
On Wed 17 Apr 2019 at 19:34, Jakub Kicinski <jakub.kicinski@netronome•com> wrote:
> On Wed, 17 Apr 2019 07:29:36 +0000, Vlad Buslov wrote:
>> On Wed 17 Apr 2019 at 00:49, Jakub Kicinski <jakub.kicinski@netronome•com> wrote:
>> > On Tue, 16 Apr 2019 17:20:47 +0300, Vlad Buslov wrote:
>> >> @@ -1551,6 +1558,10 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
>> >> goto errout_mask;
>> >>
>> >> if (!tc_skip_hw(fnew->flags)) {
>> >> + spin_lock(&tp->lock);
>> >> + list_add(&fnew->hw_list, &head->hw_filters);
>> >> + spin_unlock(&tp->lock);
>> >> +
>> >> err = fl_hw_replace_filter(tp, fnew, rtnl_held, extack);
>> >> if (err)
>> >> goto errout_ht;
>> >
>> > Duplicated deletes should be fine, but I'm not sure same is true for
>> > adds. Won't seeing an add with the same cookie twice confuse drivers?
>> >
>> > There's also the minor issue of offloaded count being off in that
>> > case :)
>>
>> Hmmm, okay. Rejecting duplicate cookies should be a trivial change to
>> drivers though. Do you see any faults with this approach in general?
>
> Trivial or not it adds up, the stack should make driver authors' job as
> easy as possible. The simplest thing to do would be to add a mutex
> around the HW calls. But that obviously doesn't work for you, cause
> you want multiple outstanding requests to the FW for a single tp, right?
>
> How about a RW lock, that would take R on normal add/replace/del paths
> and W on replays? That should scale, no?
I've been thinking some more about possible ways to mitigate the
problem. First of all I tried to implement POC of rwlock in flower and
it isn't straightforward because of lock ordering. Observe that
fl_reoffload() is always called with rtnl lock taken (I didn't do any work to
unlock bind/unbind), but fl_change() can be called without rtnl lock and
needs to obtain it before offloading rules. This means that we have
deadlock here, if fl_change() obtains locks in order rwlock --->
rtnl_lock and fl_reoffload() obtains locks in order rtnl_lock --->
rwlock.
Considering this, I tried to improve my solution to remove possibility
of multiple adds of same filter and it seems to me that it would be
enough to move hw_filters list management in flower offloads functions:
add filter to list while holding rtnl lock in fl_hw_replace_filter() and
remove it from list while holding rtnl lock in fl_hw_destroy_filter().
What do you think?
next prev parent reply other threads:[~2019-04-18 16:33 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-05 17:56 [PATCH net-next] net: sched: flower: insert filter to ht before offloading it to hw Vlad Buslov
2019-04-06 5:59 ` Jiri Pirko
2019-04-08 2:34 ` David Miller
2019-04-08 22:26 ` Jakub Kicinski
2019-04-09 8:23 ` Vlad Buslov
2019-04-09 17:10 ` Jakub Kicinski
2019-04-10 14:53 ` Vlad Buslov
2019-04-10 15:48 ` Jakub Kicinski
2019-04-10 16:02 ` Vlad Buslov
2019-04-10 16:09 ` Jakub Kicinski
2019-04-10 16:26 ` Vlad Buslov
2019-04-10 17:00 ` Jakub Kicinski
2019-04-16 14:20 ` [RFC PATCH net-next] net: sched: flower: refactor reoffload for concurrent access Vlad Buslov
2019-04-16 21:49 ` Jakub Kicinski
2019-04-17 7:29 ` Vlad Buslov
2019-04-17 16:34 ` Jakub Kicinski
2019-04-17 17:01 ` Vlad Buslov
2019-04-18 16:33 ` Vlad Buslov [this message]
2019-04-18 17:46 ` Jakub Kicinski
2019-04-18 17:58 ` Vlad Buslov
2019-04-18 18:02 ` Jakub Kicinski
2019-04-18 18:13 ` Vlad Buslov
2019-04-18 18:15 ` Jakub Kicinski
2019-04-11 11:13 ` [PATCH net-next] net: sched: flower: insert filter to ht before offloading it to hw Ido Schimmel
2019-04-11 11:28 ` Vlad Buslov
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=vbfh8avuvg3.fsf@mellanox.com \
--to=vladbu@mellanox$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=jakub.kicinski@netronome$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=netdev@vger$(echo .)kernel.org \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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