[PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Florian Fainelli]

Hi all,

This patch series is to resolve a sleeping function called in atomic context
debug splat that we observe with DSA.

Let me know what you think, I was also wondering if we should just always
make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
could cause invalid contexts to be used for rocker, mlxsw, i40e etc.

Thanks!

Florian Fainelli (4):
  net: switchdev: Prepare for deferred functions modifying objects
  net: switchdev: Add object dump deferred operation
  net: switchdev: Add switchdev_port_bridge_getlink_deferred
  net: dsa: Utilize switchdev_port_bridge_getlink_deferred()

 include/net/switchdev.h   |   3 +
 net/dsa/slave.c           |   2 +-
 net/switchdev/switchdev.c | 171 +++++++++++++++++++++++++++++++++++++---------
 3 files changed, 141 insertions(+), 35 deletions(-)

-- 
2.9.3

[PATCH net-next 1/4] net: switchdev: Prepare for deferred functions modifying objects

[Florian Fainelli]

In preparation for adding support for deferred dump operations, allow
specifying a deferred function whose signature allows read/write
objects.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/switchdev/switchdev.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 017801f9dbaa..3d70ad02c617 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -100,11 +100,14 @@ static DEFINE_SPINLOCK(deferred_lock);
 
 typedef void switchdev_deferred_func_t(struct net_device *dev,
 				       const void *data);
+typedef void switchdev_deferred_func_rw_t(struct net_device *dev,
+					  void *data);
 
 struct switchdev_deferred_item {
 	struct list_head list;
 	struct net_device *dev;
 	switchdev_deferred_func_t *func;
+	switchdev_deferred_func_rw_t *func_rw;
 	unsigned long data[0];
 };
 
@@ -138,7 +141,10 @@ void switchdev_deferred_process(void)
 	ASSERT_RTNL();
 
 	while ((dfitem = switchdev_deferred_dequeue())) {
-		dfitem->func(dfitem->dev, dfitem->data);
+		if (dfitem->func)
+			dfitem->func(dfitem->dev, dfitem->data);
+		else if (dfitem->func_rw)
+			dfitem->func_rw(dfitem->dev, dfitem->data);
 		dev_put(dfitem->dev);
 		kfree(dfitem);
 	}
@@ -156,7 +162,8 @@ static DECLARE_WORK(deferred_process_work, switchdev_deferred_process_work);
 
 static int switchdev_deferred_enqueue(struct net_device *dev,
 				      const void *data, size_t data_len,
-				      switchdev_deferred_func_t *func)
+				      switchdev_deferred_func_t *func,
+				      switchdev_deferred_func_rw_t *func_rw)
 {
 	struct switchdev_deferred_item *dfitem;
 
@@ -165,6 +172,7 @@ static int switchdev_deferred_enqueue(struct net_device *dev,
 		return -ENOMEM;
 	dfitem->dev = dev;
 	dfitem->func = func;
+	dfitem->func_rw = func_rw;
 	memcpy(dfitem->data, data, data_len);
 	dev_hold(dev);
 	spin_lock_bh(&deferred_lock);
@@ -312,7 +320,8 @@ static int switchdev_port_attr_set_defer(struct net_device *dev,
 					 const struct switchdev_attr *attr)
 {
 	return switchdev_deferred_enqueue(dev, attr, sizeof(*attr),
-					  switchdev_port_attr_set_deferred);
+					  switchdev_port_attr_set_deferred,
+					  NULL);
 }
 
 /**
@@ -441,7 +450,8 @@ static int switchdev_port_obj_add_defer(struct net_device *dev,
 					const struct switchdev_obj *obj)
 {
 	return switchdev_deferred_enqueue(dev, obj, switchdev_obj_size(obj),
-					  switchdev_port_obj_add_deferred);
+					  switchdev_port_obj_add_deferred,
+					  NULL);
 }
 
 /**
@@ -511,7 +521,8 @@ static int switchdev_port_obj_del_defer(struct net_device *dev,
 					const struct switchdev_obj *obj)
 {
 	return switchdev_deferred_enqueue(dev, obj, switchdev_obj_size(obj),
-					  switchdev_port_obj_del_deferred);
+					  switchdev_port_obj_del_deferred,
+					  NULL);
 }
 
 /**
-- 
2.9.3

[PATCH net-next 2/4] net: switchdev: Add object dump deferred operation

[Florian Fainelli]

Add plumbing required to perform dump operations in a deferred context,
this mostly wraps the existing switchdev_obj_port_dump() function into a
switchdev_obj_port_dump_now() and adds two wrappers (normal and
deferred) around.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/switchdev/switchdev.c | 71 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 14 deletions(-)

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 3d70ad02c617..4fa9972d72d2 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -545,26 +545,15 @@ int switchdev_port_obj_del(struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(switchdev_port_obj_del);
 
-/**
- *	switchdev_port_obj_dump - Dump port objects
- *
- *	@dev: port device
- *	@id: object ID
- *	@obj: object to dump
- *	@cb: function to call with a filled object
- *
- *	rtnl_lock must be held.
- */
-int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
-			    switchdev_obj_dump_cb_t *cb)
+static int switchdev_port_obj_dump_now(struct net_device *dev,
+				       struct switchdev_obj *obj,
+				       switchdev_obj_dump_cb_t *cb)
 {
 	const struct switchdev_ops *ops = dev->switchdev_ops;
 	struct net_device *lower_dev;
 	struct list_head *iter;
 	int err = -EOPNOTSUPP;
 
-	ASSERT_RTNL();
-
 	if (ops && ops->switchdev_port_obj_dump)
 		return ops->switchdev_port_obj_dump(dev, obj, cb);
 
@@ -580,6 +569,60 @@ int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
 
 	return err;
 }
+
+struct switchdev_obj_dump_deferred_item {
+	struct switchdev_obj obj;
+	switchdev_obj_dump_cb_t *cb;
+};
+
+static void switchdev_port_obj_dump_deferred(struct net_device *dev,
+					     void *data)
+{
+	struct switchdev_obj_dump_deferred_item *i = data;
+	struct switchdev_obj *obj = &i->obj;
+	int err;
+
+	err = switchdev_port_obj_dump_now(dev, obj, i->cb);
+	if (err && err != -EOPNOTSUPP)
+		netdev_err(dev, "failed (err=%d) to dump object (id=%d)\n",
+			   err, obj->id);
+	if (obj->complete)
+		obj->complete(dev, err, obj->complete_priv);
+}
+
+static int switchdev_port_obj_dump_defer(struct net_device *dev,
+					 struct switchdev_obj *obj,
+					 switchdev_obj_dump_cb_t *cb)
+{
+	struct switchdev_obj_dump_deferred_item item;
+
+	memcpy(&item.obj, obj, switchdev_obj_size(obj));
+	item.cb = cb;
+
+	return switchdev_deferred_enqueue(dev, &item, sizeof(item),
+					  NULL,
+					  switchdev_port_obj_dump_deferred);
+}
+
+/**
+ *	switchdev_port_obj_dump - Dump port objects
+ *
+ *	@dev: port device
+ *	@id: object ID
+ *	@obj: object to dump
+ *	@cb: function to call with a filled object
+ *
+ *	rtnl_lock must be held and must not be in atomic section,
+ *	in case SWITCHDEV_F_DEFER flag is not set.
+ */
+int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
+			    switchdev_obj_dump_cb_t *cb)
+{
+	if (obj->flags & SWITCHDEV_F_DEFER)
+		return switchdev_port_obj_dump_defer(dev, obj, cb);
+	ASSERT_RTNL();
+	return switchdev_port_obj_dump_now(dev, obj, cb);
+}
 EXPORT_SYMBOL_GPL(switchdev_port_obj_dump);
 
 static RAW_NOTIFIER_HEAD(switchdev_notif_chain);
-- 
2.9.3

[PATCH net-next 3/4] net: switchdev: Add switchdev_port_bridge_getlink_deferred

[Florian Fainelli]

Add switchdev_port_bridge_getlink_deferred() which does a deferred
object dump operation, this is required for e.g: DSA switches which
typically have sleeping I/O operations which is incompatible with being
in atomic context obviously.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/switchdev.h   |  3 ++
 net/switchdev/switchdev.c | 79 ++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 67 insertions(+), 15 deletions(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index eba80c4fc56f..087761b0df49 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -189,6 +189,9 @@ int call_switchdev_notifiers(unsigned long val, struct net_device *dev,
 int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 				  struct net_device *dev, u32 filter_mask,
 				  int nlflags);
+int switchdev_port_bridge_getlink_deferred(struct sk_buff *skb, u32 pid,
+					   u32 seq, struct net_device *dev,
+					   u32 filter_mask, int nlflags);
 int switchdev_port_bridge_setlink(struct net_device *dev,
 				  struct nlmsghdr *nlh, u16 flags);
 int switchdev_port_bridge_dellink(struct net_device *dev,
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 4fa9972d72d2..ab614a9dd872 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -776,12 +776,14 @@ static int switchdev_port_vlan_dump_cb(struct switchdev_obj *obj)
 	return err;
 }
 
-static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
-				    u32 filter_mask)
+static int __switchdev_port_vlan_fill(struct sk_buff *skb,
+				      struct net_device *dev,
+				      u32 filter_mask, u32 obj_flags)
 {
 	struct switchdev_vlan_dump dump = {
 		.vlan.obj.orig_dev = dev,
 		.vlan.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+		.vlan.obj.flags = obj_flags,
 		.skb = skb,
 		.filter_mask = filter_mask,
 	};
@@ -802,17 +804,27 @@ static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
 	return err == -EOPNOTSUPP ? 0 : err;
 }
 
-/**
- *	switchdev_port_bridge_getlink - Get bridge port attributes
- *
- *	@dev: port device
- *
- *	Called for SELF on rtnl_bridge_getlink to get bridge port
- *	attributes.
- */
-int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
-				  struct net_device *dev, u32 filter_mask,
-				  int nlflags)
+static int switchdev_port_vlan_fill_deferred(struct sk_buff *skb,
+					     struct net_device *dev,
+					     u32 filter_mask)
+{
+	return __switchdev_port_vlan_fill(skb, dev, filter_mask,
+					  SWITCHDEV_F_DEFER);
+}
+
+static int switchdev_port_vlan_fill(struct sk_buff *skb,
+				    struct net_device *dev,
+				    u32 filter_mask)
+{
+	return __switchdev_port_vlan_fill(skb, dev, filter_mask, 0);
+}
+
+static int __switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid,
+					   u32 seq, struct net_device *dev,
+					   u32 filter_mask, int nlflags,
+					   int (*fill_cb)(struct sk_buff *skb,
+							  struct net_device *d,
+							  u32 filter_mask))
 {
 	struct switchdev_attr attr = {
 		.orig_dev = dev,
@@ -829,12 +841,49 @@ int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 	if (err && err != -EOPNOTSUPP)
 		return err;
 
-	return ndo_dflt_bridge_getlink(skb, pid, seq, dev, mode,
+	return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,
 				       attr.u.brport_flags, mask, nlflags,
-				       filter_mask, switchdev_port_vlan_fill);
+				       filter_mask, fill_cb);
+}
+
+/**
+ *	switchdev_port_bridge_getlink - Get bridge port attributes
+ *
+ *	@dev: port device
+ *
+ *	Called for SELF on rtnl_bridge_getlink to get bridge port
+ *	attributes.
+ */
+int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
+				  struct net_device *dev, u32 filter_mask,
+				  int nlflags)
+{
+	return __switchdev_port_bridge_getlink(skb, pid, seq, dev, filter_mask,
+					       nlflags,
+					       switchdev_port_vlan_fill);
 }
 EXPORT_SYMBOL_GPL(switchdev_port_bridge_getlink);
 
+/**
+ *	switchdev_port_bridge_getlink_deferred - Get bridge port attributes
+ *	(deferred variant)
+ *
+ *	@dev: port device
+ *
+ *	Called for SELF on rtnl_bridge_getlink to get bridge port
+ *	attributes.
+ */
+int switchdev_port_bridge_getlink_deferred(struct sk_buff *skb, u32 pid,
+					   u32 seq, struct net_device *dev,
+					   u32 filter_mask,
+					   int nlflags)
+{
+	return __switchdev_port_bridge_getlink(skb, pid, seq, dev, filter_mask,
+					       nlflags,
+					       switchdev_port_vlan_fill_deferred);
+}
+EXPORT_SYMBOL_GPL(switchdev_port_bridge_getlink_deferred);
+
 static int switchdev_port_br_setflag(struct net_device *dev,
 				     struct nlattr *nlattr,
 				     unsigned long brport_flag)
-- 
2.9.3

Re: [PATCH net-next 3/4] net: switchdev: Add switchdev_port_bridge_getlink_deferred

[Marcelo Ricardo Leitner]

On Mon, Jan 09, 2017 at 11:45:02AM -0800, Florian Fainelli wrote:
> Add switchdev_port_bridge_getlink_deferred() which does a deferred
> object dump operation, this is required for e.g: DSA switches which
> typically have sleeping I/O operations which is incompatible with being
> in atomic context obviously.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/net/switchdev.h   |  3 ++
>  net/switchdev/switchdev.c | 79 ++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 67 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index eba80c4fc56f..087761b0df49 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -189,6 +189,9 @@ int call_switchdev_notifiers(unsigned long val, struct net_device *dev,
>  int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
>  				  struct net_device *dev, u32 filter_mask,
>  				  int nlflags);
> +int switchdev_port_bridge_getlink_deferred(struct sk_buff *skb, u32 pid,
> +					   u32 seq, struct net_device *dev,
> +					   u32 filter_mask, int nlflags);
>  int switchdev_port_bridge_setlink(struct net_device *dev,
>  				  struct nlmsghdr *nlh, u16 flags);
>  int switchdev_port_bridge_dellink(struct net_device *dev,
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 4fa9972d72d2..ab614a9dd872 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -776,12 +776,14 @@ static int switchdev_port_vlan_dump_cb(struct switchdev_obj *obj)
>  	return err;
>  }
>  
> -static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
> -				    u32 filter_mask)
> +static int __switchdev_port_vlan_fill(struct sk_buff *skb,
> +				      struct net_device *dev,
> +				      u32 filter_mask, u32 obj_flags)
>  {
>  	struct switchdev_vlan_dump dump = {
>  		.vlan.obj.orig_dev = dev,
>  		.vlan.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
> +		.vlan.obj.flags = obj_flags,
>  		.skb = skb,
>  		.filter_mask = filter_mask,
>  	};
> @@ -802,17 +804,27 @@ static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
>  	return err == -EOPNOTSUPP ? 0 : err;
>  }
>  
> -/**
> - *	switchdev_port_bridge_getlink - Get bridge port attributes
> - *
> - *	@dev: port device
> - *
> - *	Called for SELF on rtnl_bridge_getlink to get bridge port
> - *	attributes.
> - */
> -int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
> -				  struct net_device *dev, u32 filter_mask,
> -				  int nlflags)
> +static int switchdev_port_vlan_fill_deferred(struct sk_buff *skb,
> +					     struct net_device *dev,
> +					     u32 filter_mask)
> +{
> +	return __switchdev_port_vlan_fill(skb, dev, filter_mask,
> +					  SWITCHDEV_F_DEFER);
> +}
> +
> +static int switchdev_port_vlan_fill(struct sk_buff *skb,
> +				    struct net_device *dev,
> +				    u32 filter_mask)
> +{
> +	return __switchdev_port_vlan_fill(skb, dev, filter_mask, 0);
> +}
> +
> +static int __switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid,
> +					   u32 seq, struct net_device *dev,
> +					   u32 filter_mask, int nlflags,
> +					   int (*fill_cb)(struct sk_buff *skb,
> +							  struct net_device *d,
> +							  u32 filter_mask))
>  {
>  	struct switchdev_attr attr = {
>  		.orig_dev = dev,
> @@ -829,12 +841,49 @@ int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
>  	if (err && err != -EOPNOTSUPP)
>  		return err;
>  
> -	return ndo_dflt_bridge_getlink(skb, pid, seq, dev, mode,
> +	return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,

Was this s/dev/d/ by mistake?

>  				       attr.u.brport_flags, mask, nlflags,
> -				       filter_mask, switchdev_port_vlan_fill);
> +				       filter_mask, fill_cb);
> +}
> +
> +/**
> + *	switchdev_port_bridge_getlink - Get bridge port attributes
> + *
> + *	@dev: port device
> + *
> + *	Called for SELF on rtnl_bridge_getlink to get bridge port
> + *	attributes.
> + */
> +int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
> +				  struct net_device *dev, u32 filter_mask,
> +				  int nlflags)
> +{
> +	return __switchdev_port_bridge_getlink(skb, pid, seq, dev, filter_mask,
> +					       nlflags,
> +					       switchdev_port_vlan_fill);
>  }
>  EXPORT_SYMBOL_GPL(switchdev_port_bridge_getlink);
>  
> +/**
> + *	switchdev_port_bridge_getlink_deferred - Get bridge port attributes
> + *	(deferred variant)
> + *
> + *	@dev: port device
> + *
> + *	Called for SELF on rtnl_bridge_getlink to get bridge port
> + *	attributes.
> + */
> +int switchdev_port_bridge_getlink_deferred(struct sk_buff *skb, u32 pid,
> +					   u32 seq, struct net_device *dev,
> +					   u32 filter_mask,
> +					   int nlflags)
> +{
> +	return __switchdev_port_bridge_getlink(skb, pid, seq, dev, filter_mask,
> +					       nlflags,
> +					       switchdev_port_vlan_fill_deferred);
> +}
> +EXPORT_SYMBOL_GPL(switchdev_port_bridge_getlink_deferred);
> +
>  static int switchdev_port_br_setflag(struct net_device *dev,
>  				     struct nlattr *nlattr,
>  				     unsigned long brport_flag)
> -- 
> 2.9.3
> 

Re: [PATCH net-next 3/4] net: switchdev: Add switchdev_port_bridge_getlink_deferred

[Florian Fainelli]

On 01/09/2017 12:11 PM, Marcelo Ricardo Leitner wrote:
> On Mon, Jan 09, 2017 at 11:45:02AM -0800, Florian Fainelli wrote:
>> Add switchdev_port_bridge_getlink_deferred() which does a deferred
>> object dump operation, this is required for e.g: DSA switches which
>> typically have sleeping I/O operations which is incompatible with being
>> in atomic context obviously.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---

>>  	struct switchdev_attr attr = {
>>  		.orig_dev = dev,
>> @@ -829,12 +841,49 @@ int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
>>  	if (err && err != -EOPNOTSUPP)
>>  		return err;
>>  
>> -	return ndo_dflt_bridge_getlink(skb, pid, seq, dev, mode,
>> +	return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,
> 
> Was this s/dev/d/ by mistake?

No, it's not a mistake, it was made so that the function signature could
be within 80 columns.
-- 
Florian

Re: [PATCH net-next 3/4] net: switchdev: Add switchdev_port_bridge_getlink_deferred

[Marcelo Ricardo Leitner]

On Mon, Jan 09, 2017 at 12:13:19PM -0800, Florian Fainelli wrote:
> On 01/09/2017 12:11 PM, Marcelo Ricardo Leitner wrote:
> > On Mon, Jan 09, 2017 at 11:45:02AM -0800, Florian Fainelli wrote:
> >> Add switchdev_port_bridge_getlink_deferred() which does a deferred
> >> object dump operation, this is required for e.g: DSA switches which
> >> typically have sleeping I/O operations which is incompatible with being
> >> in atomic context obviously.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> 
> >>  	struct switchdev_attr attr = {
> >>  		.orig_dev = dev,
> >> @@ -829,12 +841,49 @@ int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
> >>  	if (err && err != -EOPNOTSUPP)
> >>  		return err;
> >>  
> >> -	return ndo_dflt_bridge_getlink(skb, pid, seq, dev, mode,
> >> +	return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,
> > 
> > Was this s/dev/d/ by mistake?
> 
> No, it's not a mistake, it was made so that the function signature could
> be within 80 columns.

Right, but the change on the signature is on the chunk prior to this
one. Yet this one is not on the prototype and leads to:

.../linux/net/switchdev/switchdev.c: In function ‘__switchdev_port_bridge_getlink’:
.../linux/net/switchdev/switchdev.c:844:48: error: ‘d’ undeclared (first use in this function)
  return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,
                                                ^
.../linux/net/switchdev/switchdev.c:844:48: note: each undeclared identifier is reported only once for each function it appears in
.../linux/net/switchdev/switchdev.c:847:1: warning: control reaches end of non-void function [-Wreturn-type]
 }

  Marcelo

Re: [PATCH net-next 3/4] net: switchdev: Add switchdev_port_bridge_getlink_deferred

[Florian Fainelli]

On 01/09/2017 12:28 PM, Marcelo Ricardo Leitner wrote:
> On Mon, Jan 09, 2017 at 12:13:19PM -0800, Florian Fainelli wrote:
>> On 01/09/2017 12:11 PM, Marcelo Ricardo Leitner wrote:
>>> On Mon, Jan 09, 2017 at 11:45:02AM -0800, Florian Fainelli wrote:
>>>> Add switchdev_port_bridge_getlink_deferred() which does a deferred
>>>> object dump operation, this is required for e.g: DSA switches which
>>>> typically have sleeping I/O operations which is incompatible with being
>>>> in atomic context obviously.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>
>>>>  	struct switchdev_attr attr = {
>>>>  		.orig_dev = dev,
>>>> @@ -829,12 +841,49 @@ int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
>>>>  	if (err && err != -EOPNOTSUPP)
>>>>  		return err;
>>>>  
>>>> -	return ndo_dflt_bridge_getlink(skb, pid, seq, dev, mode,
>>>> +	return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,
>>>
>>> Was this s/dev/d/ by mistake?
>>
>> No, it's not a mistake, it was made so that the function signature could
>> be within 80 columns.
> 
> Right, but the change on the signature is on the chunk prior to this
> one. Yet this one is not on the prototype and leads to:
> 
> .../linux/net/switchdev/switchdev.c: In function ‘__switchdev_port_bridge_getlink’:
> .../linux/net/switchdev/switchdev.c:844:48: error: ‘d’ undeclared (first use in this function)
>   return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,
>                                                 ^
> .../linux/net/switchdev/switchdev.c:844:48: note: each undeclared identifier is reported only once for each function it appears in
> .../linux/net/switchdev/switchdev.c:847:1: warning: control reaches end of non-void function [-Wreturn-type]

That's embarrassing, looks like I changed it after the fact to please
checkpatch.pl, that's a mistake. Thanks!
-- 
Florian

Re: [PATCH net-next 3/4] net: switchdev: Add switchdev_port_bridge_getlink_deferred

[kbuild test robot]

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

Hi Florian,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-switchdev-Avoid-sleep-in-atomic-with-DSA/20170110-080755
config: x86_64-randconfig-x014-201702 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   net/switchdev/switchdev.c: In function '__switchdev_port_bridge_getlink':
>> net/switchdev/switchdev.c:844:48: error: 'd' undeclared (first use in this function)
     return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,
                                                   ^
   net/switchdev/switchdev.c:844:48: note: each undeclared identifier is reported only once for each function it appears in
>> net/switchdev/switchdev.c:847:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +/d +844 net/switchdev/switchdev.c

   838			return -EOPNOTSUPP;
   839	
   840		err = switchdev_port_attr_get(dev, &attr);
   841		if (err && err != -EOPNOTSUPP)
   842			return err;
   843	
 > 844		return ndo_dflt_bridge_getlink(skb, pid, seq, d, mode,
   845					       attr.u.brport_flags, mask, nlflags,
   846					       filter_mask, fill_cb);
 > 847	}
   848	
   849	/**
   850	 *	switchdev_port_bridge_getlink - Get bridge port attributes

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30488 bytes --]

[PATCH net-next 4/4] net: dsa: Utilize switchdev_port_bridge_getlink_deferred()

[Florian Fainelli]

Fixes the following sleeping in atomic splat:

[   69.008021] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:752
[   69.016523] in_atomic(): 1, irqs_disabled(): 0, pid: 1528, name: bridge
[   69.023167] INFO: lockdep is turned off.
[   69.027118] CPU: 1 PID: 1528 Comm: bridge Not tainted 4.10.0-rc2-00131-g719651624789-dirty #205
[   69.035840] Hardware name: Broadcom STB (Flattened Device Tree)
[   69.041796] [<c020fc40>] (unwind_backtrace) from [<c020ba28>] (show_stack+0x10/0x14)
[   69.049570] [<c020ba28>] (show_stack) from [<c04fb91c>] (dump_stack+0xb0/0xdc)
[   69.056823] [<c04fb91c>] (dump_stack) from [<c0244d00>] (___might_sleep+0x1a4/0x2a4)
[   69.064599] [<c0244d00>] (___might_sleep) from [<c08f3eb8>] (mutex_lock_nested+0x28/0x7d8)
[   69.072897] [<c08f3eb8>] (mutex_lock_nested) from [<c0674dbc>] (b53_vlan_dump+0x2c/0x104)
[   69.081105] [<c0674dbc>] (b53_vlan_dump) from [<c08eba88>] (switchdev_port_obj_dump_now+0x30/0x6c)
[   69.090094] [<c08eba88>] (switchdev_port_obj_dump_now) from [<c08ebb00>] (switchdev_port_obj_dump+0x3c/0x98)
[   69.099950] [<c08ebb00>] (switchdev_port_obj_dump) from [<c08ebc34>] (switchdev_port_vlan_fill+0x68/0x90)
[   69.109550] [<c08ebc34>] (switchdev_port_vlan_fill) from [<c07f13a4>] (ndo_dflt_bridge_getlink+0x28c/0x4fc)
[   69.119320] [<c07f13a4>] (ndo_dflt_bridge_getlink) from [<c08eb2c8>] (switchdev_port_bridge_getlink+0xc4/0xd4)
[   69.129350] [<c08eb2c8>] (switchdev_port_bridge_getlink) from [<c07f0b10>] (rtnl_bridge_getlink+0x12c/0x28c)
[   69.139206] [<c07f0b10>] (rtnl_bridge_getlink) from [<c0802e28>] (netlink_dump+0xe8/0x268)
[   69.147495] [<c0802e28>] (netlink_dump) from [<c0803864>] (__netlink_dump_start+0x12c/0x18c)
[   69.155958] [<c0803864>] (__netlink_dump_start) from [<c07f3c34>] (rtnetlink_rcv_msg+0x11c/0x228)
[   69.164857] [<c07f3c34>] (rtnetlink_rcv_msg) from [<c0805e3c>] (netlink_rcv_skb+0xc4/0xd8)
[   69.173145] [<c0805e3c>] (netlink_rcv_skb) from [<c07f1110>] (rtnetlink_rcv+0x28/0x30)
[   69.181087] [<c07f1110>] (rtnetlink_rcv) from [<c080577c>] (netlink_unicast+0x16c/0x238)
[   69.189201] [<c080577c>] (netlink_unicast) from [<c0805c3c>] (netlink_sendmsg+0x350/0x364)
[   69.197493] [<c0805c3c>] (netlink_sendmsg) from [<c07beab0>] (sock_sendmsg+0x14/0x24)
[   69.205350] [<c07beab0>] (sock_sendmsg) from [<c07bfdbc>] (SyS_sendto+0xb8/0xe0)
[   69.212769] [<c07bfdbc>] (SyS_sendto) from [<c07bfdfc>] (SyS_send+0x18/0x20)
[   69.219841] [<c07bfdfc>] (SyS_send) from [<c0208100>] (ret_fast_syscall+0x0/0x1c)

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/slave.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 5cd5b8137c08..b38536f951ea 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1038,7 +1038,7 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_netpoll_cleanup	= dsa_slave_netpoll_cleanup,
 	.ndo_poll_controller	= dsa_slave_poll_controller,
 #endif
-	.ndo_bridge_getlink	= switchdev_port_bridge_getlink,
+	.ndo_bridge_getlink	= switchdev_port_bridge_getlink_deferred,
 	.ndo_bridge_setlink	= switchdev_port_bridge_setlink,
 	.ndo_bridge_dellink	= switchdev_port_bridge_dellink,
 	.ndo_get_phys_port_id	= dsa_slave_get_phys_port_id,
-- 
2.9.3

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Ido Schimmel]

Hi Florian,

On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
> Hi all,
> 
> This patch series is to resolve a sleeping function called in atomic context
> debug splat that we observe with DSA.
> 
> Let me know what you think, I was also wondering if we should just always
> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.

Isn't this a bit of overkill? All the drivers you mention fill the VLAN
dump from their cache and don't require sleeping. Even b53 that you
mention in the last patch does that, but reads the PVID from the device,
which entails taking a mutex.

Can't you just cache the PVID as well? I think this will solve your
problem. Didn't look too much into the b53 code, so maybe I'm missing
something. Seems that mv88e6xxx has a similar problem.

Thanks!

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Florian Fainelli]

On 01/09/2017 12:48 PM, Ido Schimmel wrote:
> Hi Florian,
> 
> On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
>> Hi all,
>>
>> This patch series is to resolve a sleeping function called in atomic context
>> debug splat that we observe with DSA.
>>
>> Let me know what you think, I was also wondering if we should just always
>> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
>> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
> 
> Isn't this a bit of overkill? All the drivers you mention fill the VLAN
> dump from their cache and don't require sleeping. Even b53 that you
> mention in the last patch does that, but reads the PVID from the device,
> which entails taking a mutex.

Correct.

> 
> Can't you just cache the PVID as well? I think this will solve your
> problem. Didn't look too much into the b53 code, so maybe I'm missing
> something. Seems that mv88e6xxx has a similar problem.

I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
seems like we need to perform a bunch of VTU operations, and those
access HW registers, Andrew, Vivien, how do you want to solve that, do
we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?

Thanks Ido!
-- 
Florian

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Ido Schimmel]

On Mon, Jan 09, 2017 at 12:56:48PM -0800, Florian Fainelli wrote:
> On 01/09/2017 12:48 PM, Ido Schimmel wrote:
> > Hi Florian,
> > 
> > On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
> >> Hi all,
> >>
> >> This patch series is to resolve a sleeping function called in atomic context
> >> debug splat that we observe with DSA.
> >>
> >> Let me know what you think, I was also wondering if we should just always
> >> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
> >> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
> > 
> > Isn't this a bit of overkill? All the drivers you mention fill the VLAN
> > dump from their cache and don't require sleeping. Even b53 that you
> > mention in the last patch does that, but reads the PVID from the device,
> > which entails taking a mutex.
> 
> Correct.
> 
> > 
> > Can't you just cache the PVID as well? I think this will solve your
> > problem. Didn't look too much into the b53 code, so maybe I'm missing
> > something. Seems that mv88e6xxx has a similar problem.
> 
> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
> seems like we need to perform a bunch of VTU operations, and those
> access HW registers, Andrew, Vivien, how do you want to solve that, do
> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?

Truth be told, I don't quite understand why switchdev infra even tries
to dump the VLANs from the device. Like, in which situations is this
going to be different from what the software bridge reports? Sure, you
can set the VLAN filters with SELF and skip the software bridge, but how
does that make sense in a model where you want to reflect the software
datapath?

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Andrew Lunn]

> Truth be told, I don't quite understand why switchdev infra even tries
> to dump the VLANs from the device. Like, in which situations is this
> going to be different from what the software bridge reports?

What happens when the hardware is out of resources and says sorry,
cannot do that. There has been a few discussions about what to do in
that situation. Fall back to software, i.e. the software bridge does
it, or totally fail the operation. If you are failing back to
software, the states can be different.

    Andrew

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Ido Schimmel]

On Mon, Jan 09, 2017 at 10:23:20PM +0100, Andrew Lunn wrote:
> > Truth be told, I don't quite understand why switchdev infra even tries
> > to dump the VLANs from the device. Like, in which situations is this
> > going to be different from what the software bridge reports?
> 
> What happens when the hardware is out of resources and says sorry,
> cannot do that. There has been a few discussions about what to do in
> that situation. Fall back to software, i.e. the software bridge does
> it, or totally fail the operation. If you are failing back to
> software, the states can be different.

If the driver fails to set its VLAN filters then the operation is also
rollbacked in the software bridge, in which case you're still in sync.

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Jiri Pirko]

Mon, Jan 09, 2017 at 10:14:36PM CET, idosch@idosch.org wrote:
>On Mon, Jan 09, 2017 at 12:56:48PM -0800, Florian Fainelli wrote:
>> On 01/09/2017 12:48 PM, Ido Schimmel wrote:
>> > Hi Florian,
>> > 
>> > On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
>> >> Hi all,
>> >>
>> >> This patch series is to resolve a sleeping function called in atomic context
>> >> debug splat that we observe with DSA.
>> >>
>> >> Let me know what you think, I was also wondering if we should just always
>> >> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
>> >> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
>> > 
>> > Isn't this a bit of overkill? All the drivers you mention fill the VLAN
>> > dump from their cache and don't require sleeping. Even b53 that you
>> > mention in the last patch does that, but reads the PVID from the device,
>> > which entails taking a mutex.
>> 
>> Correct.
>> 
>> > 
>> > Can't you just cache the PVID as well? I think this will solve your
>> > problem. Didn't look too much into the b53 code, so maybe I'm missing
>> > something. Seems that mv88e6xxx has a similar problem.
>> 
>> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
>> seems like we need to perform a bunch of VTU operations, and those
>> access HW registers, Andrew, Vivien, how do you want to solve that, do
>> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?
>
>Truth be told, I don't quite understand why switchdev infra even tries
>to dump the VLANs from the device. Like, in which situations is this
>going to be different from what the software bridge reports? Sure, you
>can set the VLAN filters with SELF and skip the software bridge, but how
>does that make sense in a model where you want to reflect the software
>datapath?

But the vlans added by rtnl_bridge_setlink & SELF are not tracked by the
bridge and therefore driver needs to dump them. You would have to pass
some flag down to driver when adding SWITCHDEV_OBJ_ID_PORT_VLAN
indicating the need to track the vlan and dump it. Right?

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Ido Schimmel]

On Tue, Jan 10, 2017 at 01:08:46PM +0100, Jiri Pirko wrote:
> Mon, Jan 09, 2017 at 10:14:36PM CET, idosch@idosch.org wrote:
> >On Mon, Jan 09, 2017 at 12:56:48PM -0800, Florian Fainelli wrote:
> >> On 01/09/2017 12:48 PM, Ido Schimmel wrote:
> >> > Hi Florian,
> >> > 
> >> > On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
> >> >> Hi all,
> >> >>
> >> >> This patch series is to resolve a sleeping function called in atomic context
> >> >> debug splat that we observe with DSA.
> >> >>
> >> >> Let me know what you think, I was also wondering if we should just always
> >> >> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
> >> >> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
> >> > 
> >> > Isn't this a bit of overkill? All the drivers you mention fill the VLAN
> >> > dump from their cache and don't require sleeping. Even b53 that you
> >> > mention in the last patch does that, but reads the PVID from the device,
> >> > which entails taking a mutex.
> >> 
> >> Correct.
> >> 
> >> > 
> >> > Can't you just cache the PVID as well? I think this will solve your
> >> > problem. Didn't look too much into the b53 code, so maybe I'm missing
> >> > something. Seems that mv88e6xxx has a similar problem.
> >> 
> >> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
> >> seems like we need to perform a bunch of VTU operations, and those
> >> access HW registers, Andrew, Vivien, how do you want to solve that, do
> >> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?
> >
> >Truth be told, I don't quite understand why switchdev infra even tries
> >to dump the VLANs from the device. Like, in which situations is this
> >going to be different from what the software bridge reports? Sure, you
> >can set the VLAN filters with SELF and skip the software bridge, but how
> >does that make sense in a model where you want to reflect the software
> >datapath?
> 
> But the vlans added by rtnl_bridge_setlink & SELF are not tracked by the
> bridge and therefore driver needs to dump them. You would have to pass
> some flag down to driver when adding SWITCHDEV_OBJ_ID_PORT_VLAN
> indicating the need to track the vlan and dump it. Right?

Right, but back to my question - what's the use case for the SELF flag
in the switchdev model? Why would I configure a VLAN filter in the
hardware but not in the software bridge? The whole point is reflecting
the software bridge to the hardware.

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Jiri Pirko]

Tue, Jan 10, 2017 at 02:25:06PM CET, idosch@idosch.org wrote:
>On Tue, Jan 10, 2017 at 01:08:46PM +0100, Jiri Pirko wrote:
>> Mon, Jan 09, 2017 at 10:14:36PM CET, idosch@idosch.org wrote:
>> >On Mon, Jan 09, 2017 at 12:56:48PM -0800, Florian Fainelli wrote:
>> >> On 01/09/2017 12:48 PM, Ido Schimmel wrote:
>> >> > Hi Florian,
>> >> > 
>> >> > On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
>> >> >> Hi all,
>> >> >>
>> >> >> This patch series is to resolve a sleeping function called in atomic context
>> >> >> debug splat that we observe with DSA.
>> >> >>
>> >> >> Let me know what you think, I was also wondering if we should just always
>> >> >> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
>> >> >> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
>> >> > 
>> >> > Isn't this a bit of overkill? All the drivers you mention fill the VLAN
>> >> > dump from their cache and don't require sleeping. Even b53 that you
>> >> > mention in the last patch does that, but reads the PVID from the device,
>> >> > which entails taking a mutex.
>> >> 
>> >> Correct.
>> >> 
>> >> > 
>> >> > Can't you just cache the PVID as well? I think this will solve your
>> >> > problem. Didn't look too much into the b53 code, so maybe I'm missing
>> >> > something. Seems that mv88e6xxx has a similar problem.
>> >> 
>> >> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
>> >> seems like we need to perform a bunch of VTU operations, and those
>> >> access HW registers, Andrew, Vivien, how do you want to solve that, do
>> >> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?
>> >
>> >Truth be told, I don't quite understand why switchdev infra even tries
>> >to dump the VLANs from the device. Like, in which situations is this
>> >going to be different from what the software bridge reports? Sure, you
>> >can set the VLAN filters with SELF and skip the software bridge, but how
>> >does that make sense in a model where you want to reflect the software
>> >datapath?
>> 
>> But the vlans added by rtnl_bridge_setlink & SELF are not tracked by the
>> bridge and therefore driver needs to dump them. You would have to pass
>> some flag down to driver when adding SWITCHDEV_OBJ_ID_PORT_VLAN
>> indicating the need to track the vlan and dump it. Right?
>
>Right, but back to my question - what's the use case for the SELF flag
>in the switchdev model? Why would I configure a VLAN filter in the
>hardware but not in the software bridge? The whole point is reflecting
>the software bridge to the hardware.

I agree. For the bridge-switchdev usecase, I don't see a reason to use
SELF for vlans as well. Do you suggest to simply remove this possibility?

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Ido Schimmel]

On Tue, Jan 10, 2017 at 03:18:07PM +0100, Jiri Pirko wrote:
> Tue, Jan 10, 2017 at 02:25:06PM CET, idosch@idosch.org wrote:
> >On Tue, Jan 10, 2017 at 01:08:46PM +0100, Jiri Pirko wrote:
> >> Mon, Jan 09, 2017 at 10:14:36PM CET, idosch@idosch.org wrote:
> >> >On Mon, Jan 09, 2017 at 12:56:48PM -0800, Florian Fainelli wrote:
> >> >> On 01/09/2017 12:48 PM, Ido Schimmel wrote:
> >> >> > Hi Florian,
> >> >> > 
> >> >> > On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
> >> >> >> Hi all,
> >> >> >>
> >> >> >> This patch series is to resolve a sleeping function called in atomic context
> >> >> >> debug splat that we observe with DSA.
> >> >> >>
> >> >> >> Let me know what you think, I was also wondering if we should just always
> >> >> >> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
> >> >> >> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
> >> >> > 
> >> >> > Isn't this a bit of overkill? All the drivers you mention fill the VLAN
> >> >> > dump from their cache and don't require sleeping. Even b53 that you
> >> >> > mention in the last patch does that, but reads the PVID from the device,
> >> >> > which entails taking a mutex.
> >> >> 
> >> >> Correct.
> >> >> 
> >> >> > 
> >> >> > Can't you just cache the PVID as well? I think this will solve your
> >> >> > problem. Didn't look too much into the b53 code, so maybe I'm missing
> >> >> > something. Seems that mv88e6xxx has a similar problem.
> >> >> 
> >> >> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
> >> >> seems like we need to perform a bunch of VTU operations, and those
> >> >> access HW registers, Andrew, Vivien, how do you want to solve that, do
> >> >> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?
> >> >
> >> >Truth be told, I don't quite understand why switchdev infra even tries
> >> >to dump the VLANs from the device. Like, in which situations is this
> >> >going to be different from what the software bridge reports? Sure, you
> >> >can set the VLAN filters with SELF and skip the software bridge, but how
> >> >does that make sense in a model where you want to reflect the software
> >> >datapath?
> >> 
> >> But the vlans added by rtnl_bridge_setlink & SELF are not tracked by the
> >> bridge and therefore driver needs to dump them. You would have to pass
> >> some flag down to driver when adding SWITCHDEV_OBJ_ID_PORT_VLAN
> >> indicating the need to track the vlan and dump it. Right?
> >
> >Right, but back to my question - what's the use case for the SELF flag
> >in the switchdev model? Why would I configure a VLAN filter in the
> >hardware but not in the software bridge? The whole point is reflecting
> >the software bridge to the hardware.
> 
> I agree. For the bridge-switchdev usecase, I don't see a reason to use
> SELF for vlans as well. Do you suggest to simply remove this possibility?

Yes. This would also solve Florian's problem.

Florian, what do you think?

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Jiri Pirko]

Tue, Jan 10, 2017 at 05:08:52PM CET, idosch@idosch.org wrote:
>On Tue, Jan 10, 2017 at 03:18:07PM +0100, Jiri Pirko wrote:
>> Tue, Jan 10, 2017 at 02:25:06PM CET, idosch@idosch.org wrote:
>> >On Tue, Jan 10, 2017 at 01:08:46PM +0100, Jiri Pirko wrote:
>> >> Mon, Jan 09, 2017 at 10:14:36PM CET, idosch@idosch.org wrote:
>> >> >On Mon, Jan 09, 2017 at 12:56:48PM -0800, Florian Fainelli wrote:
>> >> >> On 01/09/2017 12:48 PM, Ido Schimmel wrote:
>> >> >> > Hi Florian,
>> >> >> > 
>> >> >> > On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
>> >> >> >> Hi all,
>> >> >> >>
>> >> >> >> This patch series is to resolve a sleeping function called in atomic context
>> >> >> >> debug splat that we observe with DSA.
>> >> >> >>
>> >> >> >> Let me know what you think, I was also wondering if we should just always
>> >> >> >> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
>> >> >> >> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
>> >> >> > 
>> >> >> > Isn't this a bit of overkill? All the drivers you mention fill the VLAN
>> >> >> > dump from their cache and don't require sleeping. Even b53 that you
>> >> >> > mention in the last patch does that, but reads the PVID from the device,
>> >> >> > which entails taking a mutex.
>> >> >> 
>> >> >> Correct.
>> >> >> 
>> >> >> > 
>> >> >> > Can't you just cache the PVID as well? I think this will solve your
>> >> >> > problem. Didn't look too much into the b53 code, so maybe I'm missing
>> >> >> > something. Seems that mv88e6xxx has a similar problem.
>> >> >> 
>> >> >> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
>> >> >> seems like we need to perform a bunch of VTU operations, and those
>> >> >> access HW registers, Andrew, Vivien, how do you want to solve that, do
>> >> >> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?
>> >> >
>> >> >Truth be told, I don't quite understand why switchdev infra even tries
>> >> >to dump the VLANs from the device. Like, in which situations is this
>> >> >going to be different from what the software bridge reports? Sure, you
>> >> >can set the VLAN filters with SELF and skip the software bridge, but how
>> >> >does that make sense in a model where you want to reflect the software
>> >> >datapath?
>> >> 
>> >> But the vlans added by rtnl_bridge_setlink & SELF are not tracked by the
>> >> bridge and therefore driver needs to dump them. You would have to pass
>> >> some flag down to driver when adding SWITCHDEV_OBJ_ID_PORT_VLAN
>> >> indicating the need to track the vlan and dump it. Right?
>> >
>> >Right, but back to my question - what's the use case for the SELF flag
>> >in the switchdev model? Why would I configure a VLAN filter in the
>> >hardware but not in the software bridge? The whole point is reflecting
>> >the software bridge to the hardware.
>> 
>> I agree. For the bridge-switchdev usecase, I don't see a reason to use
>> SELF for vlans as well. Do you suggest to simply remove this possibility?
>
>Yes. This would also solve Florian's problem.

I agree, for switchdev usecase it makes no sense to implement this. I
vote for removing that.


>
>Florian, what do you think?

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Florian Fainelli]

On 01/10/2017 08:08 AM, Ido Schimmel wrote:
> On Tue, Jan 10, 2017 at 03:18:07PM +0100, Jiri Pirko wrote:
>> Tue, Jan 10, 2017 at 02:25:06PM CET, idosch@idosch.org wrote:
>>> On Tue, Jan 10, 2017 at 01:08:46PM +0100, Jiri Pirko wrote:
>>>> Mon, Jan 09, 2017 at 10:14:36PM CET, idosch@idosch.org wrote:
>>>>> On Mon, Jan 09, 2017 at 12:56:48PM -0800, Florian Fainelli wrote:
>>>>>> On 01/09/2017 12:48 PM, Ido Schimmel wrote:
>>>>>>> Hi Florian,
>>>>>>>
>>>>>>> On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> This patch series is to resolve a sleeping function called in atomic context
>>>>>>>> debug splat that we observe with DSA.
>>>>>>>>
>>>>>>>> Let me know what you think, I was also wondering if we should just always
>>>>>>>> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
>>>>>>>> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
>>>>>>>
>>>>>>> Isn't this a bit of overkill? All the drivers you mention fill the VLAN
>>>>>>> dump from their cache and don't require sleeping. Even b53 that you
>>>>>>> mention in the last patch does that, but reads the PVID from the device,
>>>>>>> which entails taking a mutex.
>>>>>>
>>>>>> Correct.
>>>>>>
>>>>>>>
>>>>>>> Can't you just cache the PVID as well? I think this will solve your
>>>>>>> problem. Didn't look too much into the b53 code, so maybe I'm missing
>>>>>>> something. Seems that mv88e6xxx has a similar problem.
>>>>>>
>>>>>> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
>>>>>> seems like we need to perform a bunch of VTU operations, and those
>>>>>> access HW registers, Andrew, Vivien, how do you want to solve that, do
>>>>>> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?
>>>>>
>>>>> Truth be told, I don't quite understand why switchdev infra even tries
>>>>> to dump the VLANs from the device. Like, in which situations is this
>>>>> going to be different from what the software bridge reports? Sure, you
>>>>> can set the VLAN filters with SELF and skip the software bridge, but how
>>>>> does that make sense in a model where you want to reflect the software
>>>>> datapath?
>>>>
>>>> But the vlans added by rtnl_bridge_setlink & SELF are not tracked by the
>>>> bridge and therefore driver needs to dump them. You would have to pass
>>>> some flag down to driver when adding SWITCHDEV_OBJ_ID_PORT_VLAN
>>>> indicating the need to track the vlan and dump it. Right?
>>>
>>> Right, but back to my question - what's the use case for the SELF flag
>>> in the switchdev model? Why would I configure a VLAN filter in the
>>> hardware but not in the software bridge? The whole point is reflecting
>>> the software bridge to the hardware.
>>
>> I agree. For the bridge-switchdev usecase, I don't see a reason to use
>> SELF for vlans as well. Do you suggest to simply remove this possibility?
> 
> Yes. This would also solve Florian's problem.
> 
> Florian, what do you think?

I don't really see a value for configuring a VLAN in HW exclusively and
not at the SW bridge level as well (unless you wanted some kind of
discarding of packets?). Even with the patch proposed a while back to
decouple the bridge master device, we would still end up with the same
VLAN attributes programmed in HW and SW.

Vivien, Andrew, are you aware of an use case where we have the HW
programmed with a superset of VLANs wrt. the SW bridge?
-- 
Florian

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Andrew Lunn]

> Vivien, Andrew, are you aware of an use case where we have the HW
> programmed with a superset of VLANs wrt. the SW bridge?

I cannot think of any case with a superset. Subset yes, but not
superset.

	Andrew

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Vivien Didelot]

Hi Florian, Ido,

Florian Fainelli <f.fainelli@gmail.com> writes:

>> Can't you just cache the PVID as well? I think this will solve your
>> problem. Didn't look too much into the b53 code, so maybe I'm missing
>> something. Seems that mv88e6xxx has a similar problem.
>
> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
> seems like we need to perform a bunch of VTU operations, and those
> access HW registers, Andrew, Vivien, how do you want to solve that, do
> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?

Yes mv88e6xxx does read the hardware registers to get the port default
VID and read the hardware VLAN table.

DSA drivers should be dumb and simply implement switch operations. If
caching is required, it must be implemented in the DSA layer. Since
switchdev is stateless, I hardly see a way to implement it there.

Thanks,

        Vivien

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 9 Jan 2017 12:56:48 -0800

> On 01/09/2017 12:48 PM, Ido Schimmel wrote:
>> Hi Florian,
>> 
>> On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
>>> Hi all,
>>>
>>> This patch series is to resolve a sleeping function called in atomic context
>>> debug splat that we observe with DSA.
>>>
>>> Let me know what you think, I was also wondering if we should just always
>>> make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
>>> could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
>> 
>> Isn't this a bit of overkill? All the drivers you mention fill the VLAN
>> dump from their cache and don't require sleeping. Even b53 that you
>> mention in the last patch does that, but reads the PVID from the device,
>> which entails taking a mutex.
> 
> Correct.
> 
>> 
>> Can't you just cache the PVID as well? I think this will solve your
>> problem. Didn't look too much into the b53 code, so maybe I'm missing
>> something. Seems that mv88e6xxx has a similar problem.
> 
> I suppose we could indeed cache the PVID for b53, but for mv88e6xxx it
> seems like we need to perform a bunch of VTU operations, and those
> access HW registers, Andrew, Vivien, how do you want to solve that, do
> we want to introduce a general VLAN cache somewhere in switchdev/DSA/driver?

I would definitely prefer to see cached state used instead of this deferred
operation stuff.

Re: [PATCH net-next 0/4] net: switchdev: Avoid sleep in atomic with DSA

[Andrew Lunn]

On Mon, Jan 09, 2017 at 10:48:49PM +0200, Ido Schimmel wrote:
> Hi Florian,
> 
> On Mon, Jan 09, 2017 at 11:44:59AM -0800, Florian Fainelli wrote:
> > Hi all,
> > 
> > This patch series is to resolve a sleeping function called in atomic context
> > debug splat that we observe with DSA.
> > 
> > Let me know what you think, I was also wondering if we should just always
> > make switchdev_port_vlan_fill() set SWITCHDEV_F_DEFER, but was afraid this
> > could cause invalid contexts to be used for rocker, mlxsw, i40e etc.
> 
> Isn't this a bit of overkill? All the drivers you mention fill the VLAN
> dump from their cache and don't require sleeping.

Hi Ido

DSA in general does not cache information. It always ask the hardware.
So for mv88e6xxx, this is going to trigger MDIO operations, which take
mutex's and do sleep.

	Andrew