netdev - Re: [patch net-next RFC 2/3] switchdev: allow caller to explicitly use deferred attr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE4R7bAXqd6HeO-FxONXJgJTGVvgv5JawxGtZJvidZ4Uc4ePJw@mail.gmail.com>
Date:	Thu, 8 Oct 2015 21:39:41 -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 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?)

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.

patch 3/3 I haven't looked at yet...I'm stuck on 2/3.
--
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