[patch net-next RFC 0/3] switchdev: change locking

[patch net-next RFC 0/3] switchdev: change locking

[Jiri Pirko]

This is something which I'm currently struggling with.
Callers of attr_set and obj_add/del often hold not only RTNL, but also
spinlock (bridge). So in that case, the driver implementing the op cannot sleep.

The way rocker is dealing with this now is just to invoke driver operation
and go out, without any checking or reporting of the operation status.

Since it would be nice to at least put a warning in case the operation fails,
it might make sense to do this in delayed work directly in switchdev core
instead of implementing this in separate drivers. And that is what this patchset
is introducing. (The part of removing "nowait" from rocker is not implemented
in this patchset, but it is straightforward).

Does this make sense. Any ideas?

Jiri Pirko (3):
  switchdev: assert rtnl in switchdev_port_obj_del
  switchdev: allow caller to explicitly use deferred attr_set version
  switchdev: introduce deferred variants of obj_add/del helpers

 include/net/switchdev.h   |   6 ++
 net/bridge/br_fdb.c       |   2 +-
 net/bridge/br_stp.c       |   4 +-
 net/switchdev/switchdev.c | 203 ++++++++++++++++++++++++++++++++++------------
 4 files changed, 160 insertions(+), 55 deletions(-)

-- 
1.9.3

[patch net-next RFC 1/3] switchdev: assert rtnl in switchdev_port_obj_del

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

RTNL mutex needs to be held for this function.
Safe usage of netdev_for_each_lower_dev requires that.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/switchdev/switchdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 6e4a4f9..3fb05d5 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -359,6 +359,8 @@ EXPORT_SYMBOL_GPL(switchdev_port_obj_add);
  *	@dev: port device
  *	@id: object ID
  *	@obj: object to delete
+ *
+ *	rtnl_lock must be held.
  */
 int switchdev_port_obj_del(struct net_device *dev,
 			   const struct switchdev_obj *obj)
@@ -368,6 +370,8 @@ int switchdev_port_obj_del(struct net_device *dev,
 	struct list_head *iter;
 	int err = -EOPNOTSUPP;
 
+	ASSERT_RTNL();
+
 	if (ops && ops->switchdev_port_obj_del)
 		return ops->switchdev_port_obj_del(dev, obj);
 
-- 
1.9.3

[patch net-next RFC 2/3] switchdev: allow caller to explicitly use deferred attr_set version

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.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@mellanox.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);
 int switchdev_port_obj_add(struct net_device *dev,
 			   const struct switchdev_obj *obj);
 int switchdev_port_obj_del(struct net_device *dev,
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 3a982c0..91a56b6 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -46,8 +46,8 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
 	int err;
 
 	p->state = state;
-	err = switchdev_port_attr_set(p->dev, &attr);
-	if (err && err != -EOPNOTSUPP)
+	err = switchdev_port_attr_set_deferred(p->dev, &attr);
+	if (err)
 		br_warn(p->br, "error setting offload STP state on port %u(%s)\n",
 				(unsigned int) p->port_no, p->dev->name);
 }
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 3fb05d5..c29f4ee 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -163,49 +163,6 @@ static int __switchdev_port_attr_set(struct net_device *dev,
 	return err;
 }
 
-struct switchdev_attr_set_work {
-	struct work_struct work;
-	struct net_device *dev;
-	struct switchdev_attr attr;
-};
-
-static void switchdev_port_attr_set_work(struct work_struct *work)
-{
-	struct switchdev_attr_set_work *asw =
-		container_of(work, struct switchdev_attr_set_work, work);
-	int err;
-
-	rtnl_lock();
-	err = switchdev_port_attr_set(asw->dev, &asw->attr);
-	if (err && err != -EOPNOTSUPP)
-		netdev_err(asw->dev, "failed (err=%d) to set attribute (id=%d)\n",
-			   err, asw->attr.id);
-	rtnl_unlock();
-
-	dev_put(asw->dev);
-	kfree(work);
-}
-
-static int switchdev_port_attr_set_defer(struct net_device *dev,
-					 struct switchdev_attr *attr)
-{
-	struct switchdev_attr_set_work *asw;
-
-	asw = kmalloc(sizeof(*asw), GFP_ATOMIC);
-	if (!asw)
-		return -ENOMEM;
-
-	INIT_WORK(&asw->work, switchdev_port_attr_set_work);
-
-	dev_hold(dev);
-	asw->dev = dev;
-	memcpy(&asw->attr, attr, sizeof(asw->attr));
-
-	schedule_work(&asw->work);
-
-	return 0;
-}
-
 /**
  *	switchdev_port_attr_set - Set port attribute
  *
@@ -215,21 +172,16 @@ static int switchdev_port_attr_set_defer(struct net_device *dev,
  *	Use a 2-phase prepare-commit transaction model to ensure
  *	system is not left in a partially updated state due to
  *	failure from driver/device.
+ *
+ *	rtnl_lock must be held.
+ *	must not be in atomic section.
  */
 int switchdev_port_attr_set(struct net_device *dev, struct switchdev_attr *attr)
 {
 	struct switchdev_trans trans;
 	int err;
 
-	if (!rtnl_is_locked()) {
-		/* Running prepare-commit transaction across stacked
-		 * devices requires nothing moves, so if rtnl_lock is
-		 * not held, schedule a worker thread to hold rtnl_lock
-		 * while setting attr.
-		 */
-
-		return switchdev_port_attr_set_defer(dev, attr);
-	}
+	ASSERT_RTNL();
 
 	switchdev_trans_init(&trans);
 
@@ -269,6 +221,63 @@ int switchdev_port_attr_set(struct net_device *dev, struct switchdev_attr *attr)
 }
 EXPORT_SYMBOL_GPL(switchdev_port_attr_set);
 
+struct switchdev_attr_set_work {
+	struct work_struct work;
+	struct net_device *dev;
+	struct switchdev_attr attr;
+};
+
+static void switchdev_port_attr_set_work(struct work_struct *work)
+{
+	struct switchdev_attr_set_work *asw =
+		container_of(work, struct switchdev_attr_set_work, work);
+	int err;
+
+	rtnl_lock();
+	err = switchdev_port_attr_set(asw->dev, &asw->attr);
+	if (err && err != -EOPNOTSUPP)
+		netdev_err(asw->dev, "failed (err=%d) to set attribute (id=%d)\n",
+			   err, asw->attr.id);
+	rtnl_unlock();
+
+	dev_put(asw->dev);
+	kfree(asw);
+}
+
+/**
+ *	switchdev_port_attr_set_deferred - Set port attribute - deferred
+ *
+ *	@dev: port device
+ *	@attr: attribute to set
+ *
+ *	Use a 2-phase prepare-commit transaction model to ensure
+ *	system is not left in a partially updated state due to
+ *	failure from driver/device.
+ *
+ *	This version can be safely called from context when RTNL
+ *	mutex is not held and from atomic context.
+ */
+int switchdev_port_attr_set_deferred(struct net_device *dev,
+				     struct switchdev_attr *attr)
+{
+	struct switchdev_attr_set_work *asw;
+
+	asw = kmalloc(sizeof(*asw), GFP_ATOMIC);
+	if (!asw)
+		return -ENOMEM;
+
+	INIT_WORK(&asw->work, switchdev_port_attr_set_work);
+
+	dev_hold(dev);
+	asw->dev = dev;
+	memcpy(&asw->attr, attr, sizeof(asw->attr));
+
+	schedule_work(&asw->work);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(switchdev_port_attr_set_deferred);
+
 static int __switchdev_port_obj_add(struct net_device *dev,
 				    const struct switchdev_obj *obj,
 				    struct switchdev_trans *trans)
-- 
1.9.3

[patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Similar to the attr usecase, the caller knows if he is holding RTNL and is
in atomic section. So let the called to decide the correct call variant.

This allows drivers to sleep inside their ops and wait for hw to get the
operation status. Then the status is propagated into switchdev core.
This avoids silent errors in drivers.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/switchdev.h   |  4 +++
 net/bridge/br_fdb.c       |  2 +-
 net/switchdev/switchdev.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 320be44..5841599 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -172,8 +172,12 @@ int switchdev_port_attr_set_deferred(struct net_device *dev,
 				     struct switchdev_attr *attr);
 int switchdev_port_obj_add(struct net_device *dev,
 			   const struct switchdev_obj *obj);
+int switchdev_port_obj_add_deferred(struct net_device *dev,
+				    const struct switchdev_obj *obj);
 int switchdev_port_obj_del(struct net_device *dev,
 			   const struct switchdev_obj *obj);
+int switchdev_port_obj_del_deferred(struct net_device *dev,
+				    const struct switchdev_obj *obj);
 int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
 			    switchdev_obj_dump_cb_t *cb);
 int register_switchdev_notifier(struct notifier_block *nb);
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 7f7d551..2086767 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -139,7 +139,7 @@ static void fdb_del_external_learn(struct net_bridge_fdb_entry *f)
 		.vid = f->vlan_id,
 	};
 
-	switchdev_port_obj_del(f->dst->dev, &fdb.obj);
+	switchdev_port_obj_del_deferred(f->dst->dev, &fdb.obj);
 }
 
 static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index c29f4ee..49e6e6f 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -362,6 +362,75 @@ int switchdev_port_obj_add(struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(switchdev_port_obj_add);
 
+struct switchdev_obj_work {
+	struct work_struct work;
+	struct net_device *dev;
+	struct switchdev_obj obj;
+	bool add; /* add of del */
+};
+
+static void switchdev_port_obj_work(struct work_struct *work)
+{
+	struct switchdev_obj_work *ow =
+			container_of(work, struct switchdev_obj_work, work);
+	int err;
+
+	rtnl_lock();
+	if (ow->add)
+		err = switchdev_port_obj_add(ow->dev, &ow->obj);
+	else
+		err = switchdev_port_obj_del(ow->dev, &ow->obj);
+	if (err && err != -EOPNOTSUPP)
+		netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n",
+			   err, ow->add ? "add" : "del", ow->obj.id);
+	rtnl_unlock();
+
+	dev_put(ow->dev);
+	kfree(ow);
+}
+
+static int switchdev_port_obj_work_schedule(struct net_device *dev,
+					    const struct switchdev_obj *obj,
+					    bool add)
+{
+	struct switchdev_obj_work *ow;
+
+	ow = kmalloc(sizeof(*ow), GFP_ATOMIC);
+	if (!ow)
+		return -ENOMEM;
+
+	INIT_WORK(&ow->work, switchdev_port_obj_work);
+
+	dev_hold(dev);
+	ow->dev = dev;
+	memcpy(&ow->obj, obj, sizeof(ow->obj));
+	ow->add = add;
+
+	schedule_work(&ow->work);
+	return 0;
+}
+
+/**
+ *	switchdev_port_obj_add_deferred - Add port object - deferred
+ *
+ *	@dev: port device
+ *	@id: object ID
+ *	@obj: object to add
+ *
+ *	Use a 2-phase prepare-commit transaction model to ensure
+ *	system is not left in a partially updated state due to
+ *	failure from driver/device.
+ *
+ *	This version can be safely called from context when RTNL
+ *	mutex is not held and from atomic context.
+ */
+int switchdev_port_obj_add_deferred(struct net_device *dev,
+				    const struct switchdev_obj *obj)
+{
+	return switchdev_port_obj_work_schedule(dev, obj, true);
+}
+EXPORT_SYMBOL_GPL(switchdev_port_obj_add_deferred);
+
 /**
  *	switchdev_port_obj_del - Delete port object
  *
@@ -400,6 +469,23 @@ int switchdev_port_obj_del(struct net_device *dev,
 EXPORT_SYMBOL_GPL(switchdev_port_obj_del);
 
 /**
+ *	switchdev_port_obj_del_deferred - Delete port object - deferred
+ *
+ *	@dev: port device
+ *	@id: object ID
+ *	@obj: object to delete
+ *
+ *	This version can be safely called from context when RTNL
+ *	mutex is not held and from atomic context.
+ */
+int switchdev_port_obj_del_deferred(struct net_device *dev,
+				    const struct switchdev_obj *obj)
+{
+	return switchdev_port_obj_work_schedule(dev, obj, false);
+}
+EXPORT_SYMBOL_GPL(switchdev_port_obj_del_deferred);
+
+/**
  *	switchdev_port_obj_dump - Dump port objects
  *
  *	@dev: port device
-- 
1.9.3

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

Hi Jiri,

[auto build test ERROR on net-next/master -- if it's inappropriate base, please ignore]

config: i386-randconfig-x003-201540 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   net/bridge/br_stp.c: In function 'br_set_state':
>> net/bridge/br_stp.c:49:8: error: implicit declaration of function 'switchdev_port_attr_set_deferred' [-Werror=implicit-function-declaration]
     err = switchdev_port_attr_set_deferred(p->dev, &attr);
           ^
   cc1: some warnings being treated as errors

vim +/switchdev_port_attr_set_deferred +49 net/bridge/br_stp.c

    43			.id = SWITCHDEV_ATTR_ID_PORT_STP_STATE,
    44			.u.stp_state = state,
    45		};
    46		int err;
    47	
    48		p->state = state;
  > 49		err = switchdev_port_attr_set_deferred(p->dev, &attr);
    50		if (err)
    51			br_warn(p->br, "error setting offload STP state on port %u(%s)\n",
    52					(unsigned int) p->port_no, p->dev->name);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26067 bytes --]

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

[Scott Feldman]

On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.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@mellanox.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).

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

[Jiri Pirko]

Thu, Oct 08, 2015 at 06:27:07AM CEST, sfeldma@gmail.com wrote:
>On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.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@mellanox.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.

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

[Scott Feldman]

On Wed, Oct 7, 2015 at 10:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 08, 2015 at 06:27:07AM CEST, sfeldma@gmail.com wrote:
>>On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> From: Jiri Pirko <jiri@mellanox.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@mellanox.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?

Re: [patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Or Gerlitz]

On Wed, Oct 7, 2015 at 9:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Similar to the attr usecase, the caller knows if he is holding RTNL and is
> in atomic section. So let the called to decide the correct call variant.
>
> This allows drivers to sleep inside their ops and wait for hw to get the
> operation status. Then the status is propagated into switchdev core.
> This avoids silent errors in drivers.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 7f7d551..2086767 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -139,7 +139,7 @@ static void fdb_del_external_learn(struct net_bridge_fdb_entry *f)
>                 .vid = f->vlan_id,
>         };
>
> -       switchdev_port_obj_del(f->dst->dev, &fdb.obj);
> +       switchdev_port_obj_del_deferred(f->dst->dev, &fdb.obj);
>  }


>  static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index c29f4ee..49e6e6f 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -362,6 +362,75 @@ int switchdev_port_obj_add(struct net_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(switchdev_port_obj_add);
>
> +struct switchdev_obj_work {
> +       struct work_struct work;
> +       struct net_device *dev;
> +       struct switchdev_obj obj;
> +       bool add; /* add of del */
> +};
> +
> +static void switchdev_port_obj_work(struct work_struct *work)
> +{
> +       struct switchdev_obj_work *ow =
> +                       container_of(work, struct switchdev_obj_work, work);
> +       int err;
> +
> +       rtnl_lock();
> +       if (ow->add)
> +               err = switchdev_port_obj_add(ow->dev, &ow->obj);
> +       else
> +               err = switchdev_port_obj_del(ow->dev, &ow->obj);
> +       if (err && err != -EOPNOTSUPP)
> +               netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n",
> +                          err, ow->add ? "add" : "del", ow->obj.id);

This introduced a regression to the 2-phase commit scheme, since the
prepare commit can fail
and that would go un-noticed toward the upper layer, agree?

Or.

> +       rtnl_unlock();
> +
> +       dev_put(ow->dev);
> +       kfree(ow);
> +}

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

[Jiri Pirko]

Thu, Oct 08, 2015 at 08:03:35AM CEST, sfeldma@gmail.com wrote:
>On Wed, Oct 7, 2015 at 10:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 08, 2015 at 06:27:07AM CEST, sfeldma@gmail.com wrote:
>>>On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> From: Jiri Pirko <jiri@mellanox.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@mellanox.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?

Thanks.

Jiri

Re: [patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Jiri Pirko]

Thu, Oct 08, 2015 at 08:45:58AM CEST, gerlitz.or@gmail.com wrote:
>On Wed, Oct 7, 2015 at 9:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Similar to the attr usecase, the caller knows if he is holding RTNL and is
>> in atomic section. So let the called to decide the correct call variant.
>>
>> This allows drivers to sleep inside their ops and wait for hw to get the
>> operation status. Then the status is propagated into switchdev core.
>> This avoids silent errors in drivers.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index 7f7d551..2086767 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -139,7 +139,7 @@ static void fdb_del_external_learn(struct net_bridge_fdb_entry *f)
>>                 .vid = f->vlan_id,
>>         };
>>
>> -       switchdev_port_obj_del(f->dst->dev, &fdb.obj);
>> +       switchdev_port_obj_del_deferred(f->dst->dev, &fdb.obj);
>>  }
>
>
>>  static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> index c29f4ee..49e6e6f 100644
>> --- a/net/switchdev/switchdev.c
>> +++ b/net/switchdev/switchdev.c
>> @@ -362,6 +362,75 @@ int switchdev_port_obj_add(struct net_device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(switchdev_port_obj_add);
>>
>> +struct switchdev_obj_work {
>> +       struct work_struct work;
>> +       struct net_device *dev;
>> +       struct switchdev_obj obj;
>> +       bool add; /* add of del */
>> +};
>> +
>> +static void switchdev_port_obj_work(struct work_struct *work)
>> +{
>> +       struct switchdev_obj_work *ow =
>> +                       container_of(work, struct switchdev_obj_work, work);
>> +       int err;
>> +
>> +       rtnl_lock();
>> +       if (ow->add)
>> +               err = switchdev_port_obj_add(ow->dev, &ow->obj);
>> +       else
>> +               err = switchdev_port_obj_del(ow->dev, &ow->obj);
>> +       if (err && err != -EOPNOTSUPP)
>> +               netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n",
>> +                          err, ow->add ? "add" : "del", ow->obj.id);
>
>This introduced a regression to the 2-phase commit scheme, since the
>prepare commit can fail
>and that would go un-noticed toward the upper layer, agree?

Well, no. This still does the transaction for all lower devices in one
go. No change in that.

Re: [patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Jiri Pirko]

Thu, Oct 08, 2015 at 10:28:58AM CEST, jiri@resnulli.us wrote:
>Thu, Oct 08, 2015 at 08:45:58AM CEST, gerlitz.or@gmail.com wrote:
>>On Wed, Oct 7, 2015 at 9:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Similar to the attr usecase, the caller knows if he is holding RTNL and is
>>> in atomic section. So let the called to decide the correct call variant.
>>>
>>> This allows drivers to sleep inside their ops and wait for hw to get the
>>> operation status. Then the status is propagated into switchdev core.
>>> This avoids silent errors in drivers.
>>>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>
>>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>>> index 7f7d551..2086767 100644
>>> --- a/net/bridge/br_fdb.c
>>> +++ b/net/bridge/br_fdb.c
>>> @@ -139,7 +139,7 @@ static void fdb_del_external_learn(struct net_bridge_fdb_entry *f)
>>>                 .vid = f->vlan_id,
>>>         };
>>>
>>> -       switchdev_port_obj_del(f->dst->dev, &fdb.obj);
>>> +       switchdev_port_obj_del_deferred(f->dst->dev, &fdb.obj);
>>>  }
>>
>>
>>>  static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
>>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>>> index c29f4ee..49e6e6f 100644
>>> --- a/net/switchdev/switchdev.c
>>> +++ b/net/switchdev/switchdev.c
>>> @@ -362,6 +362,75 @@ int switchdev_port_obj_add(struct net_device *dev,
>>>  }
>>>  EXPORT_SYMBOL_GPL(switchdev_port_obj_add);
>>>
>>> +struct switchdev_obj_work {
>>> +       struct work_struct work;
>>> +       struct net_device *dev;
>>> +       struct switchdev_obj obj;
>>> +       bool add; /* add of del */
>>> +};
>>> +
>>> +static void switchdev_port_obj_work(struct work_struct *work)
>>> +{
>>> +       struct switchdev_obj_work *ow =
>>> +                       container_of(work, struct switchdev_obj_work, work);
>>> +       int err;
>>> +
>>> +       rtnl_lock();
>>> +       if (ow->add)
>>> +               err = switchdev_port_obj_add(ow->dev, &ow->obj);
>>> +       else
>>> +               err = switchdev_port_obj_del(ow->dev, &ow->obj);
>>> +       if (err && err != -EOPNOTSUPP)
>>> +               netdev_err(ow->dev, "failed (err=%d) to %s object (id=%d)\n",
>>> +                          err, ow->add ? "add" : "del", ow->obj.id);
>>
>>This introduced a regression to the 2-phase commit scheme, since the
>>prepare commit can fail
>>and that would go un-noticed toward the upper layer, agree?
>
>Well, no. This still does the transaction for all lower devices in one
>go. No change in that.
>
Now I get it, yes you are right. But currently there is no code in
kernel which would control retval of deferred attr_set or obj_add/del

Re: [patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Or Gerlitz]

On Thu, Oct 8, 2015 at 4:09 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 08, 2015 at 10:28:58AM CEST, jiri@resnulli.us wrote:
>>Thu, Oct 08, 2015 at 08:45:58AM CEST, gerlitz.or@gmail.com wrote:
>>>On Wed, Oct 7, 2015 at 9:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:

>>>This introduced a regression to the 2-phase commit scheme, since the
>>>prepare commit can fail
>>>and that would go un-noticed toward the upper layer, agree?

>>Well, no. This still does the transaction for all lower devices in one
>>go. No change in that.

> Now I get it, yes you are right. But currently there is no code in
> kernel which would control retval of deferred attr_set or obj_add/del

I am not sure to understand your reply. You are saying that when the deferred
procedures complete (e.g fail in the prepare phase) they can't actually let
the upper layer to realize that this change isn't possible? this is
exactly the bug.


Or.


Or.

Re: [patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Jiri Pirko]

Thu, Oct 08, 2015 at 03:21:44PM CEST, gerlitz.or@gmail.com wrote:
>On Thu, Oct 8, 2015 at 4:09 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 08, 2015 at 10:28:58AM CEST, jiri@resnulli.us wrote:
>>>Thu, Oct 08, 2015 at 08:45:58AM CEST, gerlitz.or@gmail.com wrote:
>>>>On Wed, Oct 7, 2015 at 9:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>
>>>>This introduced a regression to the 2-phase commit scheme, since the
>>>>prepare commit can fail
>>>>and that would go un-noticed toward the upper layer, agree?
>
>>>Well, no. This still does the transaction for all lower devices in one
>>>go. No change in that.
>
>> Now I get it, yes you are right. But currently there is no code in
>> kernel which would control retval of deferred attr_set or obj_add/del
>
>I am not sure to understand your reply. You are saying that when the deferred
>procedures complete (e.g fail in the prepare phase) they can't actually let
>the upper layer to realize that this change isn't possible? this is
>exactly the bug.

Correct. But check the code. Callers of current deferred variants do
not care about the retval. Therefore this is not a regression.

It makes sense in my opinion. If you are a called and you explicitly say to
defer the operation, you cannot expect retval.

Re: [patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Or Gerlitz]

On Thu, Oct 8, 2015 at 4:25 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 08, 2015 at 03:21:44PM CEST, gerlitz.or@gmail.com wrote:
>>On Thu, Oct 8, 2015 at 4:09 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 08, 2015 at 10:28:58AM CEST, jiri@resnulli.us wrote:
>>>>Thu, Oct 08, 2015 at 08:45:58AM CEST, gerlitz.or@gmail.com wrote:
>>>>>On Wed, Oct 7, 2015 at 9:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>>>>>This introduced a regression to the 2-phase commit scheme, since the
>>>>>prepare commit can fail
>>>>>and that would go un-noticed toward the upper layer, agree?
>>
>>>>Well, no. This still does the transaction for all lower devices in one
>>>>go. No change in that.
>>
>>> Now I get it, yes you are right. But currently there is no code in
>>> kernel which would control retval of deferred attr_set or obj_add/del
>>
>>I am not sure to understand your reply. You are saying that when the deferred
>>procedures complete (e.g fail in the prepare phase) they can't actually let
>>the upper layer to realize that this change isn't possible? this is
>>exactly the bug.
>
> Correct. But check the code. Callers of current deferred variants do
> not care about the retval. Therefore this is not a regression.

No sure to follow on (current) callers of current deferred variants,
are there already
deferred  variants for switchdev ops? aren't they introduced in this series?

> It makes sense in my opinion. If you are a called and you explicitly say to
> defer the operation, you cannot expect retval.

yes, this might make sure for the caller, if they want to know the
retval, shouldn't use
the deferred variant.

Or.

Re: [patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Jiri Pirko]

Thu, Oct 08, 2015 at 04:41:43PM CEST, gerlitz.or@gmail.com wrote:
>On Thu, Oct 8, 2015 at 4:25 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 08, 2015 at 03:21:44PM CEST, gerlitz.or@gmail.com wrote:
>>>On Thu, Oct 8, 2015 at 4:09 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Thu, Oct 08, 2015 at 10:28:58AM CEST, jiri@resnulli.us wrote:
>>>>>Thu, Oct 08, 2015 at 08:45:58AM CEST, gerlitz.or@gmail.com wrote:
>>>>>>On Wed, Oct 7, 2015 at 9:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>
>>>>>>This introduced a regression to the 2-phase commit scheme, since the
>>>>>>prepare commit can fail
>>>>>>and that would go un-noticed toward the upper layer, agree?
>>>
>>>>>Well, no. This still does the transaction for all lower devices in one
>>>>>go. No change in that.
>>>
>>>> Now I get it, yes you are right. But currently there is no code in
>>>> kernel which would control retval of deferred attr_set or obj_add/del
>>>
>>>I am not sure to understand your reply. You are saying that when the deferred
>>>procedures complete (e.g fail in the prepare phase) they can't actually let
>>>the upper layer to realize that this change isn't possible? this is
>>>exactly the bug.
>>
>> Correct. But check the code. Callers of current deferred variants do
>> not care about the retval. Therefore this is not a regression.
>
>No sure to follow on (current) callers of current deferred variants,
>are there already
>deferred  variants for switchdev ops? aren't they introduced in this series?

Yes they are. Those are those places where deferred variants need to be
called.

>
>> It makes sense in my opinion. If you are a called and you explicitly say to
>> defer the operation, you cannot expect retval.
>
>yes, this might make sure for the caller, if they want to know the
>retval, shouldn't use
>the deferred variant.
>
>Or.

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

[Scott Feldman]

On Thu, Oct 8, 2015 at 1:26 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 08, 2015 at 08:03:35AM CEST, sfeldma@gmail.com wrote:
>>On Wed, Oct 7, 2015 at 10:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 08, 2015 at 06:27:07AM CEST, sfeldma@gmail.com wrote:
>>>>On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>> From: Jiri Pirko <jiri@mellanox.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@mellanox.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.

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

[Jiri Pirko]

Fri, Oct 09, 2015 at 06:39:41AM CEST, sfeldma@gmail.com wrote:
>On Thu, Oct 8, 2015 at 1:26 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 08, 2015 at 08:03:35AM CEST, sfeldma@gmail.com wrote:
>>>On Wed, Oct 7, 2015 at 10:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Thu, Oct 08, 2015 at 06:27:07AM CEST, sfeldma@gmail.com wrote:
>>>>>On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>> From: Jiri Pirko <jiri@mellanox.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@mellanox.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?

>
>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.


>
>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.

Re: [patch net-next RFC 3/3] switchdev: introduce deferred variants of obj_add/del helpers

[Scott Feldman]

On Thu, Oct 8, 2015 at 6:25 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 08, 2015 at 03:21:44PM CEST, gerlitz.or@gmail.com wrote:
>>On Thu, Oct 8, 2015 at 4:09 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 08, 2015 at 10:28:58AM CEST, jiri@resnulli.us wrote:
>>>>Thu, Oct 08, 2015 at 08:45:58AM CEST, gerlitz.or@gmail.com wrote:
>>>>>On Wed, Oct 7, 2015 at 9:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>>>>>This introduced a regression to the 2-phase commit scheme, since the
>>>>>prepare commit can fail
>>>>>and that would go un-noticed toward the upper layer, agree?
>>
>>>>Well, no. This still does the transaction for all lower devices in one
>>>>go. No change in that.
>>
>>> Now I get it, yes you are right. But currently there is no code in
>>> kernel which would control retval of deferred attr_set or obj_add/del
>>
>>I am not sure to understand your reply. You are saying that when the deferred
>>procedures complete (e.g fail in the prepare phase) they can't actually let
>>the upper layer to realize that this change isn't possible? this is
>>exactly the bug.
>
> Correct. But check the code. Callers of current deferred variants do
> not care about the retval. Therefore this is not a regression.
>
> It makes sense in my opinion. If you are a called and you explicitly say to
> defer the operation, you cannot expect retval.

Makes sense to me also, FWIW.

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

[Scott Feldman]

On Thu, Oct 8, 2015 at 11:46 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Oct 09, 2015 at 06:39:41AM CEST, sfeldma@gmail.com wrote:
>>On Thu, Oct 8, 2015 at 1:26 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 08, 2015 at 08:03:35AM CEST, sfeldma@gmail.com wrote:
>>>>On Wed, Oct 7, 2015 at 10:39 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>> Thu, Oct 08, 2015 at 06:27:07AM CEST, sfeldma@gmail.com wrote:
>>>>>>On Wed, Oct 7, 2015 at 11:30 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>> From: Jiri Pirko <jiri@mellanox.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@mellanox.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?

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

[Jiri Pirko]

Sat, Oct 10, 2015 at 04:43:43AM CEST, sfeldma@gmail.com wrote:

<snip>

>
>>
>>>
>>>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?

Just look at the patch :) fdb_del_external_learn()