* [PATCH net-next v2 09/12] net: sched: flower: handle concurrent tcf proto deletion
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
Without rtnl lock protection tcf proto can be deleted concurrently. Check
tcf proto 'deleting' flag after taking tcf spinlock to verify that no
concurrent deletion is in progress. Return EAGAIN error if concurrent
deletion detected, which will cause caller to retry and possibly create new
instance of tcf proto.
Retry mechanism is a result of fine-grained locking approach used in this
and previous changes in series and is necessary to allow concurrent updates
on same chain instance. Alternative approach would be to lock the whole
chain while updating filters on any of child tp's, adding and removing
classifier instances from the chain. However, since most CPU-intensive
parts of filter update code are specifically in classifier code and its
dependencies (extensions and hw offloads), such approach would negate most
of the gains introduced by this change and previous changes in the series
when updating same chain instance.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Extend commit message.
- Fix error code in comment.
net/sched/cls_flower.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 70b357f23391..25a4d64b82db 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1500,6 +1500,14 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
if (!tc_in_hw(fnew->flags))
fnew->flags |= TCA_CLS_FLAGS_NOT_IN_HW;
+ /* tp was deleted concurrently. -EAGAIN will cause caller to lookup
+ * proto again or create new one, if necessary.
+ */
+ if (tp->deleting) {
+ err = -EAGAIN;
+ goto errout_hw;
+ }
+
refcount_inc(&fnew->refcnt);
if (fold) {
/* Fold filter was deleted concurrently. Retry lookup. */
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 07/12] net: sched: flower: protect masks list with spinlock
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
Protect modifications of flower masks list with spinlock to remove
dependency on rtnl lock and allow concurrent access.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_flower.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 92478bb122d3..db47828ea5e2 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -88,6 +88,7 @@ struct fl_flow_tmplt {
struct cls_fl_head {
struct rhashtable ht;
+ spinlock_t masks_lock; /* Protect masks list */
struct list_head masks;
struct rcu_work rwork;
struct idr handle_idr;
@@ -312,6 +313,7 @@ static int fl_init(struct tcf_proto *tp)
if (!head)
return -ENOBUFS;
+ spin_lock_init(&head->masks_lock);
INIT_LIST_HEAD_RCU(&head->masks);
rcu_assign_pointer(tp->root, head);
idr_init(&head->handle_idr);
@@ -341,7 +343,11 @@ static bool fl_mask_put(struct cls_fl_head *head, struct fl_flow_mask *mask,
return false;
rhashtable_remove_fast(&head->ht, &mask->ht_node, mask_ht_params);
+
+ spin_lock(&head->masks_lock);
list_del_rcu(&mask->list);
+ spin_unlock(&head->masks_lock);
+
if (async)
tcf_queue_work(&mask->rwork, fl_mask_free_work);
else
@@ -1312,7 +1318,9 @@ static struct fl_flow_mask *fl_create_new_mask(struct cls_fl_head *head,
/* Wait until any potential concurrent users of mask are finished */
synchronize_rcu();
+ spin_lock(&head->masks_lock);
list_add_tail_rcu(&newmask->list, &head->masks);
+ spin_unlock(&head->masks_lock);
return newmask;
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 03/12] net: sched: flower: introduce reference counting for filters
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
Extend flower filters with reference counting in order to remove dependency
on rtnl lock in flower ops and allow to modify filters concurrently.
Reference to flower filter can be taken/released concurrently as soon as it
is marked as 'unlocked' by last patch in this series. Use atomic reference
counter type to make concurrent modifications safe.
Always take reference to flower filter while working with it:
- Modify fl_get() to take reference to filter.
- Implement tp->put() callback as fl_put() function to allow cls API to
release reference taken by fl_get().
- Modify fl_change() to assume that caller holds reference to fold and take
reference to fnew.
- Take reference to filter while using it in fl_walk().
Implement helper functions to get/put filter reference counter.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Refactor loop in fl_get_next_filter() to improve readability.
net/sched/cls_flower.c | 96 ++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 82 insertions(+), 14 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index d36ceb5001f9..9ed7c9b804a7 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -14,6 +14,7 @@
#include <linux/module.h>
#include <linux/rhashtable.h>
#include <linux/workqueue.h>
+#include <linux/refcount.h>
#include <linux/if_ether.h>
#include <linux/in6.h>
@@ -104,6 +105,11 @@ struct cls_fl_filter {
u32 in_hw_count;
struct rcu_work rwork;
struct net_device *hw_dev;
+ /* Flower classifier is unlocked, which means that its reference counter
+ * can be changed concurrently without any kind of external
+ * synchronization. Use atomic reference counter to be concurrency-safe.
+ */
+ refcount_t refcnt;
};
static const struct rhashtable_params mask_ht_params = {
@@ -447,6 +453,48 @@ static struct cls_fl_head *fl_head_dereference(struct tcf_proto *tp)
return rcu_dereference_raw(tp->root);
}
+static void __fl_put(struct cls_fl_filter *f)
+{
+ if (!refcount_dec_and_test(&f->refcnt))
+ return;
+
+ if (tcf_exts_get_net(&f->exts))
+ tcf_queue_work(&f->rwork, fl_destroy_filter_work);
+ else
+ __fl_destroy_filter(f);
+}
+
+static struct cls_fl_filter *__fl_get(struct cls_fl_head *head, u32 handle)
+{
+ struct cls_fl_filter *f;
+
+ rcu_read_lock();
+ f = idr_find(&head->handle_idr, handle);
+ if (f && !refcount_inc_not_zero(&f->refcnt))
+ f = NULL;
+ rcu_read_unlock();
+
+ return f;
+}
+
+static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp,
+ unsigned long *handle)
+{
+ struct cls_fl_head *head = fl_head_dereference(tp);
+ struct cls_fl_filter *f;
+
+ rcu_read_lock();
+ while ((f = idr_get_next_ul(&head->handle_idr, handle))) {
+ /* don't return filters that are being deleted */
+ if (refcount_inc_not_zero(&f->refcnt))
+ break;
+ ++(*handle);
+ }
+ rcu_read_unlock();
+
+ return f;
+}
+
static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
struct netlink_ext_ack *extack)
{
@@ -460,10 +508,7 @@ static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
if (!tc_skip_hw(f->flags))
fl_hw_destroy_filter(tp, f, extack);
tcf_unbind_filter(tp, &f->res);
- if (async)
- tcf_queue_work(&f->rwork, fl_destroy_filter_work);
- else
- __fl_destroy_filter(f);
+ __fl_put(f);
return last;
}
@@ -498,11 +543,18 @@ static void fl_destroy(struct tcf_proto *tp, bool rtnl_held,
tcf_queue_work(&head->rwork, fl_destroy_sleepable);
}
+static void fl_put(struct tcf_proto *tp, void *arg)
+{
+ struct cls_fl_filter *f = arg;
+
+ __fl_put(f);
+}
+
static void *fl_get(struct tcf_proto *tp, u32 handle)
{
struct cls_fl_head *head = fl_head_dereference(tp);
- return idr_find(&head->handle_idr, handle);
+ return __fl_get(head, handle);
}
static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
@@ -1325,12 +1377,16 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
struct nlattr **tb;
int err;
- if (!tca[TCA_OPTIONS])
- return -EINVAL;
+ if (!tca[TCA_OPTIONS]) {
+ err = -EINVAL;
+ goto errout_fold;
+ }
mask = kzalloc(sizeof(struct fl_flow_mask), GFP_KERNEL);
- if (!mask)
- return -ENOBUFS;
+ if (!mask) {
+ err = -ENOBUFS;
+ goto errout_fold;
+ }
tb = kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL);
if (!tb) {
@@ -1353,6 +1409,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
err = -ENOBUFS;
goto errout_tb;
}
+ refcount_set(&fnew->refcnt, 1);
err = tcf_exts_init(&fnew->exts, net, TCA_FLOWER_ACT, 0);
if (err < 0)
@@ -1385,6 +1442,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
if (!tc_in_hw(fnew->flags))
fnew->flags |= TCA_CLS_FLAGS_NOT_IN_HW;
+ refcount_inc(&fnew->refcnt);
if (fold) {
fnew->handle = handle;
@@ -1403,7 +1461,11 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
fl_hw_destroy_filter(tp, fold, NULL);
tcf_unbind_filter(tp, &fold->res);
tcf_exts_get_net(&fold->exts);
- tcf_queue_work(&fold->rwork, fl_destroy_filter_work);
+ /* Caller holds reference to fold, so refcnt is always > 0
+ * after this.
+ */
+ refcount_dec(&fold->refcnt);
+ __fl_put(fold);
} else {
if (__fl_lookup(fnew->mask, &fnew->mkey)) {
err = -EEXIST;
@@ -1452,6 +1514,9 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
kfree(tb);
errout_mask_alloc:
kfree(mask);
+errout_fold:
+ if (fold)
+ __fl_put(fold);
return err;
}
@@ -1465,24 +1530,26 @@ static int fl_delete(struct tcf_proto *tp, void *arg, bool *last,
f->mask->filter_ht_params);
__fl_delete(tp, f, extack);
*last = list_empty(&head->masks);
+ __fl_put(f);
+
return 0;
}
static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg,
bool rtnl_held)
{
- struct cls_fl_head *head = fl_head_dereference(tp);
struct cls_fl_filter *f;
arg->count = arg->skip;
- while ((f = idr_get_next_ul(&head->handle_idr,
- &arg->cookie)) != NULL) {
+ while ((f = fl_get_next_filter(tp, &arg->cookie)) != NULL) {
if (arg->fn(tp, f, arg) < 0) {
+ __fl_put(f);
arg->stop = 1;
break;
}
- arg->cookie = f->handle + 1;
+ __fl_put(f);
+ arg->cookie++;
arg->count++;
}
}
@@ -2156,6 +2223,7 @@ static struct tcf_proto_ops cls_fl_ops __read_mostly = {
.init = fl_init,
.destroy = fl_destroy,
.get = fl_get,
+ .put = fl_put,
.change = fl_change,
.delete = fl_delete,
.walk = fl_walk,
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 10/12] net: sched: flower: protect flower classifier state with spinlock
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
struct tcf_proto was extended with spinlock to be used by classifiers
instead of global rtnl lock. Use it to protect shared flower classifier
data structures (handle_idr, mask hashtable and list) and fields of
individual filters that can be accessed concurrently. This patch set uses
tcf_proto->lock as per instance lock that protects all filters on
tcf_proto.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/cls_flower.c | 39 ++++++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 7 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 25a4d64b82db..04210d645c78 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -384,7 +384,9 @@ static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f,
cls_flower.cookie = (unsigned long) f;
tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false);
+ spin_lock(&tp->lock);
tcf_block_offload_dec(block, &f->flags);
+ spin_unlock(&tp->lock);
}
static int fl_hw_replace_filter(struct tcf_proto *tp,
@@ -426,7 +428,9 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
return err;
} else if (err > 0) {
f->in_hw_count = err;
+ spin_lock(&tp->lock);
tcf_block_offload_inc(block, &f->flags);
+ spin_unlock(&tp->lock);
}
if (skip_sw && !(f->flags & TCA_CLS_FLAGS_IN_HW))
@@ -514,14 +518,19 @@ static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
*last = false;
- if (f->deleted)
+ spin_lock(&tp->lock);
+ if (f->deleted) {
+ spin_unlock(&tp->lock);
return -ENOENT;
+ }
f->deleted = true;
rhashtable_remove_fast(&f->mask->ht, &f->ht_node,
f->mask->filter_ht_params);
idr_remove(&head->handle_idr, f->handle);
list_del_rcu(&f->list);
+ spin_unlock(&tp->lock);
+
*last = fl_mask_put(head, f->mask, async);
if (!tc_skip_hw(f->flags))
fl_hw_destroy_filter(tp, f, extack);
@@ -1500,6 +1509,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
if (!tc_in_hw(fnew->flags))
fnew->flags |= TCA_CLS_FLAGS_NOT_IN_HW;
+ spin_lock(&tp->lock);
+
/* tp was deleted concurrently. -EAGAIN will cause caller to lookup
* proto again or create new one, if necessary.
*/
@@ -1530,6 +1541,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
list_replace_rcu(&fold->list, &fnew->list);
fold->deleted = true;
+ spin_unlock(&tp->lock);
+
fl_mask_put(head, fold->mask, true);
if (!tc_skip_hw(fold->flags))
fl_hw_destroy_filter(tp, fold, NULL);
@@ -1575,6 +1588,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
goto errout_idr;
list_add_tail_rcu(&fnew->list, &fnew->mask->filters);
+ spin_unlock(&tp->lock);
}
*arg = fnew;
@@ -1586,6 +1600,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
errout_idr:
idr_remove(&head->handle_idr, fnew->handle);
errout_hw:
+ spin_unlock(&tp->lock);
if (!tc_skip_hw(fnew->flags))
fl_hw_destroy_filter(tp, fnew, NULL);
errout_mask:
@@ -1688,8 +1703,10 @@ static int fl_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb,
continue;
}
+ spin_lock(&tp->lock);
tc_cls_offload_cnt_update(block, &f->in_hw_count,
&f->flags, add);
+ spin_unlock(&tp->lock);
}
}
@@ -2223,6 +2240,7 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
struct cls_fl_filter *f = fh;
struct nlattr *nest;
struct fl_flow_key *key, *mask;
+ bool skip_hw;
if (!f)
return skb->len;
@@ -2233,21 +2251,26 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
if (!nest)
goto nla_put_failure;
+ spin_lock(&tp->lock);
+
if (f->res.classid &&
nla_put_u32(skb, TCA_FLOWER_CLASSID, f->res.classid))
- goto nla_put_failure;
+ goto nla_put_failure_locked;
key = &f->key;
mask = &f->mask->key;
+ skip_hw = tc_skip_hw(f->flags);
if (fl_dump_key(skb, net, key, mask))
- goto nla_put_failure;
-
- if (!tc_skip_hw(f->flags))
- fl_hw_update_stats(tp, f);
+ goto nla_put_failure_locked;
if (f->flags && nla_put_u32(skb, TCA_FLOWER_FLAGS, f->flags))
- goto nla_put_failure;
+ goto nla_put_failure_locked;
+
+ spin_unlock(&tp->lock);
+
+ if (!skip_hw)
+ fl_hw_update_stats(tp, f);
if (nla_put_u32(skb, TCA_FLOWER_IN_HW_COUNT, f->in_hw_count))
goto nla_put_failure;
@@ -2262,6 +2285,8 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
return skb->len;
+nla_put_failure_locked:
+ spin_unlock(&tp->lock);
nla_put_failure:
nla_nest_cancel(skb, nest);
return -1;
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 06/12] net: sched: flower: handle concurrent mask insertion
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
Without rtnl lock protection masks with same key can be inserted
concurrently. Insert temporary mask with reference count zero to masks
hashtable. This will cause any concurrent modifications to retry.
Wait for rcu grace period to complete after removing temporary mask from
masks hashtable to accommodate concurrent readers.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Suggested-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Fix comment in fl_check_assign_mask().
net/sched/cls_flower.c | 41 ++++++++++++++++++++++++++++++++++-------
1 file changed, 34 insertions(+), 7 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index e98313cd710a..92478bb122d3 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1304,11 +1304,14 @@ static struct fl_flow_mask *fl_create_new_mask(struct cls_fl_head *head,
INIT_LIST_HEAD_RCU(&newmask->filters);
refcount_set(&newmask->refcnt, 1);
- err = rhashtable_insert_fast(&head->ht, &newmask->ht_node,
- mask_ht_params);
+ err = rhashtable_replace_fast(&head->ht, &mask->ht_node,
+ &newmask->ht_node, mask_ht_params);
if (err)
goto errout_destroy;
+ /* Wait until any potential concurrent users of mask are finished */
+ synchronize_rcu();
+
list_add_tail_rcu(&newmask->list, &head->masks);
return newmask;
@@ -1330,19 +1333,36 @@ static int fl_check_assign_mask(struct cls_fl_head *head,
int ret = 0;
rcu_read_lock();
- fnew->mask = rhashtable_lookup_fast(&head->ht, mask, mask_ht_params);
+
+ /* Insert mask as temporary node to prevent concurrent creation of mask
+ * with same key. Any concurrent lookups with same key will return
+ * -EAGAIN because mask's refcnt is zero. It is safe to insert
+ * stack-allocated 'mask' to masks hash table because we call
+ * synchronize_rcu() before returning from this function (either in case
+ * of error or after replacing it with heap-allocated mask in
+ * fl_create_new_mask()).
+ */
+ fnew->mask = rhashtable_lookup_get_insert_fast(&head->ht,
+ &mask->ht_node,
+ mask_ht_params);
if (!fnew->mask) {
rcu_read_unlock();
- if (fold)
- return -EINVAL;
+ if (fold) {
+ ret = -EINVAL;
+ goto errout_cleanup;
+ }
newmask = fl_create_new_mask(head, mask);
- if (IS_ERR(newmask))
- return PTR_ERR(newmask);
+ if (IS_ERR(newmask)) {
+ ret = PTR_ERR(newmask);
+ goto errout_cleanup;
+ }
fnew->mask = newmask;
return 0;
+ } else if (IS_ERR(fnew->mask)) {
+ ret = PTR_ERR(fnew->mask);
} else if (fold && fold->mask != fnew->mask) {
ret = -EINVAL;
} else if (!refcount_inc_not_zero(&fnew->mask->refcnt)) {
@@ -1351,6 +1371,13 @@ static int fl_check_assign_mask(struct cls_fl_head *head,
}
rcu_read_unlock();
return ret;
+
+errout_cleanup:
+ rhashtable_remove_fast(&head->ht, &mask->ht_node,
+ mask_ht_params);
+ /* Wait until any potential concurrent users of mask are finished */
+ synchronize_rcu();
+ return ret;
}
static int fl_set_parms(struct net *net, struct tcf_proto *tp,
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 08/12] net: sched: flower: handle concurrent filter insertion in fl_change
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
Check if user specified a handle and another filter with the same handle
was inserted concurrently. Return EAGAIN to retry filter processing (in
case it is an overwrite request).
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_flower.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index db47828ea5e2..70b357f23391 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1542,6 +1542,15 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
/* user specifies a handle and it doesn't exist */
err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
handle, GFP_ATOMIC);
+
+ /* Filter with specified handle was concurrently
+ * inserted after initial check in cls_api. This is not
+ * necessarily an error if NLM_F_EXCL is not set in
+ * message flags. Returning EAGAIN will cause cls_api to
+ * try to update concurrently inserted rule.
+ */
+ if (err == -ENOSPC)
+ err = -EAGAIN;
} else {
handle = 1;
err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 11/12] net: sched: flower: track rtnl lock state
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
Use 'rtnl_held' flag to track if caller holds rtnl lock. Propagate the flag
to internal functions that need to know rtnl lock state. Take rtnl lock
before calling tcf APIs that require it (hw offload, bind filter, etc.).
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Fix fl_hw_replace_filter() to always release rtnl lock in error
handlers.
net/sched/cls_flower.c | 82 ++++++++++++++++++++++++++++++++++----------------
1 file changed, 56 insertions(+), 26 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 04210d645c78..68bac808cf35 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -374,11 +374,14 @@ static void fl_destroy_filter_work(struct work_struct *work)
}
static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct tc_cls_flower_offload cls_flower = {};
struct tcf_block *block = tp->chain->block;
+ if (!rtnl_held)
+ rtnl_lock();
+
tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, extack);
cls_flower.command = TC_CLSFLOWER_DESTROY;
cls_flower.cookie = (unsigned long) f;
@@ -387,20 +390,28 @@ static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f,
spin_lock(&tp->lock);
tcf_block_offload_dec(block, &f->flags);
spin_unlock(&tp->lock);
+
+ if (!rtnl_held)
+ rtnl_unlock();
}
static int fl_hw_replace_filter(struct tcf_proto *tp,
- struct cls_fl_filter *f,
+ struct cls_fl_filter *f, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct tc_cls_flower_offload cls_flower = {};
struct tcf_block *block = tp->chain->block;
bool skip_sw = tc_skip_sw(f->flags);
- int err;
+ int err = 0;
+
+ if (!rtnl_held)
+ rtnl_lock();
cls_flower.rule = flow_rule_alloc(tcf_exts_num_actions(&f->exts));
- if (!cls_flower.rule)
- return -ENOMEM;
+ if (!cls_flower.rule) {
+ err = -ENOMEM;
+ goto errout;
+ }
tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, extack);
cls_flower.command = TC_CLSFLOWER_REPLACE;
@@ -413,37 +424,48 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
err = tc_setup_flow_action(&cls_flower.rule->action, &f->exts);
if (err) {
kfree(cls_flower.rule);
- if (skip_sw) {
+ if (skip_sw)
NL_SET_ERR_MSG_MOD(extack, "Failed to setup flow action");
- return err;
- }
- return 0;
+ else
+ err = 0;
+ goto errout;
}
err = tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, skip_sw);
kfree(cls_flower.rule);
if (err < 0) {
- fl_hw_destroy_filter(tp, f, NULL);
- return err;
+ fl_hw_destroy_filter(tp, f, true, NULL);
+ goto errout;
} else if (err > 0) {
f->in_hw_count = err;
+ err = 0;
spin_lock(&tp->lock);
tcf_block_offload_inc(block, &f->flags);
spin_unlock(&tp->lock);
}
- if (skip_sw && !(f->flags & TCA_CLS_FLAGS_IN_HW))
- return -EINVAL;
+ if (skip_sw && !(f->flags & TCA_CLS_FLAGS_IN_HW)) {
+ err = -EINVAL;
+ goto errout;
+ }
- return 0;
+errout:
+ if (!rtnl_held)
+ rtnl_unlock();
+
+ return err;
}
-static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f)
+static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
+ bool rtnl_held)
{
struct tc_cls_flower_offload cls_flower = {};
struct tcf_block *block = tp->chain->block;
+ if (!rtnl_held)
+ rtnl_lock();
+
tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, NULL);
cls_flower.command = TC_CLSFLOWER_STATS;
cls_flower.cookie = (unsigned long) f;
@@ -454,6 +476,9 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f)
tcf_exts_stats_update(&f->exts, cls_flower.stats.bytes,
cls_flower.stats.pkts,
cls_flower.stats.lastused);
+
+ if (!rtnl_held)
+ rtnl_unlock();
}
static struct cls_fl_head *fl_head_dereference(struct tcf_proto *tp)
@@ -511,7 +536,8 @@ static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp,
}
static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
- bool *last, struct netlink_ext_ack *extack)
+ bool *last, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct cls_fl_head *head = fl_head_dereference(tp);
bool async = tcf_exts_get_net(&f->exts);
@@ -533,7 +559,7 @@ static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
*last = fl_mask_put(head, f->mask, async);
if (!tc_skip_hw(f->flags))
- fl_hw_destroy_filter(tp, f, extack);
+ fl_hw_destroy_filter(tp, f, rtnl_held, extack);
tcf_unbind_filter(tp, &f->res);
__fl_put(f);
@@ -561,7 +587,7 @@ static void fl_destroy(struct tcf_proto *tp, bool rtnl_held,
list_for_each_entry_safe(mask, next_mask, &head->masks, list) {
list_for_each_entry_safe(f, next, &mask->filters, list) {
- __fl_delete(tp, f, &last, extack);
+ __fl_delete(tp, f, &last, rtnl_held, extack);
if (last)
break;
}
@@ -1401,19 +1427,23 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
struct cls_fl_filter *f, struct fl_flow_mask *mask,
unsigned long base, struct nlattr **tb,
struct nlattr *est, bool ovr,
- struct fl_flow_tmplt *tmplt,
+ struct fl_flow_tmplt *tmplt, bool rtnl_held,
struct netlink_ext_ack *extack)
{
int err;
- err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, true,
+ err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, rtnl_held,
extack);
if (err < 0)
return err;
if (tb[TCA_FLOWER_CLASSID]) {
f->res.classid = nla_get_u32(tb[TCA_FLOWER_CLASSID]);
+ if (!rtnl_held)
+ rtnl_lock();
tcf_bind_filter(tp, &f->res, base);
+ if (!rtnl_held)
+ rtnl_unlock();
}
err = fl_set_key(net, tb, &f->key, &mask->key, extack);
@@ -1492,7 +1522,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
}
err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE], ovr,
- tp->chain->tmplt_priv, extack);
+ tp->chain->tmplt_priv, rtnl_held, extack);
if (err)
goto errout;
@@ -1501,7 +1531,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
goto errout;
if (!tc_skip_hw(fnew->flags)) {
- err = fl_hw_replace_filter(tp, fnew, extack);
+ err = fl_hw_replace_filter(tp, fnew, rtnl_held, extack);
if (err)
goto errout_mask;
}
@@ -1545,7 +1575,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
fl_mask_put(head, fold->mask, true);
if (!tc_skip_hw(fold->flags))
- fl_hw_destroy_filter(tp, fold, NULL);
+ fl_hw_destroy_filter(tp, fold, rtnl_held, NULL);
tcf_unbind_filter(tp, &fold->res);
tcf_exts_get_net(&fold->exts);
/* Caller holds reference to fold, so refcnt is always > 0
@@ -1602,7 +1632,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
errout_hw:
spin_unlock(&tp->lock);
if (!tc_skip_hw(fnew->flags))
- fl_hw_destroy_filter(tp, fnew, NULL);
+ fl_hw_destroy_filter(tp, fnew, rtnl_held, NULL);
errout_mask:
fl_mask_put(head, fnew->mask, true);
errout:
@@ -1626,7 +1656,7 @@ static int fl_delete(struct tcf_proto *tp, void *arg, bool *last,
bool last_on_mask;
int err = 0;
- err = __fl_delete(tp, f, &last_on_mask, extack);
+ err = __fl_delete(tp, f, &last_on_mask, rtnl_held, extack);
*last = list_empty(&head->masks);
__fl_put(f);
@@ -2270,7 +2300,7 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
spin_unlock(&tp->lock);
if (!skip_hw)
- fl_hw_update_stats(tp, f);
+ fl_hw_update_stats(tp, f, rtnl_held);
if (nla_put_u32(skb, TCA_FLOWER_IN_HW_COUNT, f->in_hw_count))
goto nla_put_failure;
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 12/12] net: sched: flower: set unlocked flag for flower proto ops
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
Set TCF_PROTO_OPS_DOIT_UNLOCKED for flower classifier to indicate that its
ops callbacks don't require caller to hold rtnl lock. Don't take rtnl lock
in fl_destroy_filter_work() that is executed on workqueue instead of being
called by cls API and is not affected by setting
TCF_PROTO_OPS_DOIT_UNLOCKED. Rtnl mutex is still manually taken by flower
classifier before calling hardware offloads API that has not been updated
for unlocked execution.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Don't take rtnl lock before calling __fl_destroy_filter() in
workqueue context.
- Extend commit message with explanation why flower still takes rtnl
lock before calling hardware offloads API.
net/sched/cls_flower.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 68bac808cf35..0638f17ac5ab 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -368,9 +368,7 @@ static void fl_destroy_filter_work(struct work_struct *work)
struct cls_fl_filter *f = container_of(to_rcu_work(work),
struct cls_fl_filter, rwork);
- rtnl_lock();
__fl_destroy_filter(f);
- rtnl_unlock();
}
static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f,
@@ -2372,6 +2370,7 @@ static struct tcf_proto_ops cls_fl_ops __read_mostly = {
.tmplt_destroy = fl_tmplt_destroy,
.tmplt_dump = fl_tmplt_dump,
.owner = THIS_MODULE,
+ .flags = TCF_PROTO_OPS_DOIT_UNLOCKED,
};
static int __init cls_fl_init(void)
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 02/12] net: sched: flower: refactor fl_change
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
As a preparation for using classifier spinlock instead of relying on
external rtnl lock, rearrange code in fl_change. The goal is to group the
code which changes classifier state in single block in order to allow
following commits in this set to protect it from parallel modification with
tp->lock. Data structures that require tp->lock protection are mask
hashtable and filters list, and classifier handle_idr.
fl_hw_replace_filter() is a sleeping function and cannot be called while
holding a spinlock. In order to execute all sequence of changes to shared
classifier data structures atomically, call fl_hw_replace_filter() before
modifying them.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Remove redundant check in fl_change error handling code.
- Add empty line between error check and new handle assignment.
net/sched/cls_flower.c | 87 ++++++++++++++++++++++++++------------------------
1 file changed, 45 insertions(+), 42 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 37bced9e2672..d36ceb5001f9 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1358,90 +1358,93 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
if (err < 0)
goto errout;
- if (!handle) {
- handle = 1;
- err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
- INT_MAX, GFP_KERNEL);
- } else if (!fold) {
- /* user specifies a handle and it doesn't exist */
- err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
- handle, GFP_KERNEL);
- }
- if (err)
- goto errout;
- fnew->handle = handle;
-
if (tb[TCA_FLOWER_FLAGS]) {
fnew->flags = nla_get_u32(tb[TCA_FLOWER_FLAGS]);
if (!tc_flags_valid(fnew->flags)) {
err = -EINVAL;
- goto errout_idr;
+ goto errout;
}
}
err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE], ovr,
tp->chain->tmplt_priv, extack);
if (err)
- goto errout_idr;
+ goto errout;
err = fl_check_assign_mask(head, fnew, fold, mask);
if (err)
- goto errout_idr;
-
- if (!fold && __fl_lookup(fnew->mask, &fnew->mkey)) {
- err = -EEXIST;
- goto errout_mask;
- }
-
- err = rhashtable_insert_fast(&fnew->mask->ht, &fnew->ht_node,
- fnew->mask->filter_ht_params);
- if (err)
- goto errout_mask;
+ goto errout;
if (!tc_skip_hw(fnew->flags)) {
err = fl_hw_replace_filter(tp, fnew, extack);
if (err)
- goto errout_mask_ht;
+ goto errout_mask;
}
if (!tc_in_hw(fnew->flags))
fnew->flags |= TCA_CLS_FLAGS_NOT_IN_HW;
if (fold) {
+ fnew->handle = handle;
+
+ err = rhashtable_insert_fast(&fnew->mask->ht, &fnew->ht_node,
+ fnew->mask->filter_ht_params);
+ if (err)
+ goto errout_hw;
+
rhashtable_remove_fast(&fold->mask->ht,
&fold->ht_node,
fold->mask->filter_ht_params);
- if (!tc_skip_hw(fold->flags))
- fl_hw_destroy_filter(tp, fold, NULL);
- }
-
- *arg = fnew;
-
- if (fold) {
idr_replace(&head->handle_idr, fnew, fnew->handle);
list_replace_rcu(&fold->list, &fnew->list);
+
+ if (!tc_skip_hw(fold->flags))
+ fl_hw_destroy_filter(tp, fold, NULL);
tcf_unbind_filter(tp, &fold->res);
tcf_exts_get_net(&fold->exts);
tcf_queue_work(&fold->rwork, fl_destroy_filter_work);
} else {
+ if (__fl_lookup(fnew->mask, &fnew->mkey)) {
+ err = -EEXIST;
+ goto errout_hw;
+ }
+
+ if (handle) {
+ /* user specifies a handle and it doesn't exist */
+ err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
+ handle, GFP_ATOMIC);
+ } else {
+ handle = 1;
+ err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
+ INT_MAX, GFP_ATOMIC);
+ }
+ if (err)
+ goto errout_hw;
+
+ fnew->handle = handle;
+
+ err = rhashtable_insert_fast(&fnew->mask->ht, &fnew->ht_node,
+ fnew->mask->filter_ht_params);
+ if (err)
+ goto errout_idr;
+
list_add_tail_rcu(&fnew->list, &fnew->mask->filters);
}
+ *arg = fnew;
+
kfree(tb);
kfree(mask);
return 0;
-errout_mask_ht:
- rhashtable_remove_fast(&fnew->mask->ht, &fnew->ht_node,
- fnew->mask->filter_ht_params);
-
+errout_idr:
+ idr_remove(&head->handle_idr, fnew->handle);
+errout_hw:
+ if (!tc_skip_hw(fnew->flags))
+ fl_hw_destroy_filter(tp, fnew, NULL);
errout_mask:
fl_mask_put(head, fnew->mask, false);
-
-errout_idr:
- if (!fold)
- idr_remove(&head->handle_idr, fnew->handle);
errout:
tcf_exts_destroy(&fnew->exts);
kfree(fnew);
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 01/12] net: sched: flower: don't check for rtnl on head dereference
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
Flower classifier only changes root pointer during init and destroy. Cls
API implements reference counting for tcf_proto, so there is no danger of
concurrent access to tp when it is being destroyed, even without protection
provided by rtnl lock.
Implement new function fl_head_dereference() to dereference tp->root
without checking for rtnl lock. Use it in all flower function that obtain
head pointer instead of rtnl_dereference().
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Use rcu_dereference_raw() for tp->root dereference.
- Update comment in fl_head_dereference().
net/sched/cls_flower.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 27300a3e76c7..37bced9e2672 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -437,10 +437,20 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f)
cls_flower.stats.lastused);
}
+static struct cls_fl_head *fl_head_dereference(struct tcf_proto *tp)
+{
+ /* Flower classifier only changes root pointer during init and destroy.
+ * Users must obtain reference to tcf_proto instance before calling its
+ * API, so tp->root pointer is protected from concurrent call to
+ * fl_destroy() by reference counting.
+ */
+ return rcu_dereference_raw(tp->root);
+}
+
static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
struct netlink_ext_ack *extack)
{
- struct cls_fl_head *head = rtnl_dereference(tp->root);
+ struct cls_fl_head *head = fl_head_dereference(tp);
bool async = tcf_exts_get_net(&f->exts);
bool last;
@@ -472,7 +482,7 @@ static void fl_destroy_sleepable(struct work_struct *work)
static void fl_destroy(struct tcf_proto *tp, bool rtnl_held,
struct netlink_ext_ack *extack)
{
- struct cls_fl_head *head = rtnl_dereference(tp->root);
+ struct cls_fl_head *head = fl_head_dereference(tp);
struct fl_flow_mask *mask, *next_mask;
struct cls_fl_filter *f, *next;
@@ -490,7 +500,7 @@ static void fl_destroy(struct tcf_proto *tp, bool rtnl_held,
static void *fl_get(struct tcf_proto *tp, u32 handle)
{
- struct cls_fl_head *head = rtnl_dereference(tp->root);
+ struct cls_fl_head *head = fl_head_dereference(tp);
return idr_find(&head->handle_idr, handle);
}
@@ -1308,7 +1318,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
void **arg, bool ovr, bool rtnl_held,
struct netlink_ext_ack *extack)
{
- struct cls_fl_head *head = rtnl_dereference(tp->root);
+ struct cls_fl_head *head = fl_head_dereference(tp);
struct cls_fl_filter *fold = *arg;
struct cls_fl_filter *fnew;
struct fl_flow_mask *mask;
@@ -1445,7 +1455,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
static int fl_delete(struct tcf_proto *tp, void *arg, bool *last,
bool rtnl_held, struct netlink_ext_ack *extack)
{
- struct cls_fl_head *head = rtnl_dereference(tp->root);
+ struct cls_fl_head *head = fl_head_dereference(tp);
struct cls_fl_filter *f = arg;
rhashtable_remove_fast(&f->mask->ht, &f->ht_node,
@@ -1458,7 +1468,7 @@ static int fl_delete(struct tcf_proto *tp, void *arg, bool *last,
static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg,
bool rtnl_held)
{
- struct cls_fl_head *head = rtnl_dereference(tp->root);
+ struct cls_fl_head *head = fl_head_dereference(tp);
struct cls_fl_filter *f;
arg->count = arg->skip;
@@ -1477,7 +1487,7 @@ static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg,
static int fl_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb,
void *cb_priv, struct netlink_ext_ack *extack)
{
- struct cls_fl_head *head = rtnl_dereference(tp->root);
+ struct cls_fl_head *head = fl_head_dereference(tp);
struct tc_cls_flower_offload cls_flower = {};
struct tcf_block *block = tp->chain->block;
struct fl_flow_mask *mask;
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 04/12] net: sched: flower: track filter deletion with flag
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com>
In order to prevent double deletion of filter by concurrent tasks when rtnl
lock is not used for synchronization, add 'deleted' filter field. Check
value of this field when modifying filters and return error if concurrent
deletion is detected.
Refactor __fl_delete() to accept pointer to 'last' boolean as argument,
and return error code as function return value instead. This is necessary
to signal concurrent filter delete to caller.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Refactor __fl_delete() to improve readability.
net/sched/cls_flower.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 9ed7c9b804a7..dd8a65cef6e1 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -110,6 +110,7 @@ struct cls_fl_filter {
* synchronization. Use atomic reference counter to be concurrency-safe.
*/
refcount_t refcnt;
+ bool deleted;
};
static const struct rhashtable_params mask_ht_params = {
@@ -458,6 +459,8 @@ static void __fl_put(struct cls_fl_filter *f)
if (!refcount_dec_and_test(&f->refcnt))
return;
+ WARN_ON(!f->deleted);
+
if (tcf_exts_get_net(&f->exts))
tcf_queue_work(&f->rwork, fl_destroy_filter_work);
else
@@ -495,22 +498,29 @@ static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp,
return f;
}
-static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
- struct netlink_ext_ack *extack)
+static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
+ bool *last, struct netlink_ext_ack *extack)
{
struct cls_fl_head *head = fl_head_dereference(tp);
bool async = tcf_exts_get_net(&f->exts);
- bool last;
+ *last = false;
+
+ if (f->deleted)
+ return -ENOENT;
+
+ f->deleted = true;
+ rhashtable_remove_fast(&f->mask->ht, &f->ht_node,
+ f->mask->filter_ht_params);
idr_remove(&head->handle_idr, f->handle);
list_del_rcu(&f->list);
- last = fl_mask_put(head, f->mask, async);
+ *last = fl_mask_put(head, f->mask, async);
if (!tc_skip_hw(f->flags))
fl_hw_destroy_filter(tp, f, extack);
tcf_unbind_filter(tp, &f->res);
__fl_put(f);
- return last;
+ return 0;
}
static void fl_destroy_sleepable(struct work_struct *work)
@@ -530,10 +540,12 @@ static void fl_destroy(struct tcf_proto *tp, bool rtnl_held,
struct cls_fl_head *head = fl_head_dereference(tp);
struct fl_flow_mask *mask, *next_mask;
struct cls_fl_filter *f, *next;
+ bool last;
list_for_each_entry_safe(mask, next_mask, &head->masks, list) {
list_for_each_entry_safe(f, next, &mask->filters, list) {
- if (__fl_delete(tp, f, extack))
+ __fl_delete(tp, f, &last, extack);
+ if (last)
break;
}
}
@@ -1444,6 +1456,12 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
refcount_inc(&fnew->refcnt);
if (fold) {
+ /* Fold filter was deleted concurrently. Retry lookup. */
+ if (fold->deleted) {
+ err = -EAGAIN;
+ goto errout_hw;
+ }
+
fnew->handle = handle;
err = rhashtable_insert_fast(&fnew->mask->ht, &fnew->ht_node,
@@ -1456,6 +1474,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
fold->mask->filter_ht_params);
idr_replace(&head->handle_idr, fnew, fnew->handle);
list_replace_rcu(&fold->list, &fnew->list);
+ fold->deleted = true;
if (!tc_skip_hw(fold->flags))
fl_hw_destroy_filter(tp, fold, NULL);
@@ -1525,14 +1544,14 @@ static int fl_delete(struct tcf_proto *tp, void *arg, bool *last,
{
struct cls_fl_head *head = fl_head_dereference(tp);
struct cls_fl_filter *f = arg;
+ bool last_on_mask;
+ int err = 0;
- rhashtable_remove_fast(&f->mask->ht, &f->ht_node,
- f->mask->filter_ht_params);
- __fl_delete(tp, f, extack);
+ err = __fl_delete(tp, f, &last_on_mask, extack);
*last = list_empty(&head->masks);
__fl_put(f);
- return 0;
+ return err;
}
static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg,
--
2.13.6
^ permalink raw reply related
* [PATCH net-next v2 00/12] Refactor flower classifier to remove dependency on rtnl lock
From: Vlad Buslov @ 2019-02-27 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, sbrivio, Vlad Buslov
Currently, all netlink protocol handlers for updating rules, actions and
qdiscs are protected with single global rtnl lock which removes any
possibility for parallelism. This patch set is a third step to remove
rtnl lock dependency from TC rules update path.
Recently, new rtnl registration flag RTNL_FLAG_DOIT_UNLOCKED was added.
TC rule update handlers (RTM_NEWTFILTER, RTM_DELTFILTER, etc.) are
already registered with this flag and only take rtnl lock when qdisc or
classifier requires it. Classifiers can indicate that their ops
callbacks don't require caller to hold rtnl lock by setting the
TCF_PROTO_OPS_DOIT_UNLOCKED flag. The goal of this change is to refactor
flower classifier to support unlocked execution and register it with
unlocked flag.
This patch set implements following changes to make flower classifier
concurrency-safe:
- Implement reference counting for individual filters. Change fl_get to
take reference to filter. Implement tp->ops->put callback that was
introduced in cls API patch set to release reference to flower filter.
- Use tp->lock spinlock to protect internal classifier data structures
from concurrent modification.
- Handle concurrent tcf proto deletion by returning EAGAIN, which will
cause cls API to retry and create new proto instance or return error
to the user (depending on message type).
- Handle concurrent insertion of filter with same priority and handle by
returning EAGAIN, which will cause cls API to lookup filter again and
process it accordingly to netlink message flags.
- Extend flower mask with reference counting and protect masks list with
masks_lock spinlock.
- Prevent concurrent mask insertion by inserting temporary value to
masks hash table. This is necessary because mask initialization is a
sleeping operation and cannot be done while holding tp->lock.
Both chain level and classifier level conflicts are resolved by
returning -EAGAIN to cls API that results restart of whole operation.
This retry mechanism is a result of fine-grained locking approach used
in this and previous changes in series and is necessary to allow
concurrent updates on same chain instance. Alternative approach would be
to lock the whole chain while updating filters on any of child tp's,
adding and removing classifier instances from the chain. However, since
most CPU-intensive parts of filter update code are specifically in
classifier code and its dependencies (extensions and hw offloads), such
approach would negate most of the gains introduced by this change and
previous changes in the series when updating same chain instance.
Tcf hw offloads API is not changed by this patch set and still requires
caller to hold rtnl lock. Refactored flower classifier tracks rtnl lock
state by means of 'rtnl_held' flag provided by cls API and obtains the
lock before calling hw offloads. Following patch set will lift this
restriction and refactor cls hw offloads API to support unlocked
execution.
With these changes flower classifier is safely registered with
TCF_PROTO_OPS_DOIT_UNLOCKED flag in last patch.
Changes from V1 to V2:
- Extend cover letter with explanation about retry mechanism.
- Rebase on current net-next.
- Patch 1:
- Use rcu_dereference_raw() for tp->root dereference.
- Update comment in fl_head_dereference().
- Patch 2:
- Remove redundant check in fl_change error handling code.
- Add empty line between error check and new handle assignment.
- Patch 3:
- Refactor loop in fl_get_next_filter() to improve readability.
- Patch 4:
- Refactor __fl_delete() to improve readability.
- Patch 6:
- Fix comment in fl_check_assign_mask().
- Patch 9:
- Extend commit message.
- Fix error code in comment.
- Patch 11:
- Fix fl_hw_replace_filter() to always release rtnl lock in error
handlers.
- Patch 12:
- Don't take rtnl lock before calling __fl_destroy_filter() in
workqueue context.
- Extend commit message with explanation why flower still takes rtnl
lock before calling hardware offloads API.
Github: [https://github.com/vbuslov/linux/tree/unlocked_flower_cong2]
Vlad Buslov (12):
net: sched: flower: don't check for rtnl on head dereference
net: sched: flower: refactor fl_change
net: sched: flower: introduce reference counting for filters
net: sched: flower: track filter deletion with flag
net: sched: flower: add reference counter to flower mask
net: sched: flower: handle concurrent mask insertion
net: sched: flower: protect masks list with spinlock
net: sched: flower: handle concurrent filter insertion in fl_change
net: sched: flower: handle concurrent tcf proto deletion
net: sched: flower: protect flower classifier state with spinlock
net: sched: flower: track rtnl lock state
net: sched: flower: set unlocked flag for flower proto ops
net/sched/cls_flower.c | 440 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 329 insertions(+), 111 deletions(-)
--
2.13.6
^ permalink raw reply
* RE: [PATCH net-next] net: sched: pie: fix 64-bit division
From: David Laight @ 2019-02-27 10:11 UTC (permalink / raw)
To: 'Leslie Monis', davem@davemloft.net; +Cc: netdev@vger.kernel.org
In-Reply-To: <20190227010006.22219-1-lesliemonis@gmail.com>
From: Leslie Monis
> Sent: 27 February 2019 01:00
> Use div_u64() to resolve build failures on 32-bit platforms.
>
> Fixes: 3f7ae5f3dc52 ("net: sched: pie: add more cases to auto-tune alpha and beta")
> Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
> ---
> net/sched/sch_pie.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
> index 4c0670b6aec1..f93cfe034c72 100644
> --- a/net/sched/sch_pie.c
> +++ b/net/sched/sch_pie.c
> @@ -429,7 +429,7 @@ static void calculate_probability(struct Qdisc *sch)
> */
>
> if (qdelay == 0 && qdelay_old == 0 && update_prob)
> - q->vars.prob = (q->vars.prob * 98) / 100;
> + q->vars.prob = 98 * div_u64(q->vars.prob, 100);
This has significantly different rounding after the change.
The result for small values is very different.
The alterative:
q->vars.prob -= div_u64(q->vars.prob, 50);
is much nearer to the original - but never goes to zero.
If the 98% decay factor isn't critical then you could remove
1/64th or 1/32nd + 1/16th to avoid the slow division.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-27 10:02 UTC (permalink / raw)
To: Brian Norris
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, David S. Miller, devicetree, linux-arm-kernel,
linux-kernel, linux-rockchip, linux-wireless, netdev, linux-pm,
Jeffy Chen, Rafael J. Wysocki, Tony Lindgren, Lorenzo Pieralisi
In-Reply-To: <20190226232822.GA174696@google.com>
+ Lorenzo
Hi Brian,
On 26/02/2019 23:28, Brian Norris wrote:
> + others
>
> Hi Marc,
>
> Thanks for the series. I have a few bits of history to add to this, and
> some comments.
>
> On Sun, Feb 24, 2019 at 02:04:22PM +0000, Marc Zyngier wrote:
>> For quite some time, I wondered why the PCI mwifiex device built in my
>> Chromebook was unable to use the good old legacy interrupts. But as MSIs
>> were working fine, I never really bothered investigating. I finally had a
>> look, and the result isn't very pretty.
>>
>> On this machine (rk3399-based kevin), the wake-up interrupt is described as
>> such:
>>
>> &pci_rootport {
>> mvl_wifi: wifi@0,0 {
>> compatible = "pci1b4b,2b42";
>> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
>> interrupt-parent = <&gpio0>;
>> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&wlan_host_wake_l>;
>> wakeup-source;
>> };
>> };
>>
>> Note how the interrupt is part of the properties directly attached to the
>> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
>> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
>> altogether (Yay for the broken design!). This is in total violation of the
>> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
>> specifiers describe the PCI device interrupts, and must obey the
>> INT-{A,B,C,D} mapping. Oops!
>
> You're not the first person to notice this. All the motivations are not
> necessarily painted clearly in their cover letter, but here are some
> previous attempts at solving this problem:
>
> [RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
> https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.chen@rock-chips.com/
> http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.chen@rock-chips.com/
>
> As you can see by the 12th iteration, it wasn't left unsolved for lack
> of trying...
I wasn't aware of this. That's definitely a better approach than my
hack, and I would really like this to be revived.
>
> Frankly, if a proper DT replacement to the admittedly bad binding isn't
> agreed upon quickly, I'd be very happy to just have WAKE# support
> removed from the DTS for now, and the existing mwifiex binding should
> just be removed. (Wake-on-WiFi was never properly vetted on these
> platforms anyway.) It mostly serves to just cause problems like you've
> noticed.
Agreed. If there is no actual use for this, and that we can build a case
for a better solution, let's remove the wakeup support from the Gru DT
(it is invalid anyway), and bring it back if and when we get the right
level of support.
[...]
> One problem Rockchip authors were also trying to resolve here is that
> PCIe WAKE# handling should not really be something the PCI device driver
> has to handle directly. Despite your complaints about not using in-band
> TLP wakeup, a separate WAKE# pin is in fact a documented part of the
> PCIe standard, and it so happens that the Rockchip RC does not support
> handling TLPs in S3, if you want to have decent power consumption. (Your
> "bad hardware" complaints could justifiably fall here, I suppose.)
>
> Additionally, I've had pushback from PCI driver authors/maintainers on
> adding more special handling for this sort of interrupt property (not
> the binding specifically, but just the concept of handling WAKE# in the
> driver), as they claim this should be handled by the system firmware,
> when they set the appropriate wakeup flags, which filter down to
> __pci_enable_wake() -> platform_pci_set_wakeup(). That's how x86 systems
> do it (note: I know for a fact that many have a very similar
> architecture -- WAKE# is not routed to the RC, because, why does it need
> to? and they *don't* use TLP wakeup either -- they just hide it in
> firmware better), and it Just Works.
Even on an arm64 platform, there is no reason why a wakeup interrupt
couldn't be handled by FW rather than the OS. It just need to be wired
to the right spot (so that it generates a secure interrupt that can be
handled by FW).
> So, we basically concluded that we should standardize on a way to
> describe WAKE# interrupts such that PCI drivers don't have to deal with
> it at all, and the PCI core can do it for us. 12 revisions later
> and...we still never agreed on a good device tree binding for this.
Is the DT binding the only problem? Do we have an agreement for the core
code?
> IOW, maybe your wake-up sub-node is the best way to side-step the
> problems of conflicting with the OF PCI spec. But I'd still really like
> to avoid parsing it in mwifiex, if at all possible.
Honestly, my solution is just a terrible hack. I wasn't aware that this
was a more general problem, and I'd love it to be addressed in the core
PCI code.
> (We'd still be left with the marvell,wakeup-pin propery to parse
> specifically in mwifiex, which sadly has to exist because....well,
> Samsung decided to do chip-on-board, and then they failed to use the
> correct pin on Marvell's side when wiring up WAKE#. Sigh.)
Oh well...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-27 9:27 UTC (permalink / raw)
To: Brian Norris
Cc: Ard Biesheuvel, Amitkumar Karwar, Enric Balletbo i Serra,
Ganapathi Bhat, Heiko Stuebner, Kalle Valo, Nishant Sarmukadam,
Rob Herring, Xinming Hu, Devicetree List,
<netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel, linux-pci,
linux-pm, Jeffy Chen
In-Reply-To: <20190226234422.GD174696@google.com>
On 26/02/2019 23:44, Brian Norris wrote:
> Hi,
>
> On Tue, Feb 26, 2019 at 05:14:00PM +0000, Marc Zyngier wrote:
>> On 26/02/2019 16:21, Ard Biesheuvel wrote:
>>> On Mon, 25 Feb 2019 at 15:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> It outlines one thing: If you have to interpret per-device PCI
>>>> properties from DT, you're in for serious trouble. I should get some
>>>> better HW.
>>>>
>>>
>>> Yeah, it obviously makes no sense at all for the interrupt parent of a
>>> PCI device to deviate from the host bridge's interrupt parent, and
>>> it's quite unfortunate that we can't simply ban it now that the cat is
>>> out of the bag already.
>>>
>>> Arguably, the wake up widget is not part of the PCI device, but I have
>>> no opinion as to whether it is better modeling it as a sub device as
>>> you are proposing or as an entirely separate device referenced via a
>>> phandle.
>>
>> It is not that clear. The widget seems to be an integral part of the
>> device, as it is the same basic IP that is used for SDIO and USB.
>
> It's not really a widget specific to this IP. It's just a GPIO. It so
> happens that both SDIO and PCIe designs have wanted to use a GPIO for
> wakeup, as many other devices do. (Note: it's not just cheap ARM
> devices; pulling up some Intel Chromebook designs, I see the exact same
> WAKE# GPIO on their PCIe WiFi as well.)
Arghh! If I can't even point people to an Intel design as an example of
something done halfway right, we're screwed! ;-)
>
>> It looks like the good old pre-PCI-2.2 days, where you had to have a
>> separate cable between your network card and the base-board for the
>> wake-up interrupt to be delivered. Starting with PCI-2.2, the bus can
>> carry the signal just fine. With PCIe, it should just be an interrupt
>> TLP sent to the RC, but that's obviously not within the capabilities of
>> the HW.
>
> You should search the PCI Express specification for WAKE#. There is a
> clearly-documented "side-band wake" feature that is part of the
> standard, as an alternative to in-band TLP wakeup. While you claim this
> is an ancient thing, it in fact still in use on many systems -- it's
> just usually abstracted better by ACPI firmware, whereas the dirty
> laundry is aired a bit more on a Device Tree system. And we got it
> wrong.
I stand corrected. I was really hoping for these side-band wires to be a
thing of the past, but clearly the world hasn't moved as quickly as I hoped.
>> Anyway, it'd be good if the Marvell people could chime in and let us
>> know how they'd prefer to handle this.
>
> I'm not sure this is really a Marvell-specific problem. (Well, except
> for the marvell,wakeup-pin silliness, which is somewhat orthogonal.) In
> fact, if we cared a little more about Wake-on-WiFi, we'd be trying to
> support the same (out-of-band WAKE#) with other WiFi drivers.
I'd definitely like to see this standardized. It would give us more
coverage, and prevent everyone from doing their own thing. But as you
mention, WoW doesn't seem to get much traction.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH net] selftests: fixes for UDP GRO
From: Paolo Abeni @ 2019-02-27 9:26 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, David S. Miller, Willem de Bruijn
In-Reply-To: <CAF=yD-LeXUVU26iX-di5vfdDvrfuuAcJmyGGpUGN5FHX0fi0Wg@mail.gmail.com>
Hi,
On Tue, 2019-02-26 at 13:38 -0500, Willem de Bruijn wrote:
> On Tue, Feb 26, 2019 at 9:28 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > The current implementation for UDP GRO tests is racy: the receiver
> > may flush the RX queue while the sending is still transmitting and
> > incorrectly report RX errors, with a wrong number of packet received.
> >
> > Add explicit timeouts to the receiver for both connection activation
> > (first packet received for UDP) and reception completion, so that
> > in the above critical scenario the receiver will wait for the
> > transfer completion.
> >
> > Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>
> Acked-by: Willem de Bruijn <willemb@google.com>
Thanks for reviewing.
> ---
>
> This is because of that "force termination after the second poll()"?
Yes, exactly.
> Could perhaps also just extend do_recv to wait while (!interrupted &&
> tnow < tstart + 2000) and avoid the explicit arguments.
I thought about that, but then UDP GRO self-tests would take a bit more
and the binary helper would be less re-usable for future test cases.
Cheers,
Paolo
^ permalink raw reply
* Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read
From: Harini Katakam @ 2019-02-27 9:05 UTC (permalink / raw)
To: Michal Simek
Cc: Paul Kocialkowski, Andrew Lunn, Florian Fainelli, netdev,
linux-arm-kernel, linux-kernel, David S . Miller,
Thomas Petazzoni, Heiner Kallweit
In-Reply-To: <8bb813fb-102b-00c9-fb6f-a3e928965051@xilinx.com>
Hi Andrew, Paul,
On Wed, Feb 27, 2019 at 2:15 PM Michal Simek <michal.simek@xilinx.com> wrote:
>
> On 21. 02. 19 12:03, Michal Simek wrote:
> > On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> >> Hi,
> >>
> >> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> >>> Hi,
> >>>
> >>> On 19. 02. 19 18:25, Andrew Lunn wrote:
> >>>>> Thanks for the suggestion! So I had a closer look at that driver to try
> >>>>> and see what could go wrong and it looks like I found a few things
> >>>>> there.
> >>>>
> >>>> Hi Paul
> >>>>
> >>>> Yes, this driver has issues. If i remember correctly, it got merged
> >>>> while i was on vacation. I pointed out a few issues, but the authors
> >>>> never responded. Feel free to fix it up.
> >>>
Sorry for this - I've synced up with the author and got the comments from the
time this driver was upstreamed. I'll try to address those and Paul's
suggestions
going forward.
> >>> Will be good to know who was that person.
> >>>
> >>> I can't do much this week with this because responsible person for this
> >>> driver is out of office this week. That's why please give us some time
> >>> to get back to this.
> >>
> >> Understood. I think we need to start a discussion about how the general
> >> design of this driver can be improved.
> >>
> >> In particular, I wonder if it could work better to make this driver a
> >> PHY driver that just redirects all its ops to the actual PHY driver,
> >> except for read_status where it should also add some code.
Thanks, I'm looking into this option and also a way to expose the correct
interface mode setting as you mentioned below. I'll get back before the
end of the week. Please do let me know if you have any further suggestions.
Regards,
Harini
> >
> > I didn't take a look at Linux driver but it should work in a way that it
> > checks description (more below) and then wait for attached phy to do its
> > work and on the way back just setup this bridge based on that.
> >
> >> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
> >> this way. Currently, the PHY mode has to be set to GMII for the MAC to
> >> be configured correctly, but the PHY also gets this information while
> >> it should be told that RGMII is in use. This doesn't seem to play a big
> >> role in PHY configuration though, but it's still inadequate.
> >>
> >> What do you think?
<snip>
^ permalink raw reply
* [PATCH net] bnxt_en: Drop oversize TX packets to prevent errors.
From: Michael Chan @ 2019-02-27 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev
There have been reports of oversize UDP packets being sent to the
driver to be transmitted, causing error conditions. The issue is
likely caused by the dst of the SKB switching between 'lo' with
64K MTU and the hardware device with a smaller MTU. Patches are
being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
issue.
In the meantime, add a quick length check in the driver to prevent
the error. The driver uses the TX packet size as index to look up an
array to setup the TX BD. The array is large enough to support all MTU
sizes supported by the driver. The oversize TX packet causes the
driver to index beyond the array and put garbage values into the
TX BD. Add a simple check to prevent this.
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
David, I think this should be queued for stable as well.
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d95730c..803f799 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -500,6 +500,12 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
}
length >>= 9;
+ if (unlikely(length >= ARRAY_SIZE(bnxt_lhint_arr))) {
+ dev_warn_ratelimited(&pdev->dev, "Dropped oversize %d bytes TX packet.\n",
+ skb->len);
+ i = 0;
+ goto tx_dma_error;
+ }
flags |= bnxt_lhint_arr[length];
txbd->tx_bd_len_flags_type = cpu_to_le32(flags);
--
2.5.1
^ permalink raw reply related
* Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read
From: Michal Simek @ 2019-02-27 8:43 UTC (permalink / raw)
To: Michal Simek, Paul Kocialkowski, Andrew Lunn, Harini Katakam
Cc: Florian Fainelli, netdev, linux-arm-kernel, linux-kernel,
David S . Miller, Thomas Petazzoni, Heiner Kallweit
In-Reply-To: <9cb2f7a8-a8cf-ef80-d260-cc67c072b5c5@xilinx.com>
On 21. 02. 19 12:03, Michal Simek wrote:
> On 21. 02. 19 11:24, Paul Kocialkowski wrote:
>> Hi,
>>
>> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
>>> Hi,
>>>
>>> On 19. 02. 19 18:25, Andrew Lunn wrote:
>>>>> Thanks for the suggestion! So I had a closer look at that driver to try
>>>>> and see what could go wrong and it looks like I found a few things
>>>>> there.
>>>>
>>>> Hi Paul
>>>>
>>>> Yes, this driver has issues. If i remember correctly, it got merged
>>>> while i was on vacation. I pointed out a few issues, but the authors
>>>> never responded. Feel free to fix it up.
>>>
>>> Will be good to know who was that person.
>>>
>>> I can't do much this week with this because responsible person for this
>>> driver is out of office this week. That's why please give us some time
>>> to get back to this.
>>
>> Understood. I think we need to start a discussion about how the general
>> design of this driver can be improved.
>>
>> In particular, I wonder if it could work better to make this driver a
>> PHY driver that just redirects all its ops to the actual PHY driver,
>> except for read_status where it should also add some code.
>
> I didn't take a look at Linux driver but it should work in a way that it
> checks description (more below) and then wait for attached phy to do its
> work and on the way back just setup this bridge based on that.
>
>> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
>> this way. Currently, the PHY mode has to be set to GMII for the MAC to
>> be configured correctly, but the PHY also gets this information while
>> it should be told that RGMII is in use. This doesn't seem to play a big
>> role in PHY configuration though, but it's still inadequate.
>>
>> What do you think?
>
> I stop the driver to be applied to u-boot because exactly this
> gmii/rgmii configuration. There is a need that mac is configured to gmii
> and this needs to be checked but to phy rgmii needs to be exposed.
> I was trying to find out hardware design and board where I can do some
> experiments but didn't find out. That's why I need to wait for
> colleagues to point me to that.
Harini: Can you please respond this thread?
Thanks,
Michal
^ permalink raw reply
* Re: Handling an Extra Signal at PHY Reset
From: Paul Kocialkowski @ 2019-02-27 8:19 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, Heiner Kallweit, netdev, Thomas Petazzoni,
Mylène Josserand
In-Reply-To: <20190221140458.GF5653@lunn.ch>
Hi Andrew,
On Thu, 2019-02-21 at 15:04 +0100, Andrew Lunn wrote:
> > I can also confirm that it does not prevent contacting the PHY on the
> > MDIO bus, contrary to what I have stated previously.
>
> O.K, so wrong voltage does not matter, you can still probe the PHY.
>
> Ignore the switch. Use a pin hog to set the GPIO to the disabled
> state. And set the voltage using the PHY register during probe().
Thanks a lot for your suggestion! I have tried it out and it appears to
work just as well as before.
Although the CONFIG pin state is still sampled at PHY reset, having it
connected to the PTP clock instead of the LED pin does not seem to
change the address we're getting.
We'll stick with that solution, since it's the least invasive one.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH v2 net] ipv4: Add ICMPv6 support when parse route ipproto
From: Hangbin Liu @ 2019-02-27 8:15 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, Roopa Prabhu, davem, Sabrina Dubroca, Hangbin Liu
In-Reply-To: <20190225074700.7316-1-liuhangbin@gmail.com>
For ip rules, we need to use 'ipproto ipv6-icmp' to match ICMPv6 headers.
But for ip -6 route, currently we only support tcp, udp and icmp.
Add ICMPv6 support so we can match ipv6-icmp rules for route lookup.
v2: As David Ahern and Sabrina Dubroca suggested, Add an argument to
rtm_getroute_parse_ip_proto() to handle ICMP/ICMPv6 with different family.
Reported-by: Jianlin Shi <jishi@redhat.com>
Fixes: eacb9384a3fe ("ipv6: support sport, dport and ip_proto in RTM_GETROUTE")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
include/net/ip.h | 2 +-
net/ipv4/netlink.c | 17 +++++++++++++----
net/ipv4/route.c | 2 +-
net/ipv6/route.c | 3 ++-
4 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/include/net/ip.h b/include/net/ip.h
index 8866bfce6121..80300e8a6280 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -716,7 +716,7 @@ extern int sysctl_icmp_msgs_burst;
int ip_misc_proc_init(void);
#endif
-int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
+int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto, u8 family,
struct netlink_ext_ack *extack);
#endif /* _IP_H */
diff --git a/net/ipv4/netlink.c b/net/ipv4/netlink.c
index f86bb4f06609..d8e3a1fb8e82 100644
--- a/net/ipv4/netlink.c
+++ b/net/ipv4/netlink.c
@@ -3,9 +3,10 @@
#include <linux/types.h>
#include <net/net_namespace.h>
#include <net/netlink.h>
+#include <linux/in6.h>
#include <net/ip.h>
-int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
+int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto, u8 family,
struct netlink_ext_ack *extack)
{
*ip_proto = nla_get_u8(attr);
@@ -13,11 +14,19 @@ int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
switch (*ip_proto) {
case IPPROTO_TCP:
case IPPROTO_UDP:
+ return 0;
case IPPROTO_ICMP:
+ if (family != AF_INET)
+ break;
+ return 0;
+#if IS_ENABLED(CONFIG_IPV6)
+ case IPPROTO_ICMPV6:
+ if (family != AF_INET6)
+ break;
return 0;
- default:
- NL_SET_ERR_MSG(extack, "Unsupported ip proto");
- return -EOPNOTSUPP;
+#endif
}
+ NL_SET_ERR_MSG(extack, "Unsupported ip proto");
+ return -EOPNOTSUPP;
}
EXPORT_SYMBOL_GPL(rtm_getroute_parse_ip_proto);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 5163b64f8fb3..7bb9128c8363 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2803,7 +2803,7 @@ static int inet_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
if (tb[RTA_IP_PROTO]) {
err = rtm_getroute_parse_ip_proto(tb[RTA_IP_PROTO],
- &ip_proto, extack);
+ &ip_proto, AF_INET, extack);
if (err)
return err;
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ce15dc4ccbfa..6f949aa5b64f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4889,7 +4889,8 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
if (tb[RTA_IP_PROTO]) {
err = rtm_getroute_parse_ip_proto(tb[RTA_IP_PROTO],
- &fl6.flowi6_proto, extack);
+ &fl6.flowi6_proto, AF_INET6,
+ extack);
if (err)
goto errout;
}
--
2.19.2
^ permalink raw reply related
* Re: [PATCH net-next v2] net: sched: act_tunnel_key: fix metadata handling
From: Roi Dayan @ 2019-02-27 8:14 UTC (permalink / raw)
To: Vlad Buslov, netdev@vger.kernel.org, dcaratti@redhat.com
Cc: jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us,
davem@davemloft.net, wenxu@ucloud.cn
In-Reply-To: <20190225153014.8885-1-vladbu@mellanox.com>
On 25/02/2019 17:30, Vlad Buslov wrote:
> Tunnel key action params->tcft_enc_metadata is only set when action is
> TCA_TUNNEL_KEY_ACT_SET. However, metadata pointer is incorrectly
> dereferenced during tunnel key init and release without verifying that
> action is if correct type, which causes NULL pointer dereference. Metadata
> tunnel dst_cache is also leaked on action overwrite.
>
> Fix metadata handling:
> - Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
> - Move dst_cache destroy code into tunnel_key_release_params() function
> that is called in both action overwrite and release cases (fixes resource
> leak) and verifies that actions has correct type before dereferencing
> metadata pointer (fixes NULL pointer dereference).
>
> Oops with KASAN enabled during tdc tests execution:
>
> [ 261.080482] ==================================================================
> [ 261.088049] BUG: KASAN: null-ptr-deref in dst_cache_destroy+0x21/0xa0
> [ 261.094613] Read of size 8 at addr 00000000000000b0 by task tc/2976
> [ 261.102524] CPU: 14 PID: 2976 Comm: tc Not tainted 5.0.0-rc7+ #157
> [ 261.108844] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
> [ 261.116726] Call Trace:
> [ 261.119234] dump_stack+0x9a/0xeb
> [ 261.122625] ? dst_cache_destroy+0x21/0xa0
> [ 261.126818] ? dst_cache_destroy+0x21/0xa0
> [ 261.131004] kasan_report+0x176/0x192
> [ 261.134752] ? idr_get_next+0xd0/0x120
> [ 261.138578] ? dst_cache_destroy+0x21/0xa0
> [ 261.142768] dst_cache_destroy+0x21/0xa0
> [ 261.146799] tunnel_key_release+0x3a/0x50 [act_tunnel_key]
> [ 261.152392] tcf_action_cleanup+0x2c/0xc0
> [ 261.156490] tcf_generic_walker+0x4c2/0x5c0
> [ 261.160794] ? tcf_action_dump_1+0x390/0x390
> [ 261.165163] ? tunnel_key_walker+0x5/0x1a0 [act_tunnel_key]
> [ 261.170865] ? tunnel_key_walker+0xe9/0x1a0 [act_tunnel_key]
> [ 261.176641] tca_action_gd+0x600/0xa40
> [ 261.180482] ? tca_get_fill.constprop.17+0x200/0x200
> [ 261.185548] ? __lock_acquire+0x588/0x1d20
> [ 261.189741] ? __lock_acquire+0x588/0x1d20
> [ 261.193922] ? mark_held_locks+0x90/0x90
> [ 261.197944] ? mark_held_locks+0x90/0x90
> [ 261.202018] ? __nla_parse+0xfe/0x190
> [ 261.205774] tc_ctl_action+0x218/0x230
> [ 261.209614] ? tcf_action_add+0x230/0x230
> [ 261.213726] rtnetlink_rcv_msg+0x3a5/0x600
> [ 261.217910] ? lock_downgrade+0x2d0/0x2d0
> [ 261.222006] ? validate_linkmsg+0x400/0x400
> [ 261.226278] ? find_held_lock+0x6d/0xd0
> [ 261.230200] ? match_held_lock+0x1b/0x210
> [ 261.234296] ? validate_linkmsg+0x400/0x400
> [ 261.238567] netlink_rcv_skb+0xc7/0x1f0
> [ 261.242489] ? netlink_ack+0x470/0x470
> [ 261.246319] ? netlink_deliver_tap+0x1f3/0x5a0
> [ 261.250874] netlink_unicast+0x2ae/0x350
> [ 261.254884] ? netlink_attachskb+0x340/0x340
> [ 261.261647] ? _copy_from_iter_full+0xdd/0x380
> [ 261.268576] ? __virt_addr_valid+0xb6/0xf0
> [ 261.275227] ? __check_object_size+0x159/0x240
> [ 261.282184] netlink_sendmsg+0x4d3/0x630
> [ 261.288572] ? netlink_unicast+0x350/0x350
> [ 261.295132] ? netlink_unicast+0x350/0x350
> [ 261.301608] sock_sendmsg+0x6d/0x80
> [ 261.307467] ___sys_sendmsg+0x48e/0x540
> [ 261.313633] ? copy_msghdr_from_user+0x210/0x210
> [ 261.320545] ? save_stack+0x89/0xb0
> [ 261.326289] ? __lock_acquire+0x588/0x1d20
> [ 261.332605] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.340063] ? mark_held_locks+0x90/0x90
> [ 261.346162] ? do_filp_open+0x138/0x1d0
> [ 261.352108] ? may_open_dev+0x50/0x50
> [ 261.357897] ? match_held_lock+0x1b/0x210
> [ 261.364016] ? __fget_light+0xa6/0xe0
> [ 261.369840] ? __sys_sendmsg+0xd2/0x150
> [ 261.375814] __sys_sendmsg+0xd2/0x150
> [ 261.381610] ? __ia32_sys_shutdown+0x30/0x30
> [ 261.388026] ? lock_downgrade+0x2d0/0x2d0
> [ 261.394182] ? mark_held_locks+0x1c/0x90
> [ 261.400230] ? do_syscall_64+0x1e/0x280
> [ 261.406172] do_syscall_64+0x78/0x280
> [ 261.411932] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.419103] RIP: 0033:0x7f28e91a8b87
> [ 261.424791] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53 48 89 f3 48
> [ 261.448226] RSP: 002b:00007ffdc5c4e2d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [ 261.458183] RAX: ffffffffffffffda RBX: 000000005c73c202 RCX: 00007f28e91a8b87
> [ 261.467728] RDX: 0000000000000000 RSI: 00007ffdc5c4e340 RDI: 0000000000000003
> [ 261.477342] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000c
> [ 261.486970] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
> [ 261.496599] R13: 000000000067b4e0 R14: 00007ffdc5c5248c R15: 00007ffdc5c52480
> [ 261.506281] ==================================================================
> [ 261.516076] Disabling lock debugging due to kernel taint
> [ 261.523979] BUG: unable to handle kernel NULL pointer dereference at 00000000000000b0
> [ 261.534413] #PF error: [normal kernel read fault]
> [ 261.541730] PGD 8000000317400067 P4D 8000000317400067 PUD 316878067 PMD 0
> [ 261.551294] Oops: 0000 [#1] SMP KASAN PTI
> [ 261.557985] CPU: 14 PID: 2976 Comm: tc Tainted: G B 5.0.0-rc7+ #157
> [ 261.568306] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
> [ 261.578874] RIP: 0010:dst_cache_destroy+0x21/0xa0
> [ 261.586413] Code: f4 ff ff ff eb f6 0f 1f 00 0f 1f 44 00 00 41 56 41 55 49 c7 c6 60 fe 35 af 41 54 55 49 89 fc 53 bd ff ff ff ff e8 ef 98 73 ff <49> 83 3c 24 00 75 35 eb 6c 4c 63 ed e8 de 98 73 ff 4a 8d 3c ed 40
> [ 261.611247] RSP: 0018:ffff888316447160 EFLAGS: 00010282
> [ 261.619564] RAX: 0000000000000000 RBX: ffff88835b3e2f00 RCX: ffffffffad1c5071
> [ 261.629862] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: 0000000000000297
> [ 261.640149] RBP: 00000000ffffffff R08: fffffbfff5dd4e89 R09: fffffbfff5dd4e89
> [ 261.650467] R10: 0000000000000001 R11: fffffbfff5dd4e88 R12: 00000000000000b0
> [ 261.660785] R13: ffff8883267a10c0 R14: ffffffffaf35fe60 R15: 0000000000000001
> [ 261.671110] FS: 00007f28ea3e6400(0000) GS:ffff888364200000(0000) knlGS:0000000000000000
> [ 261.682447] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 261.691491] CR2: 00000000000000b0 CR3: 00000003178ae004 CR4: 00000000001606e0
> [ 261.701283] Call Trace:
> [ 261.706374] tunnel_key_release+0x3a/0x50 [act_tunnel_key]
> [ 261.714522] tcf_action_cleanup+0x2c/0xc0
> [ 261.721208] tcf_generic_walker+0x4c2/0x5c0
> [ 261.728074] ? tcf_action_dump_1+0x390/0x390
> [ 261.734996] ? tunnel_key_walker+0x5/0x1a0 [act_tunnel_key]
> [ 261.743247] ? tunnel_key_walker+0xe9/0x1a0 [act_tunnel_key]
> [ 261.751557] tca_action_gd+0x600/0xa40
> [ 261.757991] ? tca_get_fill.constprop.17+0x200/0x200
> [ 261.765644] ? __lock_acquire+0x588/0x1d20
> [ 261.772461] ? __lock_acquire+0x588/0x1d20
> [ 261.779266] ? mark_held_locks+0x90/0x90
> [ 261.785880] ? mark_held_locks+0x90/0x90
> [ 261.792470] ? __nla_parse+0xfe/0x190
> [ 261.798738] tc_ctl_action+0x218/0x230
> [ 261.805145] ? tcf_action_add+0x230/0x230
> [ 261.811760] rtnetlink_rcv_msg+0x3a5/0x600
> [ 261.818564] ? lock_downgrade+0x2d0/0x2d0
> [ 261.825433] ? validate_linkmsg+0x400/0x400
> [ 261.832256] ? find_held_lock+0x6d/0xd0
> [ 261.838624] ? match_held_lock+0x1b/0x210
> [ 261.845142] ? validate_linkmsg+0x400/0x400
> [ 261.851729] netlink_rcv_skb+0xc7/0x1f0
> [ 261.857976] ? netlink_ack+0x470/0x470
> [ 261.864132] ? netlink_deliver_tap+0x1f3/0x5a0
> [ 261.870969] netlink_unicast+0x2ae/0x350
> [ 261.877294] ? netlink_attachskb+0x340/0x340
> [ 261.883962] ? _copy_from_iter_full+0xdd/0x380
> [ 261.890750] ? __virt_addr_valid+0xb6/0xf0
> [ 261.897188] ? __check_object_size+0x159/0x240
> [ 261.903928] netlink_sendmsg+0x4d3/0x630
> [ 261.910112] ? netlink_unicast+0x350/0x350
> [ 261.916410] ? netlink_unicast+0x350/0x350
> [ 261.922656] sock_sendmsg+0x6d/0x80
> [ 261.928257] ___sys_sendmsg+0x48e/0x540
> [ 261.934183] ? copy_msghdr_from_user+0x210/0x210
> [ 261.940865] ? save_stack+0x89/0xb0
> [ 261.946355] ? __lock_acquire+0x588/0x1d20
> [ 261.952358] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.959468] ? mark_held_locks+0x90/0x90
> [ 261.965248] ? do_filp_open+0x138/0x1d0
> [ 261.970910] ? may_open_dev+0x50/0x50
> [ 261.976386] ? match_held_lock+0x1b/0x210
> [ 261.982210] ? __fget_light+0xa6/0xe0
> [ 261.987648] ? __sys_sendmsg+0xd2/0x150
> [ 261.993263] __sys_sendmsg+0xd2/0x150
> [ 261.998613] ? __ia32_sys_shutdown+0x30/0x30
> [ 262.004555] ? lock_downgrade+0x2d0/0x2d0
> [ 262.010236] ? mark_held_locks+0x1c/0x90
> [ 262.015758] ? do_syscall_64+0x1e/0x280
> [ 262.021234] do_syscall_64+0x78/0x280
> [ 262.026500] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 262.033207] RIP: 0033:0x7f28e91a8b87
> [ 262.038421] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53 48 89 f3 48
> [ 262.060708] RSP: 002b:00007ffdc5c4e2d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [ 262.070112] RAX: ffffffffffffffda RBX: 000000005c73c202 RCX: 00007f28e91a8b87
> [ 262.079087] RDX: 0000000000000000 RSI: 00007ffdc5c4e340 RDI: 0000000000000003
> [ 262.088122] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000c
> [ 262.097157] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
> [ 262.106207] R13: 000000000067b4e0 R14: 00007ffdc5c5248c R15: 00007ffdc5c52480
> [ 262.115271] Modules linked in: act_tunnel_key act_skbmod act_simple act_connmark nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 act_csum libcrc32c act_meta_skbtcindex act_meta_skbprio act_meta_mark act_ife ife act_police act_sample psample act_gact veth nfsv3 nfs_acl nfs lockd grace fscache bridge stp llc intel_rapl sb_edac mlx5_ib x86_pkg_temp_thermal sunrpc intel_powerclamp coretemp ib_uverbs kvm_intel ib_core kvm irqbypass mlx5_core crct10dif_pclmul crc32_pclmul crc32c_intel igb ghash_clmulni_intel intel_cstate mlxfw iTCO_wdt devlink intel_uncore iTCO_vendor_support ipmi_ssif ptp mei_me intel_rapl_perf ioatdma joydev pps_core ses mei i2c_i801 pcspkr enclosure lpc_ich dca wmi ipmi_si ipmi_devintf ipmi_msghandler acpi_pad acpi_power_meter pcc_cpufreq ast i2c_algo_bit drm_kms_helper ttm drm mpt3sas raid_class scsi_transport_sas
> [ 262.204393] CR2: 00000000000000b0
> [ 262.210390] ---[ end trace 2e41d786f2c7901a ]---
> [ 262.226790] RIP: 0010:dst_cache_destroy+0x21/0xa0
> [ 262.234083] Code: f4 ff ff ff eb f6 0f 1f 00 0f 1f 44 00 00 41 56 41 55 49 c7 c6 60 fe 35 af 41 54 55 49 89 fc 53 bd ff ff ff ff e8 ef 98 73 ff <49> 83 3c 24 00 75 35 eb 6c 4c 63 ed e8 de 98 73 ff 4a 8d 3c ed 40
> [ 262.258311] RSP: 0018:ffff888316447160 EFLAGS: 00010282
> [ 262.266304] RAX: 0000000000000000 RBX: ffff88835b3e2f00 RCX: ffffffffad1c5071
> [ 262.276251] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: 0000000000000297
> [ 262.286208] RBP: 00000000ffffffff R08: fffffbfff5dd4e89 R09: fffffbfff5dd4e89
> [ 262.296183] R10: 0000000000000001 R11: fffffbfff5dd4e88 R12: 00000000000000b0
> [ 262.306157] R13: ffff8883267a10c0 R14: ffffffffaf35fe60 R15: 0000000000000001
> [ 262.316139] FS: 00007f28ea3e6400(0000) GS:ffff888364200000(0000) knlGS:0000000000000000
> [ 262.327146] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 262.335815] CR2: 00000000000000b0 CR3: 00000003178ae004 CR4: 00000000001606e0
>
> Fixes: 41411e2fd6b8 ("net/sched: act_tunnel_key: Add dst_cache support")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> ---
> Changes from V1 to V2:
> - Extract metadata->dst error handler fix into standalone patch that targets
> net.
>
> net/sched/act_tunnel_key.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index 3404af72b4c1..fc2b884b170c 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -201,8 +201,14 @@ static void tunnel_key_release_params(struct tcf_tunnel_key_params *p)
> {
> if (!p)
> return;
> - if (p->tcft_action == TCA_TUNNEL_KEY_ACT_SET)
> + if (p->tcft_action == TCA_TUNNEL_KEY_ACT_SET) {
> +#ifdef CONFIG_DST_CACHE
> + struct ip_tunnel_info *info = &p->tcft_enc_metadata->u.tun_info;
> +
> + dst_cache_destroy(&info->dst_cache);
> +#endif
> dst_release(&p->tcft_enc_metadata->dst);
> + }
> kfree_rcu(p, rcu);
> }
>
> @@ -384,7 +390,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>
> release_dst_cache:
> #ifdef CONFIG_DST_CACHE
> - dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> + if (metadata)
> + dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> #endif
> release_tun_meta:
> dst_release(&metadata->dst);
> @@ -401,15 +408,8 @@ static void tunnel_key_release(struct tc_action *a)
> {
> struct tcf_tunnel_key *t = to_tunnel_key(a);
> struct tcf_tunnel_key_params *params;
> -#ifdef CONFIG_DST_CACHE
> - struct ip_tunnel_info *info;
> -#endif
>
> params = rcu_dereference_protected(t->params, 1);
> -#ifdef CONFIG_DST_CACHE
> - info = ¶ms->tcft_enc_metadata->u.tun_info;
> - dst_cache_destroy(&info->dst_cache);
> -#endif
> tunnel_key_release_params(params);
> }
>
>
Reviewed-by: Roi Dayan <roid@mellanox.com>
^ permalink raw reply
* Re: AF_XDP design flaws
From: Björn Töpel @ 2019-02-27 8:08 UTC (permalink / raw)
To: John Fastabend
Cc: Maxim Mikityanskiy, netdev@vger.kernel.org, Björn Töpel,
Magnus Karlsson, David S. Miller, Tariq Toukan, Saeed Mahameed,
Eran Ben Elisha
In-Reply-To: <b48e282f-8405-8974-8b71-df15f4bda8ab@gmail.com>
On 2019-02-26 17:41, John Fastabend wrote:
> On 2/26/19 6:49 AM, Maxim Mikityanskiy wrote:
>> Hi everyone,
>>
Hi! It's exciting to see more vendors looking into AF_XDP. ARM was
involved early on, and now Mellanox. :-) It's really important that
the AF_XDP model is a good fit for all drivers/vendors! Thanks for
looking into this.
>> I would like to discuss some design flaws of AF_XDP socket (XSK) implementation
>> in kernel. At the moment I don't see a way to work around them without changing
>> the API, so I would like to make sure that I'm not missing anything and to
>> suggest and discuss some possible improvements that can be made.
>>
>> The issues I describe below are caused by the fact that the driver depends on
>> the application doing some things, and if the application is
>> slow/buggy/malicious, the driver is forced to busy poll because of the lack of a
>> notification mechanism from the application side. I will refer to the i40e
>> driver implementation a lot, as it is the first implementation of AF_XDP, but
>> the issues are general and affect any driver. I already considered trying to fix
>> it on driver level, but it doesn't seem possible, so it looks like the behavior
>> and implementation of AF_XDP in the kernel has to be changed.
>>
>> RX side busy polling
>> ====================
>>
>> On the RX side, the driver expects the application to put some descriptors in
>> the Fill Ring. There is no way for the application to notify the driver that
>> there are more Fill Ring descriptors to take, so the driver is forced to busy
>> poll the Fill Ring if it gets empty. E.g., the i40e driver does it in NAPI poll:
>>
>> int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>> {
>> ...
>> failure = failure ||
>> !i40e_alloc_rx_buffers_fast_zc(rx_ring,
>> cleaned_count);
>> ...
>> return failure ? budget : (int)total_rx_packets;
>> }
>>
>> Basically, it means that if there are no descriptors in the Fill Ring, NAPI will
>> never stop, draining CPU.
>>
>> Possible cases when it happens
>> ------------------------------
>>
>> 1. The application is slow, it received some frames in the RX Ring, and it is
>> still handling the data, so it has no free frames to put to the Fill Ring.
>>
>> 2. The application is malicious, it opens an XSK and puts no frames to the Fill
>> Ring. It can be used as a local DoS attack.
>>
>> 3. The application is buggy and stops filling the Fill Ring for whatever reason
>> (deadlock, waiting for another blocking operation, other bugs).
>>
>> Although loading an XDP program requires root access, the DoS attack can be
>> targeted to setups that already use XDP, i.e. an XDP program is already loaded.
>> Even under root, userspace applications should not be able to disrupt system
>> stability by just calling normal APIs without an intention to destroy the
>> system, and here it happens in case 1.
> I believe this is by design. If packets per second is your top priority
> (at the expense of power, cpu, etc.) this is the behavior you might
> want. To resolve your points if you don't trust the application it
> should be isolated to a queue pair and cores so the impact is known and
> managed.
>
Correct, and I agree with John here. AF_XDP is a raw interface, and
needs to be managed.
> That said having a flag to back-off seems like a good idea. But should
> be doable as a feature without breaking current API.
>
>> Possible way to solve the issue
>> -------------------------------
>>
>> When the driver can't take new Fill Ring frames, it shouldn't busy poll.
>> Instead, it signals the failure to the application (e.g., with POLLERR), and
>> after that it's up to the application to restart polling (e.g., by calling
>> sendto()) after refilling the Fill Ring. The issue with this approach is that it
>> changes the API, so we either have to deal with it or to introduce some API
>> version field.
> See above. I like the idea here though.
>
The uapi doesn't mandate *how* the driver should behave. The rational
for the aggressive i40e fill ring polling (when the ring is empty) is
to minimize the latency. The "fill ring is empty" behavior might be
different on different drivers.
I see a number of different alternatives to the current i40e way of
busy-polling, all ranging from watchdog timers to actual HW
implementations where writing to the tail pointer of the fill ring
kicks a door bell.
That being said, I agree with previous speakers; Being able to set a
policy/driver behavior from userland is a good idea. This can be added
without breaking the existing model.
Max, any input to what this interface should look like from your
perspective?
>> TX side getting stuck
>> =====================
>>
>> On the TX side, there is the Completion Ring that the application has to clean.
>> If it doesn't, the i40e driver stops taking descriptors from the TX Ring. If the
>> application finally completes something, the driver can go on transmitting.
>> However, it would require busy polling the Completion Ring (just like with the
>> Fill Ring on the RX side). i40e doesn't do it, instead, it relies on the
>> application to kick the TX by calling sendto(). The issue is that poll() doesn't
>> return POLLOUT in this case, because the TX Ring is full, so the application
>> will never call sendto(), and the ring is stuck forever (or at least until
>> something triggers NAPI).
>>
>> Possible way to solve the issue
>> -------------------------------
>>
>> When the driver can't reserve a descriptor in the Completion Ring, it should
>> signal the failure to the application (e.g., with POLLERR). The application
>> shouldn't call sendto() every time it sees that the number of not completed
>> frames is greater than zero (like xdpsock sample does). Instead, the application
>> should kick the TX only when it wants to flush the ring, and, in addition, after
>> resolving the cause for POLLERR, i.e. after handling Completion Ring entries.
>> The API will also have to change with this approach.
>>
> +1 seems to me this can be done without breaking existing API though.
>
Keep in mind that the xdpsock application is sample/toy, and should
not be seen as the API documentation.
I don't see that the API need to be changed. AF_XDP requires that the
fill ring has entries, in order to receive data. Analogous, the
completion ring needs to managed by the application to send frames.
Are you suggesting that using poll() et al should be mandatory? Again,
take me through your send scenario, step by step.
To me, the userland application has all the knobs to determine whether
the Tx ring can be flushed or not.
>> Triggering NAPI on a different CPU core
>> =======================================
>>
>> .ndo_xsk_async_xmit runs on a random CPU core, so, to preserve CPU affinity,
>> i40e triggers an interrupt to schedule NAPI, instead of calling napi_schedule
>> directly. Scheduling NAPI on the correct CPU is what would every driver do, I
>> guess, but currently it has to be implemented differently in every driver, and
>> it relies on hardware features (the ability to trigger an IRQ).
> Ideally the application would be pinned to a core and the traffic
> steered to that core using ethtool/tc. Yes it requires a bit of mgmt on
> the platform but I think this is needed for best pps numbers.
>
>> I suggest introducing a kernel API that would allow triggering NAPI on a given
>> CPU. A brief look shows that something like smp_call_function_single_async can
>> be used. Advantages:
> Assuming you want to avoid pinning cores/traffic for some reason. Could
> this be done with some combination of cpumap + af_xdp? I haven't thought
> too much about it though. Maybe Bjorn has some ideas.
>
For the Intel drivers, the NAPI is associated to a certain
interrupt. This does not change with AF_XDP, so ndo_xsk_async_xmit
simply kicks the NAPI to run on the CPU bound defined by the irq
affinity.
I'm all open for additionally kernel APIs, and I'm happy to hack the
i40e internals as well. Again, it must be really simple to add
XDP/AF_XDP-ZC support for other vendor.
Ideally, what I would like is a generic way of associating netdev
queues (think ethtools-channels) with NAPI contexts. E.g. "queues
1,2,3 on NAPI foo, and I,II,III on NAPI bar, and the NAPIs should run
on on the baz cgroup". This is a much bigger task and outside the
AF_XDP scope. It ties in to the whole "threaded NAPIs and netdev
queues as a first class kernel object" discussion. ;-)
Magnus is working on proper busy-poll() (i.e. that the poll() syscall
executes the NAPI context). With that solution the user application
can select which core it wants to execute the poll() on -- and the
NAPI will be executed from the poll().
>> 1. It lifts the hardware requirement to be able to raise an interrupt on demand.
>>
>> 2. It would allow to move common code to the kernel (.ndo_xsk_async_xmit).
>>
>> 3. It is also useful in the situation where CPU affinity changes while being in
>> NAPI poll. Currently, i40e and mlx5e try to stop NAPI polling by returning
>> a value less than budget if CPU affinity changes. However, there are cases
>> (e.g., NAPIF_STATE_MISSED) when NAPI will be rescheduled on a wrong CPU. It's a
>> race between the interrupt, which will move NAPI to the correct CPU, and
>> __napi_schedule from a wrong CPU. Having an API to schedule NAPI on a given CPU
>> will benefit both mlx5e and i40e, because when this situation happens, it kills
>> the performance.
> How would we know what core to trigger NAPI on?
>
>> I would be happy to hear your thoughts about these issues.
> At least the first two observations seem incrementally solvable to me
> and would be nice improvements. I assume the last case can be resolved
> by pinning cores/traffic but for the general case perhaps it is useful.
>
Good summary, John. Really good input Max, and addressable -- but API
changes as far as I see it is not required.
(I'm out-of-office this week, skiing. So, I'll be away from the lists
until Monday!)
Cheers,
Björn
>> Thanks,
>> Max
>>
^ permalink raw reply
* [PATCH V2] samples: bpf: fix: broken sample regarding removed function
From: Daniel T. Lee @ 2019-02-27 7:52 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev
Currently, running sample "task_fd_query" and "tracex3" occurs the
following error. On kernel v5.0-rc* this sample will be unavailable
due to the removal of function 'blk_start_request' at commit "a1ce35f".
(function removed, as "Single Queue IO scheduler" no longer exists)
$ sudo ./task_fd_query
failed to create kprobe 'blk_start_request' error 'No such file or
directory'
This commit will change the function 'blk_start_request' to
'blk_mq_start_request' to fix the broken sample.
Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
V2 : typo fixed
---
samples/bpf/task_fd_query_kern.c | 2 +-
samples/bpf/task_fd_query_user.c | 2 +-
samples/bpf/tracex3_kern.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/samples/bpf/task_fd_query_kern.c b/samples/bpf/task_fd_query_kern.c
index f4b0a9ea674d..5f1b2cdababd 100644
--- a/samples/bpf/task_fd_query_kern.c
+++ b/samples/bpf/task_fd_query_kern.c
@@ -4,7 +4,7 @@
#include <uapi/linux/bpf.h>
#include "bpf_helpers.h"
-SEC("kprobe/blk_start_request")
+SEC("kprobe/blk_mq_start_request")
int bpf_prog1(struct pt_regs *ctx)
{
return 0;
diff --git a/samples/bpf/task_fd_query_user.c b/samples/bpf/task_fd_query_user.c
index 8381d792f138..a6e6c76e50c9 100644
--- a/samples/bpf/task_fd_query_user.c
+++ b/samples/bpf/task_fd_query_user.c
@@ -311,7 +311,7 @@ int main(int argc, char **argv)
}
/* test two functions in the corresponding *_kern.c file */
- CHECK_AND_RET(test_debug_fs_kprobe(0, "blk_start_request",
+ CHECK_AND_RET(test_debug_fs_kprobe(0, "blk_mq_start_request",
BPF_FD_TYPE_KPROBE));
CHECK_AND_RET(test_debug_fs_kprobe(1, "blk_account_io_completion",
BPF_FD_TYPE_KRETPROBE));
diff --git a/samples/bpf/tracex3_kern.c b/samples/bpf/tracex3_kern.c
index 9974c3d7c18b..4378492a970a 100644
--- a/samples/bpf/tracex3_kern.c
+++ b/samples/bpf/tracex3_kern.c
@@ -20,7 +20,7 @@ struct bpf_map_def SEC("maps") my_map = {
/* kprobe is NOT a stable ABI. If kernel internals change this bpf+kprobe
* example will no longer be meaningful
*/
-SEC("kprobe/blk_start_request")
+SEC("kprobe/blk_mq_start_request")
int bpf_prog1(struct pt_regs *ctx)
{
long rq = PT_REGS_PARM1(ctx);
--
2.17.1
^ permalink raw reply related
* Re: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
From: Gal Pressman @ 2019-02-27 7:31 UTC (permalink / raw)
To: Saleem, Shiraz, dledford@redhat.com, jgg@ziepe.ca,
davem@davemloft.net
Cc: linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
Ismail, Mustafa, Kirsher, Jeffrey T, Yossi Leybovich
In-Reply-To: <9DD61F30A802C4429A01CA4200E302A7A5A4FB85@fmsmsx124.amr.corp.intel.com>
On 26-Feb-19 23:09, Saleem, Shiraz wrote:
>> Subject: Re: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
>>
>> On 15-Feb-19 19:10, Shiraz Saleem wrote:
>>> /**
>>> * irdma_dealloc_ucontext - deallocate the user context data structure
>>> * @context: user context created during alloc */ static int
>>> irdma_dealloc_ucontext(struct ib_ucontext *context) {
>>> struct irdma_ucontext *ucontext = to_ucontext(context);
>>> unsigned long flags;
>>>
>>> spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
>>> if (!list_empty(&ucontext->cq_reg_mem_list)) {
>>> spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
>>> return -EBUSY;
>>> }
>>> spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
>>>
>>> spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
>>> if (!list_empty(&ucontext->qp_reg_mem_list)) {
>>> spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
>>> return -EBUSY;
>>
>> Drivers are not permitted to fail dealloc_ucontext.
>
> This is fixed in RFC v1 submission. Maybe this was pasted from the v0 ver?
>
> [..]
>
>>> +/**
>>> + * irdma_alloc_pd - allocate protection domain
>>> + * @pd: PD pointer
>>> + * @context: user context created during alloc
>>> + * @udata: user data
>>> + */
>>> +static int irdma_alloc_pd(struct ib_pd *pd,
>>> + struct ib_ucontext *context,
>>> + struct ib_udata *udata)
>>> +{
>>> + struct irdma_pd *iwpd = to_iwpd(pd);
>>> + struct irdma_device *iwdev = to_iwdev(pd->device);
>>> + struct irdma_sc_dev *dev = &iwdev->rf->sc_dev;
>>> + struct irdma_pci_f *rf = iwdev->rf;
>>> + struct irdma_alloc_pd_resp uresp = {};
>>> + struct irdma_sc_pd *sc_pd;
>>> + struct irdma_ucontext *ucontext;
>>> + u32 pd_id = 0;
>>> + int err;
>>> +
>>> + if (iwdev->closing)
>>> + return -ENODEV;
>>> +
>>> + err = irdma_alloc_rsrc(rf, rf->allocated_pds, rf->max_pd, &pd_id,
>>> + &rf->next_pd);
>>> + if (err)
>>> + return err;
>>> +
>>> + sc_pd = &iwpd->sc_pd;
>>> + if (context) {
>>
>> I think this should be 'if (udata)', this applies to many other places in this driver.
>
> That’s right. Will fix it.
>
>>
>>> + ucontext = to_ucontext(context);
>>> + dev->iw_pd_ops->pd_init(dev, sc_pd, pd_id, ucontext->abi_ver);
>>> + uresp.pd_id = pd_id;
>>> + if (ib_copy_to_udata(udata, &uresp, sizeof(uresp))) {
>>> + err = -EFAULT;
>>> + goto error;
>>> + }
>>> + } else {
>>> + dev->iw_pd_ops->pd_init(dev, sc_pd, pd_id, -1);
>>> + }
>>> +
>>> + irdma_add_pdusecount(iwpd);
>>> +
>>> + return 0;
>>> +error:
>>> + irdma_free_rsrc(rf, rf->allocated_pds, pd_id);
>>> +
>>> + return err;
>>> +}
>>> +/**
>>> + * irdma_create_qp - create qp
>>> + * @ibpd: ptr of pd
>>> + * @init_attr: attributes for qp
>>> + * @udata: user data for create qp
>>> + */
>>> +static struct ib_qp *irdma_create_qp(struct ib_pd *ibpd,
>>> + struct ib_qp_init_attr *init_attr,
>>> + struct ib_udata *udata)
>>> +{
>>> + struct irdma_pd *iwpd = to_iwpd(ibpd);
>>> + struct irdma_device *iwdev = to_iwdev(ibpd->device);
>>> + struct irdma_pci_f *rf = iwdev->rf;
>>> + struct irdma_cqp *iwcqp = &rf->cqp;
>>> + struct irdma_qp *iwqp;
>>> + struct irdma_ucontext *ucontext;
>>> + struct irdma_create_qp_req req;
>>> + struct irdma_create_qp_resp uresp = {};
>>> + struct i40iw_create_qp_resp uresp_gen1 = {};
>>> + u32 qp_num = 0;
>>> + void *mem;
>>> + enum irdma_status_code ret;
>>> + int err_code = 0;
>>> + int sq_size;
>>> + int rq_size;
>>> + struct irdma_sc_qp *qp;
>>> + struct irdma_sc_dev *dev = &rf->sc_dev;
>>> + struct irdma_qp_init_info init_info = {};
>>> + struct irdma_create_qp_info *qp_info;
>>> + struct irdma_cqp_request *cqp_request;
>>> + struct cqp_cmds_info *cqp_info;
>>> + struct irdma_qp_host_ctx_info *ctx_info;
>>> + struct irdma_iwarp_offload_info *iwarp_info;
>>> + struct irdma_roce_offload_info *roce_info;
>>> + struct irdma_udp_offload_info *udp_info;
>>> + unsigned long flags;
>>> +
>>> + if (iwdev->closing)
>>> + return ERR_PTR(-ENODEV);
>>> +
>>> + if (init_attr->create_flags)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + if (init_attr->cap.max_inline_data > dev->hw_attrs.max_hw_inline)
>>> + init_attr->cap.max_inline_data = dev->hw_attrs.max_hw_inline;
>>> +
>>> + if (init_attr->cap.max_send_sge > dev->hw_attrs.max_hw_wq_frags)
>>> + init_attr->cap.max_send_sge = dev-
>>> hw_attrs.max_hw_wq_frags;
>>> +
>>> + if (init_attr->cap.max_recv_sge > dev->hw_attrs.max_hw_wq_frags)
>>> + init_attr->cap.max_recv_sge = dev->hw_attrs.max_hw_wq_frags;
>>
>> AFAIK, you can change the requested values to be greater than or equal to the
>> values requested. I don't think you can change them to something smaller.
>
> Hmm...This is a sanity check to make sure we don’t exceed the device supported values.
> But we should fail the call.
>
> [..]
>
>>> + mem = kzalloc(sizeof(*iwqp), GFP_KERNEL);
>>> + if (!mem)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + iwqp = (struct irdma_qp *)mem;
>>> + iwqp->allocated_buf = mem;
>>
>> 'allocated_buf' feels redundant. Why is iwqp not sufficient?
>
> I agree.
> [..]
>
>>> + if (udata) {
>>> + err_code = ib_copy_from_udata(&req, udata, sizeof(req));
>>
>> Perhaps ib_copy_from_udata(&req, udata, min(sizeof(req), udata->inlen)?
>> Applies to other call sites of ib_copy_from/to_udata as well.
>>
>
> It’s a good idea.
>
>>> + * irdma_query - query qp attributes
>>> + * @ibqp: qp pointer
>>> + * @attr: attributes pointer
>>> + * @attr_mask: Not used
>>> + * @init_attr: qp attributes to return */ static int
>>> +irdma_query_qp(struct ib_qp *ibqp,
>>> + struct ib_qp_attr *attr,
>>> + int attr_mask,
>>> + struct ib_qp_init_attr *init_attr) {
>>> + struct irdma_qp *iwqp = to_iwqp(ibqp);
>>> + struct irdma_sc_qp *qp = &iwqp->sc_qp;
>>> +
>>> + attr->qp_state = iwqp->ibqp_state;
>>> + attr->cur_qp_state = iwqp->ibqp_state;
>>> + attr->qp_access_flags = 0;
>>> + attr->cap.max_send_wr = qp->qp_uk.sq_size - 1;
>>> + attr->cap.max_recv_wr = qp->qp_uk.rq_size - 1;
>>
>> Why -1?
>
> It's reserved for HW. But the equation should be
> (sqdepth - I40IW_SQ_RSVD) >> sqshift.
>
> [....]
>>
>>> + attr->cap.max_inline_data = qp->qp_uk.max_inline_data;
>>> + attr->cap.max_send_sge = qp->qp_uk.max_sq_frag_cnt;
>>> + attr->cap.max_recv_sge = qp->qp_uk.max_rq_frag_cnt;
>>> + attr->qkey = iwqp->roce_info.qkey;
>>> +
>>> + init_attr->event_handler = iwqp->ibqp.event_handler;
>>> + init_attr->qp_context = iwqp->ibqp.qp_context;
>>> + init_attr->send_cq = iwqp->ibqp.send_cq;
>>> + init_attr->recv_cq = iwqp->ibqp.recv_cq;
>>> + init_attr->srq = iwqp->ibqp.srq;
>>> + init_attr->cap = attr->cap;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * irdma_destroy_cq - destroy cq
>>> + * @ib_cq: cq pointer
>>> + */
>>> +static int irdma_destroy_cq(struct ib_cq *ib_cq) {
>>> + struct irdma_cq *iwcq;
>>> + struct irdma_device *iwdev;
>>> + struct irdma_sc_cq *cq;
>>> +
>>> + if (!ib_cq) {
>>> + irdma_pr_err("ib_cq == NULL\n");
>>> + return 0;
>>> + }
>>
>> Is this really needed? Which caller can pass NULL pointer?
>
> Not needed.
>
>>> +
>>> +/**
>>> + * board_id_show
>>> + */
>>> +static ssize_t board_id_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + return sprintf(buf, "%.*s\n", 32, "IRDMA Board ID");
>>
>> That doesn't add much information.
>
> Will fix.
>
>>
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(hw_rev);
>>> +static DEVICE_ATTR_RO(hca_type);
>>> +static DEVICE_ATTR_RO(board_id);
>>> +
>>> +static struct attribute *irdma_dev_attributes[] = {
>>> + &dev_attr_hw_rev.attr,
>>> + &dev_attr_hca_type.attr,
>>> + &dev_attr_board_id.attr,
>>> + NULL
>>> +};
>>> +
>>> +static const struct attribute_group irdma_attr_group = {
>>> + .attrs = irdma_dev_attributes,
>>> +};
>>> +
>>> +/**
>>> + * irdma_modify_port Modify port properties
>>> + * @ibdev: device pointer from stack
>>> + * @port: port number
>>> + * @port_modify_mask: mask for port modifications
>>> + * @props: port properties
>>> + */
>>> +static int irdma_modify_port(struct ib_device *ibdev,
>>> + u8 port,
>>> + int port_modify_mask,
>>> + struct ib_port_modify *props) {
>>> + return 0;
>>> +}
>>
>> Same question as disacossiate_ucontext.
>
> This was likely added during early dev. and can be removed.
>
>>
>>> +
>>> +/**
>>> + * irdma_query_gid_roce - Query port GID for Roce
>>> + * @ibdev: device pointer from stack
>>> + * @port: port number
>>> + * @index: Entry index
>>> + * @gid: Global ID
>>> + */
>>> +static int irdma_query_gid_roce(struct ib_device *ibdev,
>>> + u8 port,
>>> + int index,
>>> + union ib_gid *gid)
>>> +{
>>> + int ret;
>>> +
>>> + ret = rdma_query_gid(ibdev, port, index, gid);
>>> + if (ret == -EAGAIN) {
>>
>> I can't see a path where rdma_query_gid returns -EAGAIN.
>
> This function can be removed now. It's only applicable to non-Roce providers.
>
>>
>>> + memcpy(gid, &zgid, sizeof(*gid));
>>> + return 0;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>
>>> +/**
>>> + * irdma_create_ah - create address handle
>>> + * @ibpd: ptr to protection domain
>>> + * @ah_attr: address handle attributes
>>
>> 'ah_attr' -> 'attr', missing flags and udata.
>
> Will fix all these hits in the driver.
>
> [..]
>>> + */
>>> +static int irdma_destroy_ah(struct ib_ah *ibah, u32 flags) {
>>> + struct irdma_device *iwdev = to_iwdev(ibah->device);
>>> + struct irdma_ah *ah = to_iwah(ibah);
>>> + int err;
>>> +
>>> + if (!ah->sc_ah.ah_info.ah_valid)
>>> + return -EINVAL;
>>> +
>>> + err = irdma_ah_cqp_op(iwdev->rf, &ah->sc_ah,
>> IRDMA_OP_AH_DESTROY,
>>> + flags & RDMA_DESTROY_AH_SLEEPABLE,
>>> + irdma_destroy_ah_cb, ah);
>>> + if (!err)
>>> + return 0;
>>
>> Why are the rest of the cleanups only in case of error?
>
> On success, the cleanup is done in the callback, irdma_destroy_ah_cb.
>
> [...]
>
>
>>> +static __be64 irdma_mac_to_guid(struct net_device *ndev) {
>>> + unsigned char *mac = ndev->dev_addr;
>>> + __be64 guid;
>>> + unsigned char *dst = (unsigned char *)&guid;
>>> +
>>> + dst[0] = mac[0] ^ 2;
>>> + dst[1] = mac[1];
>>> + dst[2] = mac[2];
>>> + dst[3] = 0xff;
>>> + dst[4] = 0xfe;
>>> + dst[5] = mac[3];
>>> + dst[6] = mac[4];
>>> + dst[7] = mac[5];
>>> +
>>> + return guid;
>>> +}
>>
>> There's a variant of this function in irdma, bnxt_re, ocrdma and qedr.
>> Maybe it's time to provide it in common code?
>
> Agreed.
>
Other than that:
Reviewed-by: Gal Pressman <galpress@amazon.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox