Date:	Fri, 9 Oct 2015 19:43:43 -0700
From:	Scott Feldman <sfeldma@...il.com>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	Netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Ido Schimmel <idosch@...lanox.com>, eladr@...lanox.com,
	Florian Fainelli <f.fainelli@...il.com>,
	Guenter Roeck <linux@...ck-us.net>,
	Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
	"andrew@...n.ch" <andrew@...n.ch>,
	john fastabend <john.fastabend@...il.com>,
	David Laight <David.Laight@...lab.com>
Subject: Re: [patch net-next RFC 2/3] switchdev: allow caller to explicitly
 use deferred attr_set version

On Thu, Oct 8, 2015 at 11:46 PM, Jiri Pirko <jiri@...nulli.us> wrote:
> Fri, Oct 09, 2015 at 06:39:41AM CEST, sfeldma@...il.com wrote:
>>On Thu, Oct 8, 2015 at 1:26 AM, Jiri Pirko <jiri@...nulli.us> wrote:
>>> Thu, Oct 08, 2015 at 08:03:35AM CEST, sfeldma@...il.com wrote:
>>>>On Wed, Oct 7, 2015 at 10:39 PM, Jiri Pirko <jiri@...nulli.us> wrote:
>>>>> Thu, Oct 08, 2015 at 06:27:07AM CEST, sfeldma@...il.com wrote:
>>>>>>On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko <jiri@...nulli.us> wrote:
>>>>>>> From: Jiri Pirko <jiri@...lanox.com>
>>>>>>>
>>>>>>> Caller should know if he can call attr_set directly (when holding RTNL)
>>>>>>> or if he has to use deferred version of this function.
>>>>>>>
>>>>>>> This also allows drivers to sleep inside attr_set and report operation
>>>>>>> status back to switchdev core. Switchdev core then warns if status is
>>>>>>> not ok, instead of silent errors happening in drivers.
>>>>>>>
>>>>>>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>>>>>>> ---
>>>>>>>  include/net/switchdev.h   |   2 +
>>>>>>>  net/bridge/br_stp.c       |   4 +-
>>>>>>>  net/switchdev/switchdev.c | 113 +++++++++++++++++++++++++---------------------
>>>>>>>  3 files changed, 65 insertions(+), 54 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>>>>>>> index 89266a3..320be44 100644
>>>>>>> --- a/include/net/switchdev.h
>>>>>>> +++ b/include/net/switchdev.h
>>>>>>> @@ -168,6 +168,8 @@ int switchdev_port_attr_get(struct net_device *dev,
>>>>>>>                             struct switchdev_attr *attr);
>>>>>>>  int switchdev_port_attr_set(struct net_device *dev,
>>>>>>>                             struct switchdev_attr *attr);
>>>>>>> +int switchdev_port_attr_set_deferred(struct net_device *dev,
>>>>>>> +                                    struct switchdev_attr *attr);
>>>>>>
>>>>>>Rather than adding another op, use attr->flags and define:
>>>>>>
>>>>>>#define SWITCHDEV_F_DEFERRED          BIT(x)
>>>>>>
>>>>>>So we get:
>>>>>>
>>>>>>void br_set_state(struct net_bridge_port *p, unsigned int state)
>>>>>>{
>>>>>>        struct switchdev_attr attr = {
>>>>>>                .id = SWITCHDEV_ATTR_ID_PORT_STP_STATE,
>>>>>>+              .flags = SWITCHDEV_F_DEFERRED,
>>>>>>                .u.stp_state = state,
>>>>>>        };
>>>>>>        int err;
>>>>>>
>>>>>>        p->state = state;
>>>>>>        err = switchdev_port_attr_set(p->dev, &attr);
>>>>>>        if (err && err != -EOPNOTSUPP)
>>>>>>                br_warn(p->br, "error setting offload STP state on
>>>>>>port %u(%s)\n",
>>>>>>                                (unsigned int) p->port_no,
>>>>>>p->dev->name);
>>>>>>}
>>>>>>
>>>>>>(And add obj->flags to do the same).
>>>>>
>>>>> That's what I wanted to avoid. Also because the obj is const and for
>>>>> call from work, this flag would have to be removed.
>>>>
>>>>What did you want to avoid?
>>>
>>> Having this as a flag. I don't like it too much.
>>> But that is cosmetics. Other than that, does the patchset make sense?
>>> Do you see some possible issues?
>>
>>patch 1/3 makes sense, I tested it out and no issues.  (Looks like
>>there are other places to assert rtnl_lock, are you going to add
>>those?)
>
> Sure, can you pinpoint the places?

Isn't every place we use netdev_for_each_lower_dev, like you mentioned
in 1/3 patch?

>>patch 2/3: Rather than trying to guess the call context in the core,
>>make the caller call the right variant for its context.  That part is
>>good.  On the flag vs. no flags, the reasons why I want this as a flag
>>are:
>>
>>a) I want to keep the switchdev ops set to the core set: get/set attr
>>and add/del/dump objs.  I've pushed back on changing this before.  I
>>don't want ops explosion (like netdev_ops), and I'd like to avoid the
>>1000-line patch when the arg list in an op changes, and we need to
>>update N drivers.  The flags lets the caller modify the algo behavior,
>>while keeping the core call (and args) fixed.
>>
>>b) the caller can combine flags, where it makes sense.  For example,
>>maybe I'm in a locked context and I don't want to recurse the device
>>tree, so I would make the call with NO_RECURSE | DEFERRED.  If we
>>didn't use flags, then we need to supply ops for each variant on the
>>call, and then things explode.
>
> Fair enough. I'll process this in.

Actually, I realized later that my reply here was only half true.
Part b) to combine flags for various calling situation is good.   Part
a) is bogus because I confused adding a new op or adding a new wrapper
to call existing op.  You did the latter; but I was complaining about
the former.  Sorry about that.  Regardless, port b) I think justifies
using flags.

>
>>
>>patch 3/3 I haven't looked at yet...I'm stuck on 2/3.
>
> It is very similar to 2/3, only for obj_add/del.

Do we have examples of a deferred obj add or del?  Maybe we should
hold off adding that support until someone finds a use-case.  I'm kind
of hoping there isn't a use-case, but who knows?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists