* [net-next PATCH v2] ixgbe: Remove IXGBE_FLAG_IN_NETPOLL since it doesn't do anything
From: Alexander Duyck @ 2014-11-19 23:28 UTC (permalink / raw)
To: netdev; +Cc: donald.c.skidmore, jeffrey.t.kirsher
This patch removes some dead code from the cleanup path for ixgbe.
Setting and clearing the flag doesn't do anything since all we are doing is
setting the flag, scheduling napi, clearing the flag and then letting
netpoll do the polling cleanup. As such it doesn't make much sense to have
it there.
This patch also removes one minor white-space error.
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
v2: Fixed an unused variable warning for adapter in ixgbe_rx_skb
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 18 ++++--------------
2 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 86fa607..f7d46b3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -610,7 +610,6 @@ struct ixgbe_adapter {
#define IXGBE_FLAG_RX_1BUF_CAPABLE (u32)(1 << 4)
#define IXGBE_FLAG_RX_PS_CAPABLE (u32)(1 << 5)
#define IXGBE_FLAG_RX_PS_ENABLED (u32)(1 << 6)
-#define IXGBE_FLAG_IN_NETPOLL (u32)(1 << 7)
#define IXGBE_FLAG_DCA_ENABLED (u32)(1 << 8)
#define IXGBE_FLAG_DCA_CAPABLE (u32)(1 << 9)
#define IXGBE_FLAG_IMIR_ENABLED (u32)(1 << 10)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 7c4d3b3..3dfec61 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1583,14 +1583,10 @@ static void ixgbe_process_skb_fields(struct ixgbe_ring *rx_ring,
static void ixgbe_rx_skb(struct ixgbe_q_vector *q_vector,
struct sk_buff *skb)
{
- struct ixgbe_adapter *adapter = q_vector->adapter;
-
if (ixgbe_qv_busy_polling(q_vector))
netif_receive_skb(skb);
- else if (!(adapter->flags & IXGBE_FLAG_IN_NETPOLL))
- napi_gro_receive(&q_vector->napi, skb);
else
- netif_rx(skb);
+ napi_gro_receive(&q_vector->napi, skb);
}
/**
@@ -6064,7 +6060,6 @@ static void ixgbe_check_hang_subtask(struct ixgbe_adapter *adapter)
/* Cause software interrupt to ensure rings are cleaned */
ixgbe_irq_rearm_queues(adapter, eics);
-
}
/**
@@ -7397,14 +7392,9 @@ static void ixgbe_netpoll(struct net_device *netdev)
if (test_bit(__IXGBE_DOWN, &adapter->state))
return;
- adapter->flags |= IXGBE_FLAG_IN_NETPOLL;
- if (adapter->flags & IXGBE_FLAG_MSIX_ENABLED) {
- for (i = 0; i < adapter->num_q_vectors; i++)
- ixgbe_msix_clean_rings(0, adapter->q_vector[i]);
- } else {
- ixgbe_intr(adapter->pdev->irq, netdev);
- }
- adapter->flags &= ~IXGBE_FLAG_IN_NETPOLL;
+ /* loop through and schedule all active queues */
+ for (i = 0; i < adapter->num_q_vectors; i++)
+ ixgbe_msix_clean_rings(0, adapter->q_vector[i]);
}
#endif
^ permalink raw reply related
* Re: [PATCHv10 ovs 12/15] datapath: Add support for unique flow identifiers.
From: Pravin Shelar @ 2014-11-19 23:34 UTC (permalink / raw)
To: Joe Stringer; +Cc: dev@openvswitch.org, netdev
In-Reply-To: <1415906275-3172-13-git-send-email-joestringer@nicira.com>
On Thu, Nov 13, 2014 at 11:17 AM, Joe Stringer <joestringer@nicira.com> wrote:
> Previously, flows were manipulated by userspace specifying a full,
> unmasked flow key. This adds significant burden onto flow
> serialization/deserialization, particularly when dumping flows.
>
> This patch adds an alternative way to refer to flows using a
> variable-length "unique flow identifier" (UFID). At flow setup time,
> userspace may specify a UFID for a flow, which is stored with the flow
> and inserted into a separate table for lookup, in addition to the
> standard flow table. Flows created using a UFID must be fetched or
> deleted using the UFID.
>
> All flow dump operations may now be made more terse with OVS_UFID_F_*
> flags. For example, the OVS_UFID_F_OMIT_KEY flag allows responses to
> omit the flow key from a datapath operation if the flow has a
> corresponding UFID. This significantly reduces the time spent assembling
> and transacting netlink messages. With all OVS_UFID_F_OMIT_* flags
> enabled, the datapath only returns the UFID and statistics for each flow
> during flow dump, increasing ovs-vswitchd revalidator performance by up
> to 50%.
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> CC: Pravin B Shelar <pshelar@nicira.com>
> CC: netdev@vger.kernel.org
> ---
> v10: Ignore flow_key in requests if UFID is specified.
> Only allow UFID flows to be indexed by UFID.
> Only allow non-UFID flows to be indexed by unmasked flow key.
> Unite the unmasked_key and ufid+ufid_hash in 'struct sw_flow'.
> Don't periodically rehash the UFID table.
> Resize the UFID table independently from the flow table.
> Modify table_destroy() to iterate once and delete from both tables.
> Fix UFID memory leak in flow_free().
> Remove kernel-only UFIDs for non-UFID cases.
> Rename "OVS_UFID_F_SKIP_*" -> "OVS_UFID_F_OMIT_*"
> Update documentation.
> Rebase.
> v9: No change.
> v8: Rename UID -> UFID "unique flow identifier".
> Fix null dereference when adding flow without uid or mask.
> If UFID and not match are specified, and lookup fails, return ENOENT.
> Rebase.
> v7: Remove OVS_DP_F_INDEX_BY_UID.
> Rework UID serialisation for variable-length UID.
> Log error if uid not specified and OVS_UID_F_SKIP_KEY is set.
> Rebase against "probe" logging changes.
> v6: Fix documentation for supporting UIDs between 32-128 bits.
> Minor style fixes.
> Rebase.
> v5: No change.
> v4: Fix memory leaks.
> Log when triggering the older userspace issue above.
> v3: Initial post.
> ---
> datapath/README.md | 13 ++
> datapath/datapath.c | 248 +++++++++++++++------
> datapath/flow.h | 20 +-
> datapath/flow_netlink.c | 35 +++
> datapath/flow_netlink.h | 1 +
> datapath/flow_table.c | 214 ++++++++++++++----
> datapath/flow_table.h | 8 +
> datapath/linux/compat/include/linux/openvswitch.h | 30 +++
> 8 files changed, 453 insertions(+), 116 deletions(-)
>
> diff --git a/datapath/README.md b/datapath/README.md
> index a8effa3..9c03a2b 100644
> --- a/datapath/README.md
> +++ b/datapath/README.md
> @@ -131,6 +131,19 @@ performs best-effort detection of overlapping wildcarded flows and may reject
> some but not all of them. However, this behavior may change in future versions.
>
>
> +Unique flow identifiers
> +-----------------------
> +
> +An alternative to using the original match portion of a key as the handle for
> +flow identification is a unique flow identifier, or "UFID". UFIDs are optional
> +for both the kernel and user space program.
> +
> +User space programs that support UFID are expected to provide it during flow
> +setup in addition to the flow, then refer to the flow using the UFID for all
> +future operations. The kernel is not required to index flows by the original
> +flow key if a UFID is specified.
> +
> +
> Basic rule for evolving flow keys
> ---------------------------------
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index a898584..c14d834 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -671,11 +671,18 @@ static void get_dp_stats(const struct datapath *dp, struct ovs_dp_stats *stats,
> }
> }
>
> -static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts)
> +static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts,
> + const struct sw_flow_id *sfid)
> {
> + size_t sfid_len = 0;
> +
> + if (sfid && sfid->ufid_len)
> + sfid_len = nla_total_size(sfid->ufid_len);
> +
> return NLMSG_ALIGN(sizeof(struct ovs_header))
> + nla_total_size(ovs_key_attr_size()) /* OVS_FLOW_ATTR_KEY */
> + nla_total_size(ovs_key_attr_size()) /* OVS_FLOW_ATTR_MASK */
> + + sfid_len /* OVS_FLOW_ATTR_UFID */
> + nla_total_size(sizeof(struct ovs_flow_stats)) /* OVS_FLOW_ATTR_STATS */
> + nla_total_size(1) /* OVS_FLOW_ATTR_TCP_FLAGS */
> + nla_total_size(8) /* OVS_FLOW_ATTR_USED */
> @@ -684,33 +691,43 @@ static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts)
>
> /* Called with ovs_mutex or RCU read lock. */
> static int ovs_flow_cmd_fill_match(const struct sw_flow *flow,
> - struct sk_buff *skb)
> + struct sk_buff *skb, u32 ufid_flags)
> {
> struct nlattr *nla;
> int err;
>
> - /* Fill flow key. */
> - nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
> - if (!nla)
> - return -EMSGSIZE;
> -
> - err = ovs_nla_put_flow(&flow->unmasked_key, &flow->unmasked_key, skb,
> - false);
> - if (err)
> - return err;
> -
> - nla_nest_end(skb, nla);
> + /* Fill flow key. If userspace didn't specify a UFID, then ignore the
> + * OMIT_KEY flag. */
> + if (!(ufid_flags & OVS_UFID_F_OMIT_KEY) ||
> + !flow->index_by_ufid) {
I am not sure about this check, userspace needs to send atleast ufid
or the unmasked key as id for flow. otherwise we shld flag error. Here
we can serialize flow->key.
There could be another function which takes care of flow-id
serialization where we serialize use ufid or unmasked key as flow id.
Lets group ufid and unmasked key together rather than masked key and
unmasked key which are not related.
> + nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
> + if (!nla)
> + return -EMSGSIZE;
> +
> + if (flow->index_by_ufid)
> + err = ovs_nla_put_flow(&flow->mask->key, &flow->key,
> + skb, false);
> + else
> + err = ovs_nla_put_flow(&flow->index.unmasked_key,
> + &flow->index.unmasked_key, skb,
> + false);
> + if (err)
> + return err;
> + nla_nest_end(skb, nla);
> + }
>
> /* Fill flow mask. */
> - nla = nla_nest_start(skb, OVS_FLOW_ATTR_MASK);
> - if (!nla)
> - return -EMSGSIZE;
> + if (!(ufid_flags & OVS_UFID_F_OMIT_MASK)) {
> + nla = nla_nest_start(skb, OVS_FLOW_ATTR_MASK);
> + if (!nla)
> + return -EMSGSIZE;
>
> - err = ovs_nla_put_flow(&flow->key, &flow->mask->key, skb, true);
> - if (err)
> - return err;
> + err = ovs_nla_put_flow(&flow->key, &flow->mask->key, skb, true);
> + if (err)
> + return err;
> + nla_nest_end(skb, nla);
> + }
>
> - nla_nest_end(skb, nla);
> return 0;
> }
>
> @@ -740,6 +757,32 @@ static int ovs_flow_cmd_fill_stats(const struct sw_flow *flow,
> }
>
> /* Called with ovs_mutex or RCU read lock. */
> +static int ovs_flow_cmd_fill_ufid(const struct sw_flow *flow,
> + struct sk_buff *skb)
> +{
> + struct nlattr *start;
> + const struct sw_flow_id *sfid;
> +
> + if (!flow->index_by_ufid)
> + return 0;
> +
> + sfid = &flow->index.ufid;
> + start = nla_nest_start(skb, OVS_FLOW_ATTR_UFID);
> + if (start) {
> + int err;
> +
> + err = nla_put(skb, OVS_UFID_ATTR_ID, sfid->ufid_len,
> + sfid->ufid);
> + if (err)
> + return err;
> + nla_nest_end(skb, start);
> + } else
> + return -EMSGSIZE;
> +
> + return 0;
> +}
> +
Can you change this function according to comments above?
> +/* Called with ovs_mutex or RCU read lock. */
> static int ovs_flow_cmd_fill_actions(const struct sw_flow *flow,
> struct sk_buff *skb, int skb_orig_len)
> {
> @@ -782,7 +825,7 @@ static int ovs_flow_cmd_fill_actions(const struct sw_flow *flow,
> /* Called with ovs_mutex or RCU read lock. */
> static int ovs_flow_cmd_fill_info(const struct sw_flow *flow, int dp_ifindex,
> struct sk_buff *skb, u32 portid,
> - u32 seq, u32 flags, u8 cmd)
> + u32 seq, u32 flags, u8 cmd, u32 ufid_flags)
> {
> const int skb_orig_len = skb->len;
> struct ovs_header *ovs_header;
> @@ -795,18 +838,24 @@ static int ovs_flow_cmd_fill_info(const struct sw_flow *flow, int dp_ifindex,
>
> ovs_header->dp_ifindex = dp_ifindex;
>
> - err = ovs_flow_cmd_fill_match(flow, skb);
> + err = ovs_flow_cmd_fill_match(flow, skb, ufid_flags);
> if (err)
> goto error;
>
> - err = ovs_flow_cmd_fill_stats(flow, skb);
> + err = ovs_flow_cmd_fill_ufid(flow, skb);
> if (err)
> goto error;
Flow ID should go first in the netlink msg.
>
> - err = ovs_flow_cmd_fill_actions(flow, skb, skb_orig_len);
> + err = ovs_flow_cmd_fill_stats(flow, skb);
> if (err)
> goto error;
>
> + if (!(ufid_flags & OVS_UFID_F_OMIT_ACTIONS)) {
> + err = ovs_flow_cmd_fill_actions(flow, skb, skb_orig_len);
> + if (err)
> + goto error;
> + }
> +
> return genlmsg_end(skb, ovs_header);
>
> error:
> @@ -816,6 +865,7 @@ error:
>
> /* May not be called with RCU read lock. */
> static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *acts,
> + const struct sw_flow_id *sfid,
> struct genl_info *info,
> bool always)
> {
> @@ -825,30 +875,36 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *act
> GROUP_ID(&ovs_dp_flow_multicast_group)))
> return NULL;
>
> - skb = genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts), info, GFP_KERNEL);
> + skb = genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts, sfid), info,
> + GFP_KERNEL);
> if (!skb)
> return ERR_PTR(-ENOMEM);
>
> return skb;
> }
>
> +static const struct sw_flow_id *flow_get_ufid(const struct sw_flow *flow)
> +{
> + return flow->index_by_ufid ? &flow->index.ufid : NULL;
> +}
> +
> /* Called with ovs_mutex. */
> static struct sk_buff *ovs_flow_cmd_build_info(const struct sw_flow *flow,
> int dp_ifindex,
> struct genl_info *info, u8 cmd,
> - bool always)
> + bool always, u32 ufid_flags)
> {
> struct sk_buff *skb;
> int retval;
>
> - skb = ovs_flow_cmd_alloc_info(ovsl_dereference(flow->sf_acts), info,
> - always);
> + skb = ovs_flow_cmd_alloc_info(ovsl_dereference(flow->sf_acts),
> + flow_get_ufid(flow), info, always);
> if (IS_ERR_OR_NULL(skb))
> return skb;
>
> retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb,
> info->snd_portid, info->snd_seq, 0,
> - cmd);
> + cmd, ufid_flags);
> BUG_ON(retval < 0);
> return skb;
> }
> @@ -863,6 +919,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> struct datapath *dp;
> struct sw_flow_actions *acts;
> struct sw_flow_match match;
> + struct sw_flow_id sfid;
> + u32 ufid_flags;
> int error;
> bool log = !a[OVS_FLOW_ATTR_PROBE];
>
> @@ -887,13 +945,20 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> }
>
> /* Extract key. */
> - ovs_match_init(&match, &new_flow->unmasked_key, &mask);
> + ovs_match_init(&match, &new_flow->index.unmasked_key, &mask);
> error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY],
> a[OVS_FLOW_ATTR_MASK], log);
> if (error)
> goto err_kfree_flow;
>
> - ovs_flow_mask_key(&new_flow->key, &new_flow->unmasked_key, &mask);
> + ovs_flow_mask_key(&new_flow->key, &new_flow->index.unmasked_key, &mask);
> +
> + /* Extract ufid. */
> + error = ovs_nla_get_ufid(a[OVS_FLOW_ATTR_UFID], &sfid, &ufid_flags);
> + if (!error)
> + error = ovs_flow_ufid(new_flow, &sfid);
> + if (error)
> + goto err_kfree_flow;
>
> /* Validate actions. */
> error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &new_flow->key,
> @@ -903,7 +968,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> goto err_kfree_flow;
> }
>
> - reply = ovs_flow_cmd_alloc_info(acts, info, false);
> + reply = ovs_flow_cmd_alloc_info(acts, &sfid, info, false);
> if (IS_ERR(reply)) {
> error = PTR_ERR(reply);
> goto err_kfree_acts;
> @@ -915,8 +980,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> error = -ENODEV;
> goto err_unlock_ovs;
> }
> +
> /* Check if this is a duplicate flow */
> - flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->unmasked_key);
> + flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
Need to check for ufid table to find duplicate ufid entry here.
> if (likely(!flow)) {
> rcu_assign_pointer(new_flow->sf_acts, acts);
>
> @@ -932,7 +998,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> ovs_header->dp_ifindex,
> reply, info->snd_portid,
> info->snd_seq, 0,
> - OVS_FLOW_CMD_NEW);
> + OVS_FLOW_CMD_NEW,
> + ufid_flags);
> BUG_ON(error < 0);
> }
> ovs_unlock();
> @@ -950,14 +1017,16 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> error = -EEXIST;
> goto err_unlock_ovs;
> }
> - /* The unmasked key has to be the same for flow updates. */
> - if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match))) {
> - /* Look for any overlapping flow. */
> + /* The flow identifier has to be the same for flow updates. */
> + if (sfid.ufid_len) {
> + if (unlikely(!ovs_flow_cmp_ufid(flow, &sfid)))
> + flow = NULL;
> + } else if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match))) {
> flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> - if (!flow) {
> - error = -ENOENT;
> - goto err_unlock_ovs;
> - }
> + }
> + if (unlikely(!flow)) {
> + error = -ENOENT;
> + goto err_unlock_ovs;
> }
> /* Update actions. */
> old_acts = ovsl_dereference(flow->sf_acts);
> @@ -968,7 +1037,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> ovs_header->dp_ifindex,
> reply, info->snd_portid,
> info->snd_seq, 0,
> - OVS_FLOW_CMD_NEW);
> + OVS_FLOW_CMD_NEW,
> + ufid_flags);
> BUG_ON(error < 0);
> }
> ovs_unlock();
> @@ -1018,30 +1088,36 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> struct nlattr **a = info->attrs;
> struct ovs_header *ovs_header = info->userhdr;
> struct sw_flow_key key;
> - struct sw_flow *flow;
> + struct sw_flow *flow = NULL;
> struct sw_flow_mask mask;
> struct sk_buff *reply = NULL;
> struct datapath *dp;
> struct sw_flow_actions *old_acts = NULL, *acts = NULL;
> struct sw_flow_match match;
> + struct sw_flow_id ufid;
> + u32 ufid_flags;
> int error;
> bool log = !a[OVS_FLOW_ATTR_PROBE];
>
> - /* Extract key. */
> - error = -EINVAL;
> - if (!a[OVS_FLOW_ATTR_KEY]) {
> - OVS_NLERR(log, "Flow key attribute not present in set flow.");
> + /* Extract ufid/key. */
> + error = ovs_nla_get_ufid(a[OVS_FLOW_ATTR_UFID], &ufid,
> + &ufid_flags);
> + if (error)
> goto error;
> + if (a[OVS_FLOW_ATTR_KEY]) {
> + ovs_match_init(&match, &key, &mask);
> + error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY],
> + a[OVS_FLOW_ATTR_MASK], log);
> + } else if (!ufid.ufid_len) {
> + OVS_NLERR(log, "Flow key attribute not present in set flow.\n");
> + error = -EINVAL;
> }
> -
> - ovs_match_init(&match, &key, &mask);
> - error = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY],
> - a[OVS_FLOW_ATTR_MASK], log);
> if (error)
> goto error;
>
> /* Validate actions. */
> - if (a[OVS_FLOW_ATTR_ACTIONS]) {
> + if (a[OVS_FLOW_ATTR_ACTIONS] && a[OVS_FLOW_ATTR_KEY] &&
> + a[OVS_FLOW_ATTR_MASK]) {
> acts = get_flow_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, &mask,
> log);
> if (IS_ERR(acts)) {
> @@ -1050,7 +1126,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> }
>
> /* Can allocate before locking if have acts. */
> - reply = ovs_flow_cmd_alloc_info(acts, info, false);
> + reply = ovs_flow_cmd_alloc_info(acts, &ufid, info, false);
> if (IS_ERR(reply)) {
> error = PTR_ERR(reply);
> goto err_kfree_acts;
> @@ -1064,7 +1140,10 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> goto err_unlock_ovs;
> }
> /* Check that the flow exists. */
> - flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> + if (ufid.ufid_len)
> + flow = ovs_flow_tbl_lookup_ufid(&dp->table, &ufid);
> + else
> + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> if (unlikely(!flow)) {
> error = -ENOENT;
> goto err_unlock_ovs;
> @@ -1080,13 +1159,15 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> ovs_header->dp_ifindex,
> reply, info->snd_portid,
> info->snd_seq, 0,
> - OVS_FLOW_CMD_NEW);
> + OVS_FLOW_CMD_NEW,
> + ufid_flags);
> BUG_ON(error < 0);
> }
> } else {
> /* Could not alloc without acts before locking. */
> reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> - info, OVS_FLOW_CMD_NEW, false);
> + info, OVS_FLOW_CMD_NEW, false,
> + ufid_flags);
> if (unlikely(IS_ERR(reply))) {
> error = PTR_ERR(reply);
> goto err_unlock_ovs;
> @@ -1123,17 +1204,22 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct genl_info *info)
> struct sw_flow *flow;
> struct datapath *dp;
> struct sw_flow_match match;
> + struct sw_flow_id ufid;
> + u32 ufid_flags;
> int err;
> bool log = !a[OVS_FLOW_ATTR_PROBE];
>
> - if (!a[OVS_FLOW_ATTR_KEY]) {
> + err = ovs_nla_get_ufid(a[OVS_FLOW_ATTR_UFID], &ufid, &ufid_flags);
> + if (err)
> + return err;
> + if (a[OVS_FLOW_ATTR_KEY]) {
> + ovs_match_init(&match, &key, NULL);
> + err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL, log);
> + } else if (!ufid.ufid_len) {
> OVS_NLERR(log,
> - "Flow get message rejected, Key attribute missing.");
> - return -EINVAL;
> + "Flow get message rejected, Key attribute missing.\n");
> + err = -EINVAL;
> }
> -
> - ovs_match_init(&match, &key, NULL);
> - err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL, log);
> if (err)
> return err;
>
> @@ -1144,14 +1230,17 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct genl_info *info)
> goto unlock;
> }
>
> - flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> + if (ufid.ufid_len)
> + flow = ovs_flow_tbl_lookup_ufid(&dp->table, &ufid);
> + else
> + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> if (!flow) {
> err = -ENOENT;
> goto unlock;
> }
>
> reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex, info,
> - OVS_FLOW_CMD_NEW, true);
> + OVS_FLOW_CMD_NEW, true, ufid_flags);
> if (IS_ERR(reply)) {
> err = PTR_ERR(reply);
> goto unlock;
> @@ -1170,13 +1259,18 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> struct ovs_header *ovs_header = info->userhdr;
> struct sw_flow_key key;
> struct sk_buff *reply;
> - struct sw_flow *flow;
> + struct sw_flow *flow = NULL;
> struct datapath *dp;
> struct sw_flow_match match;
> + struct sw_flow_id ufid;
> + u32 ufid_flags;
> int err;
> bool log = !a[OVS_FLOW_ATTR_PROBE];
>
> - if (likely(a[OVS_FLOW_ATTR_KEY])) {
> + err = ovs_nla_get_ufid(a[OVS_FLOW_ATTR_UFID], &ufid, &ufid_flags);
> + if (err)
> + return err;
> + if (a[OVS_FLOW_ATTR_KEY]) {
> ovs_match_init(&match, &key, NULL);
> err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL,
> log);
> @@ -1191,12 +1285,15 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> goto unlock;
> }
>
> - if (unlikely(!a[OVS_FLOW_ATTR_KEY])) {
> + if (unlikely(!a[OVS_FLOW_ATTR_KEY] && !ufid.ufid_len)) {
> err = ovs_flow_tbl_flush(&dp->table);
> goto unlock;
> }
>
> - flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> + if (ufid.ufid_len)
> + flow = ovs_flow_tbl_lookup_ufid(&dp->table, &ufid);
> + else
> + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> if (unlikely(!flow)) {
> err = -ENOENT;
> goto unlock;
> @@ -1206,7 +1303,7 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> ovs_unlock();
>
> reply = ovs_flow_cmd_alloc_info(rcu_dereference_raw(flow->sf_acts),
> - info, false);
> + flow_get_ufid(flow), info, false);
>
> if (likely(reply)) {
> if (likely(!IS_ERR(reply))) {
> @@ -1214,7 +1311,8 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
> err = ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex,
> reply, info->snd_portid,
> info->snd_seq, 0,
> - OVS_FLOW_CMD_DEL);
> + OVS_FLOW_CMD_DEL,
> + ufid_flags);
> rcu_read_unlock();
> BUG_ON(err < 0);
> ovs_notify(&dp_flow_genl_family, &ovs_dp_flow_multicast_group, reply, info);
> @@ -1235,8 +1333,15 @@ unlock:
> static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
> {
> struct ovs_header *ovs_header = genlmsg_data(nlmsg_data(cb->nlh));
> + struct nlattr *nla, *ufid;
> struct table_instance *ti;
> struct datapath *dp;
> + u32 ufid_flags = 0;
> +
> + nla = nlmsg_attrdata(cb->nlh, sizeof(*ovs_header));
> + ufid = nla_find_nested(nla, OVS_FLOW_ATTR_UFID);
> + if (ufid && ovs_nla_get_ufid(ufid, NULL, &ufid_flags))
> + OVS_NLERR(true, "Error occurred parsing UFID flags on dump");
>
> rcu_read_lock();
> dp = get_dp_rcu(sock_net(skb->sk), ovs_header->dp_ifindex);
> @@ -1259,7 +1364,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
> if (ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex, skb,
> NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq, NLM_F_MULTI,
> - OVS_FLOW_CMD_NEW) < 0)
> + OVS_FLOW_CMD_NEW, ufid_flags) < 0)
> break;
>
> cb->args[0] = bucket;
> @@ -1275,6 +1380,7 @@ static const struct nla_policy flow_policy[OVS_FLOW_ATTR_MAX + 1] = {
> [OVS_FLOW_ATTR_ACTIONS] = { .type = NLA_NESTED },
> [OVS_FLOW_ATTR_CLEAR] = { .type = NLA_FLAG },
> [OVS_FLOW_ATTR_PROBE] = { .type = NLA_FLAG },
> + [OVS_FLOW_ATTR_UFID] = { .type = NLA_NESTED },
> };
>
> static struct genl_ops dp_flow_genl_ops[] = {
> diff --git a/datapath/flow.h b/datapath/flow.h
> index 2bbf789..736e0eb 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -196,6 +196,13 @@ struct sw_flow_match {
> struct sw_flow_mask *mask;
> };
>
> +struct sw_flow_id {
> + struct hlist_node node[2];
> + u32 hash;
> + u8 *ufid;
> + u8 ufid_len;
> +};
> +
Lets make ufid array of size 256, we can reject any key greater than
this. current patch does not support key greater than 256 anyways.
> struct sw_flow_actions {
> struct rcu_head rcu;
> u32 actions_len;
> @@ -212,13 +219,20 @@ struct flow_stats {
>
> struct sw_flow {
> struct rcu_head rcu;
> - struct hlist_node hash_node[2];
> - u32 hash;
> + struct {
> + struct hlist_node node[2];
> + u32 hash;
> + } flow_hash;
This change does not look related to this work.
> int stats_last_writer; /* NUMA-node id of the last writer on
> * 'stats[0]'.
> */
> struct sw_flow_key key;
> - struct sw_flow_key unmasked_key;
> + bool index_by_ufid; /* Which of the below that userspace
> + uses to index this flow. */
> + union {
> + struct sw_flow_key unmasked_key;
> + struct sw_flow_id ufid;
> + } index;
Rather than storing ufid or unmasked key inside struct flow we can
keep pointer to these objects, that will save some memory.
> struct sw_flow_mask *mask;
> struct sw_flow_actions __rcu *sf_acts;
> struct flow_stats __rcu *stats[]; /* One for each NUMA node. First one
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index c1c29f5..7927462 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -1094,6 +1094,41 @@ free_newmask:
> return err;
> }
>
> +int ovs_nla_get_ufid(const struct nlattr *attr, struct sw_flow_id *sfid,
> + u32 *flags)
> +{
> + static const struct nla_policy ovs_ufid_policy[OVS_UFID_ATTR_MAX + 1] = {
> + [OVS_UFID_ATTR_FLAGS] = { .type = NLA_U32 },
> + [OVS_UFID_ATTR_ID] = { .len = sizeof(u32) },
> + };
> + const struct nlattr *a[OVS_UFID_ATTR_MAX + 1];
> + int err;
> +
> + if (sfid) {
> + sfid->ufid = NULL;
> + sfid->ufid_len = 0;
> + }
> + if (flags)
> + *flags = 0;
> + if (!attr)
> + return 0;
> +
> + err = nla_parse_nested((struct nlattr **)a, OVS_UFID_ATTR_MAX, attr,
> + ovs_ufid_policy);
> + if (err)
> + return err;
> +
> + if (sfid && a[OVS_UFID_ATTR_ID]) {
> + sfid->ufid = nla_data(a[OVS_UFID_ATTR_ID]);
> + sfid->ufid_len = nla_len(a[OVS_UFID_ATTR_ID]);
> + }
> +
> + if (flags && a[OVS_UFID_ATTR_FLAGS])
> + *flags = nla_get_u32(a[OVS_UFID_ATTR_FLAGS]);
> +
> + return 0;
> +}
> +
> /**
> * ovs_nla_get_flow_metadata - parses Netlink attributes into a flow key.
> * @key: Receives extracted in_port, priority, tun_key and skb_mark.
> diff --git a/datapath/flow_netlink.h b/datapath/flow_netlink.h
> index fde7616..9a14ad9 100644
> --- a/datapath/flow_netlink.h
> +++ b/datapath/flow_netlink.h
> @@ -52,6 +52,7 @@ int ovs_nla_get_match(struct sw_flow_match *, const struct nlattr *key,
> const struct nlattr *mask, bool log);
> int ovs_nla_put_egress_tunnel_key(struct sk_buff *,
> const struct ovs_tunnel_info *);
> +int ovs_nla_get_ufid(const struct nlattr *, struct sw_flow_id *, u32 *flags);
>
> int ovs_nla_copy_actions(const struct nlattr *attr,
> const struct sw_flow_key *key,
> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> index ad410fd..03e7040 100644
> --- a/datapath/flow_table.c
> +++ b/datapath/flow_table.c
> @@ -92,6 +92,7 @@ struct sw_flow *ovs_flow_alloc(void)
>
> flow->sf_acts = NULL;
> flow->mask = NULL;
> + flow->index_by_ufid = false;
> flow->stats_last_writer = NUMA_NO_NODE;
>
> /* Initialize the default stat node. */
> @@ -146,6 +147,8 @@ static void flow_free(struct sw_flow *flow)
> {
> int node;
>
> + if (flow->index_by_ufid)
> + kfree(flow->index.ufid.ufid);
> kfree(rcu_dereference_raw(flow->sf_acts));
> for_each_node(node)
> if (flow->stats[node])
> @@ -265,7 +268,7 @@ static int tbl_mask_array_realloc(struct flow_table *tbl, int size)
>
> int ovs_flow_tbl_init(struct flow_table *table)
> {
> - struct table_instance *ti;
> + struct table_instance *ti, *ufid_ti;
> struct mask_array *ma;
>
> table->mask_cache = __alloc_percpu(sizeof(struct mask_cache_entry) *
> @@ -277,16 +280,24 @@ int ovs_flow_tbl_init(struct flow_table *table)
> if (!ma)
> goto free_mask_cache;
>
> + ufid_ti = table_instance_alloc(TBL_MIN_BUCKETS);
> + if (!ufid_ti)
> + goto free_mask_array;
> +
> ti = table_instance_alloc(TBL_MIN_BUCKETS);
> if (!ti)
> - goto free_mask_array;
> + goto free_ti;
>
> rcu_assign_pointer(table->ti, ti);
> + rcu_assign_pointer(table->ufid_ti, ufid_ti);
> rcu_assign_pointer(table->mask_array, ma);
> - table->last_rehash = jiffies;
> table->count = 0;
> + table->ufid_count = 0;
> + table->last_rehash = jiffies;
> return 0;
>
> +free_ti:
> + __table_instance_destroy(ufid_ti);
> free_mask_array:
> kfree(ma);
> free_mask_cache:
> @@ -301,13 +312,16 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head *rcu)
> __table_instance_destroy(ti);
> }
>
> -static void table_instance_destroy(struct table_instance *ti, bool deferred)
> +static void table_instance_destroy(struct table_instance *ti,
> + struct table_instance *ufid_ti,
> + bool deferred)
> {
> int i;
>
> if (!ti)
> return;
>
> + BUG_ON(!ufid_ti);
> if (ti->keep_flows)
> goto skip_flows;
>
> @@ -316,18 +330,24 @@ static void table_instance_destroy(struct table_instance *ti, bool deferred)
> struct hlist_head *head = flex_array_get(ti->buckets, i);
> struct hlist_node *n;
> int ver = ti->node_ver;
> + int ufid_ver = ufid_ti->node_ver;
>
> - hlist_for_each_entry_safe(flow, n, head, hash_node[ver]) {
> - hlist_del_rcu(&flow->hash_node[ver]);
> + hlist_for_each_entry_safe(flow, n, head, flow_hash.node[ver]) {
> + hlist_del_rcu(&flow->flow_hash.node[ver]);
> + if (flow->index_by_ufid)
> + hlist_del_rcu(&flow->index.ufid.node[ufid_ver]);
> ovs_flow_free(flow, deferred);
> }
> }
>
> skip_flows:
> - if (deferred)
> + if (deferred) {
> call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
> - else
> + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
> + } else {
> __table_instance_destroy(ti);
> + __table_instance_destroy(ufid_ti);
> + }
> }
>
> /* No need for locking this function is called from RCU callback or
> @@ -336,10 +356,11 @@ skip_flows:
> void ovs_flow_tbl_destroy(struct flow_table *table)
> {
> struct table_instance *ti = rcu_dereference_raw(table->ti);
> + struct table_instance *ufid_ti = rcu_dereference_raw(table->ufid_ti);
>
> free_percpu(table->mask_cache);
> - kfree(rcu_dereference_raw(table->mask_array));
> - table_instance_destroy(ti, false);
> + kfree((struct mask_array __force *)table->mask_array);
> + table_instance_destroy(ti, ufid_ti, false);
> }
>
> struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
> @@ -354,7 +375,7 @@ struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
> while (*bucket < ti->n_buckets) {
> i = 0;
> head = flex_array_get(ti->buckets, *bucket);
> - hlist_for_each_entry_rcu(flow, head, hash_node[ver]) {
> + hlist_for_each_entry_rcu(flow, head, flow_hash.node[ver]) {
> if (i < *last) {
> i++;
> continue;
> @@ -380,12 +401,21 @@ static void table_instance_insert(struct table_instance *ti, struct sw_flow *flo
> {
> struct hlist_head *head;
>
> - head = find_bucket(ti, flow->hash);
> - hlist_add_head_rcu(&flow->hash_node[ti->node_ver], head);
> + head = find_bucket(ti, flow->flow_hash.hash);
> + hlist_add_head_rcu(&flow->flow_hash.node[ti->node_ver], head);
> +}
> +
> +static void ufid_table_instance_insert(struct table_instance *ti,
> + struct sw_flow *flow)
> +{
> + struct hlist_head *head;
> +
> + head = find_bucket(ti, flow->index.ufid.hash);
> + hlist_add_head_rcu(&flow->index.ufid.node[ti->node_ver], head);
> }
>
> static void flow_table_copy_flows(struct table_instance *old,
> - struct table_instance *new)
> + struct table_instance *new, bool ufid)
> {
> int old_ver;
> int i;
> @@ -400,42 +430,69 @@ static void flow_table_copy_flows(struct table_instance *old,
>
> head = flex_array_get(old->buckets, i);
>
> - hlist_for_each_entry(flow, head, hash_node[old_ver])
> - table_instance_insert(new, flow);
> + if (ufid)
> + hlist_for_each_entry(flow, head,
> + index.ufid.node[old_ver])
> + ufid_table_instance_insert(new, flow);
> + else
> + hlist_for_each_entry(flow, head, flow_hash.node[old_ver])
> + table_instance_insert(new, flow);
> }
>
> old->keep_flows = true;
> }
>
> -static struct table_instance *table_instance_rehash(struct table_instance *ti,
> - int n_buckets)
> +static int flow_table_instance_alloc(struct table_instance **ti,
> + int n_buckets)
> {
> struct table_instance *new_ti;
>
> new_ti = table_instance_alloc(n_buckets);
> if (!new_ti)
> - return NULL;
> + return -ENOMEM;
>
> - flow_table_copy_flows(ti, new_ti);
> + *ti = new_ti;
> + return 0;
> +}
> +
> +static struct table_instance *flow_table_rehash(struct table_instance *old_ti,
> + int n_buckets, bool ufid)
> +{
> + struct table_instance *new_ti;
> + int err;
> +
> + err = flow_table_instance_alloc(&new_ti, n_buckets);
> + if (err)
> + return NULL;
> + flow_table_copy_flows(old_ti, new_ti, ufid);
>
> return new_ti;
> }
>
> int ovs_flow_tbl_flush(struct flow_table *flow_table)
> {
> - struct table_instance *old_ti;
> - struct table_instance *new_ti;
> + struct table_instance *old_ti, *new_ti, *old_ufid_ti;
> + struct table_instance *new_ufid_ti = NULL;
> + int err;
>
> old_ti = ovsl_dereference(flow_table->ti);
> - new_ti = table_instance_alloc(TBL_MIN_BUCKETS);
> - if (!new_ti)
> - return -ENOMEM;
> + old_ufid_ti = ovsl_dereference(flow_table->ufid_ti);
> + err = flow_table_instance_alloc(&new_ti, TBL_MIN_BUCKETS);
> + if (err)
> + return err;
> + err = flow_table_instance_alloc(&new_ufid_ti, TBL_MIN_BUCKETS);
> + if (err) {
> + __table_instance_destroy(new_ti);
> + return err;
> + }
>
> rcu_assign_pointer(flow_table->ti, new_ti);
> + rcu_assign_pointer(flow_table->ufid_ti, new_ufid_ti);
> flow_table->last_rehash = jiffies;
> flow_table->count = 0;
> + flow_table->ufid_count = 0;
>
> - table_instance_destroy(old_ti, true);
> + table_instance_destroy(old_ti, old_ufid_ti, true);
> return 0;
> }
>
> @@ -489,7 +546,8 @@ bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
> int key_start = flow_key_start(key);
> int key_end = match->range.end;
>
> - return cmp_key(&flow->unmasked_key, key, key_start, key_end);
> + BUG_ON(flow->index_by_ufid);
> + return cmp_key(&flow->index.unmasked_key, key, key_start, key_end);
> }
>
> static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
> @@ -508,10 +566,9 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
> hash = flow_hash(&masked_key, key_start, key_end);
> head = find_bucket(ti, hash);
> (*n_mask_hit)++;
> - hlist_for_each_entry_rcu(flow, head, hash_node[ti->node_ver]) {
> - if (flow->mask == mask && flow->hash == hash &&
> - flow_cmp_masked_key(flow, &masked_key,
> - key_start, key_end))
> + hlist_for_each_entry_rcu(flow, head, flow_hash.node[ti->node_ver]) {
> + if (flow->mask == mask && flow->flow_hash.hash == hash &&
> + flow_cmp_masked_key(flow, &masked_key, key_start, key_end))
> return flow;
> }
> return NULL;
> @@ -644,7 +701,40 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> if (!mask)
> continue;
> flow = masked_flow_lookup(ti, match->key, mask, &n_mask_hit);
> - if (flow && ovs_flow_cmp_unmasked_key(flow, match))
> + if (flow && !flow->index_by_ufid &&
> + ovs_flow_cmp_unmasked_key(flow, match))
> + return flow;
> + }
> + return NULL;
> +}
> +
> +static u32 ufid_hash(const struct sw_flow_id *sfid)
> +{
> + return arch_fast_hash(sfid->ufid, sfid->ufid_len, 0);
> +}
> +
> +bool ovs_flow_cmp_ufid(const struct sw_flow *flow,
> + const struct sw_flow_id *sfid)
> +{
> + if (flow->index.ufid.ufid_len != sfid->ufid_len)
> + return false;
> +
> + return !memcmp(flow->index.ufid.ufid, sfid->ufid, sfid->ufid_len);
> +}
> +
> +struct sw_flow *ovs_flow_tbl_lookup_ufid(struct flow_table *tbl,
> + const struct sw_flow_id *ufid)
> +{
> + struct table_instance *ti = rcu_dereference_ovsl(tbl->ufid_ti);
> + struct sw_flow *flow;
> + struct hlist_head *head;
> + u32 hash;
> +
> + hash = ufid_hash(ufid);
> + head = find_bucket(ti, hash);
> + hlist_for_each_entry_rcu(flow, head, index.ufid.node[ti->node_ver]) {
> + if (flow->index.ufid.hash == hash &&
> + ovs_flow_cmp_ufid(flow, ufid))
> return flow;
> }
> return NULL;
> @@ -658,9 +748,10 @@ int ovs_flow_tbl_num_masks(const struct flow_table *table)
> return ma->count;
> }
>
> -static struct table_instance *table_instance_expand(struct table_instance *ti)
> +static struct table_instance *flow_table_expand(struct table_instance *old_ti,
> + bool ufid)
> {
> - return table_instance_rehash(ti, ti->n_buckets * 2);
> + return flow_table_rehash(old_ti, old_ti->n_buckets * 2, ufid);
> }
>
> static void tbl_mask_array_delete_mask(struct mask_array *ma,
> @@ -710,10 +801,15 @@ static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
> void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow)
> {
> struct table_instance *ti = ovsl_dereference(table->ti);
> + struct table_instance *ufid_ti = ovsl_dereference(table->ufid_ti);
>
> BUG_ON(table->count == 0);
> - hlist_del_rcu(&flow->hash_node[ti->node_ver]);
> + hlist_del_rcu(&flow->flow_hash.node[ti->node_ver]);
> table->count--;
> + if (flow->index_by_ufid) {
> + hlist_del_rcu(&flow->index.ufid.node[ufid_ti->node_ver]);
> + table->ufid_count--;
> + }
>
> /* RCU delete the mask. 'flow->mask' is not NULLed, as it should be
> * accessible as long as the RCU read lock is held.
> @@ -818,31 +914,65 @@ static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
> int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
> const struct sw_flow_mask *mask)
> {
> - struct table_instance *new_ti = NULL;
> - struct table_instance *ti;
> + struct table_instance *new_ti = NULL, *new_ufid_ti = NULL;
> + struct table_instance *ti, *ufid_ti = NULL;
> int err;
>
> err = flow_mask_insert(table, flow, mask);
> if (err)
> return err;
>
> - flow->hash = flow_hash(&flow->key, flow->mask->range.start,
> - flow->mask->range.end);
> + flow->flow_hash.hash = flow_hash(&flow->key, flow->mask->range.start,
> + flow->mask->range.end);
> ti = ovsl_dereference(table->ti);
> table_instance_insert(ti, flow);
> table->count++;
> + if (flow->index_by_ufid) {
> + flow->index.ufid.hash = ufid_hash(&flow->index.ufid);
> + ufid_ti = ovsl_dereference(table->ufid_ti);
> + ufid_table_instance_insert(ufid_ti, flow);
> + table->ufid_count++;
> + }
>
> /* Expand table, if necessary, to make room. */
> if (table->count > ti->n_buckets)
> - new_ti = table_instance_expand(ti);
> + new_ti = flow_table_expand(ti, false);
> else if (time_after(jiffies, table->last_rehash + REHASH_INTERVAL))
> - new_ti = table_instance_rehash(ti, ti->n_buckets);
> + new_ti = flow_table_rehash(ti, ti->n_buckets, false);
> + if (ufid_ti && table->ufid_count > ufid_ti->n_buckets)
> + new_ufid_ti = flow_table_expand(ufid_ti, true);
>
> if (new_ti) {
> rcu_assign_pointer(table->ti, new_ti);
> - table_instance_destroy(ti, true);
> + call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
> table->last_rehash = jiffies;
> }
> + if (new_ufid_ti) {
> + rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
> + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
> + }
> + return 0;
> +}
Insert function can be simplified by first updating flow-table and
then updating ufid table.
> +
> +/* Initializes 'flow->ufid'. */
> +int ovs_flow_ufid(struct sw_flow *flow, const struct sw_flow_id *src)
> +{
> + if (src->ufid_len) {
> + struct sw_flow_id *dst = &flow->index.ufid;
> +
> + /* Use userspace-specified flow-id. */
> + dst->ufid = kmalloc(src->ufid_len, GFP_KERNEL);
> + if (!dst->ufid)
> + return -ENOMEM;
> +
> + memcpy(dst->ufid, src->ufid, src->ufid_len);
> + dst->ufid_len = src->ufid_len;
> + flow->index_by_ufid = true;
> + } else {
> + /* Don't index by UFID. */
> + flow->index_by_ufid = false;
> + }
> +
> return 0;
> }
>
> diff --git a/datapath/flow_table.h b/datapath/flow_table.h
> index 80c01a3..e05b59c 100644
> --- a/datapath/flow_table.h
> +++ b/datapath/flow_table.h
> @@ -60,8 +60,10 @@ struct flow_table {
> struct table_instance __rcu *ti;
> struct mask_cache_entry __percpu *mask_cache;
> struct mask_array __rcu *mask_array;
> + struct table_instance __rcu *ufid_ti;
> unsigned long last_rehash;
> unsigned int count;
> + unsigned int ufid_count;
> };
>
> extern struct kmem_cache *flow_stats_cache;
> @@ -91,9 +93,15 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *,
> const struct sw_flow_key *);
> struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> const struct sw_flow_match *match);
> +struct sw_flow *ovs_flow_tbl_lookup_ufid(struct flow_table *,
> + const struct sw_flow_id *);
> +
> bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
> const struct sw_flow_match *match);
> +bool ovs_flow_cmp_ufid(const struct sw_flow *flow,
> + const struct sw_flow_id *sfid);
>
> void ovs_flow_mask_key(struct sw_flow_key *dst, const struct sw_flow_key *src,
> const struct sw_flow_mask *mask);
> +int ovs_flow_ufid(struct sw_flow *flow, const struct sw_flow_id *src);
> #endif /* flow_table.h */
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index c8fa66e..7d759a4 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -471,6 +471,9 @@ struct ovs_key_nd {
> * a wildcarded match. Omitting attribute is treated as wildcarding all
> * corresponding fields. Optional for all requests. If not present,
> * all flow key bits are exact match bits.
> + * @OVS_FLOW_ATTR_UFID: Nested %OVS_UFID_ATTR_* attributes specifying unique
> + * identifiers for flows and providing alternative semantics for flow
> + * installation and retrieval.
> *
> * These attributes follow the &struct ovs_header within the Generic Netlink
> * payload for %OVS_FLOW_* commands.
> @@ -486,12 +489,39 @@ enum ovs_flow_attr {
> OVS_FLOW_ATTR_MASK, /* Sequence of OVS_KEY_ATTR_* attributes. */
> OVS_FLOW_ATTR_PROBE, /* Flow operation is a feature probe, error
> * logging should be suppressed. */
> + OVS_FLOW_ATTR_UFID, /* Unique flow identifier. */
> __OVS_FLOW_ATTR_MAX
> };
>
> #define OVS_FLOW_ATTR_MAX (__OVS_FLOW_ATTR_MAX - 1)
>
> /**
> + * enum ovs_ufid_attr - Unique identifier types.
> + *
> + * @OVS_UFID_ATTR_FLAGS: A 32-bit value specifying changes to the behaviour of
> + * the current %OVS_FLOW_CMD_* request. Optional for all requests.
> + * @OVS_UFID_ATTR_ID: A unique identifier for a flow.
> + */
> +enum ovs_ufid_attr {
> + OVS_UFID_ATTR_UNSPEC,
> + OVS_UFID_ATTR_FLAGS, /* u32 of OVS_UFID_F_* */
> + OVS_UFID_ATTR_ID, /* variable length identifier. */
> + __OVS_UFID_ATTR_MAX
> +};
> +
> +#define OVS_UFID_ATTR_MAX (__OVS_UFID_ATTR_MAX - 1)
> +
> +/**
> + * Omit attributes for notifications.
> + *
> + * If a datapath request contains an OVS_UFID_F_OMIT_* flag, then the datapath
> + * may omit the corresponding 'ovs_flow_attr' from the response.
> + */
> +#define OVS_UFID_F_OMIT_KEY (1 << 0)
> +#define OVS_UFID_F_OMIT_MASK (1 << 1)
> +#define OVS_UFID_F_OMIT_ACTIONS (1 << 2)
> +
These flags are related to flow operations. So OVS_UFID_ATTR_FLAGS
should be part of enum ovs_flow_attr.
This way we do not need to make UFID nested attr.
> +/**
> * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE action.
> * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample with
> * @OVS_ACTION_ATTR_SAMPLE. A value of 0 samples no packets, a value of
> --
> 1.7.10.4
>
^ permalink raw reply
* Re: [PATCH RFC] net: Pass full skb hash to ndo_rx_flow_steer
From: David Miller @ 2014-11-19 23:56 UTC (permalink / raw)
To: therbert; +Cc: luto, ben, netdev
In-Reply-To: <1416370137-2512-1-git-send-email-therbert@google.com>
From: Tom Herbert <therbert@google.com>
Date: Tue, 18 Nov 2014 20:08:57 -0800
> Currently, for aRFS the index into the flow table is passed to
> ndo_rx_flow_steer as the flow ID of a connection. This is the skb->hash
> & the table mask. It looks like the backend can accept the full
> skb->hash as the flow ID which should reduce the number of collisions
> in the hardware tables.
>
> This patch provides the skb->hash to the driver for flow steering.
> Expiration of HW steered flows was also updated.
>
> With a hash collision in RFS, ndo_rx_flow_steer will continue to be
> called with different CPUs, but now with different flow_ids. If this
> is still too much device interaction, then it might make sense for the
> driver to do its own lookup in its structure to see if a matching
> filter is already installed for a given flow_id, an if it is just
> refresh a timestamp to avoid expiration (based on looking at sfc
> driver).
>
> I don't currently have any HW to test this, if someone could try this
> on hardware with aRFS and provide feedback that would be appreciated.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
This seems legitimate to me.
The only thing a driver can reliably do with the flow_id is
pass it back into rps_may_expire_flow(). Therefore you can
pass it in whatever format you like.
^ permalink raw reply
* Re: [PATCH V2 net] virtio-net: validate features during probe
From: David Miller @ 2014-11-20 0:00 UTC (permalink / raw)
To: jasowang; +Cc: mst, netdev, linux-kernel, virtualization
In-Reply-To: <546C612A.4080807@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 19 Nov 2014 17:21:46 +0800
> More compact, looks good. Thanks
I am assuming there is therefore a V3 of this patch forthcoming.
^ permalink raw reply
* Re: [net-next PATCH v2] ixgbe: Remove IXGBE_FLAG_IN_NETPOLL since it doesn't do anything
From: Jeff Kirsher @ 2014-11-20 0:09 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, donald.c.skidmore
In-Reply-To: <20141119232710.19544.20012.stgit@ahduyck-vm-fedora20>
[-- Attachment #1: Type: text/plain, Size: 838 bytes --]
On Wed, 2014-11-19 at 15:28 -0800, Alexander Duyck wrote:
> This patch removes some dead code from the cleanup path for ixgbe.
>
> Setting and clearing the flag doesn't do anything since all we are
> doing is
> setting the flag, scheduling napi, clearing the flag and then letting
> netpoll do the polling cleanup. As such it doesn't make much sense to
> have
> it there.
>
> This patch also removes one minor white-space error.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>
> v2: Fixed an unused variable warning for adapter in ixgbe_rx_skb
>
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 -
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 18
> ++++--------------
> 2 files changed, 4 insertions(+), 15 deletions(-)
Thanks Alex, I will add your patch to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 net-next 0/2] bonding: Introduce 4 AD link speed
From: David Miller @ 2014-11-20 0:10 UTC (permalink / raw)
To: Jianhua.Xie; +Cc: netdev, edumazet, j.vosburgh, vfalico, andy
In-Reply-To: <1416386939-24591-1-git-send-email-Jianhua.Xie@freescale.com>
From: Xie Jianhua <Jianhua.Xie@freescale.com>
Date: Wed, 19 Nov 2014 16:48:57 +0800
> The speed field of AD Port Key was based on bitmask, it supported 5
> kinds of link speed at most, as there were only 5 bits in the speed
> field of the AD Port Key. This patches series change the speed type
> (AD_LINK_SPEED_BITMASK) from bitmask to enum type in order to enhance
> speed type from 5 to 32, and then introduce 4 AD link speed to fix
> agg_bandwidth.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: use skb_get_hash_raw instead of skb_get_hash in set_rps_cpu
From: David Miller @ 2014-11-20 0:16 UTC (permalink / raw)
To: _govind; +Cc: netdev, therbert
In-Reply-To: <1416391244-27390-1-git-send-email-_govind@gmx.com>
From: Govindarajulu Varadarajan <_govind@gmx.com>
Date: Wed, 19 Nov 2014 15:30:44 +0530
> In set_rps_cpu, we call skb_get_hash to get the hash of skb. The caller
> get_rps_cpu has already determined the hash of skb by calling skb_get_hash.
>
> Since get_rps_cpu is the only caller of set_rps_cpu, we can use skb_get_hash_raw
>
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
Tom, please review. And if you agree with this change, you'll need to
be spinning your flow_id change relative to this I guess since it overlaps.
> ---
> net/core/dev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1ab168e..b4ad4d1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3055,7 +3055,7 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
> flow_table = rcu_dereference(rxqueue->rps_flow_table);
> if (!flow_table)
> goto out;
> - flow_id = skb_get_hash(skb) & flow_table->mask;
> + flow_id = skb_get_hash_raw(skb) & flow_table->mask;
> rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
> rxq_index, flow_id);
> if (rc < 0)
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH RFC] net: Pass full skb hash to ndo_rx_flow_steer
From: Andy Lutomirski @ 2014-11-20 0:21 UTC (permalink / raw)
To: Tom Herbert; +Cc: Ben Hutchings, Network Development
In-Reply-To: <1416370137-2512-1-git-send-email-therbert@google.com>
On Tue, Nov 18, 2014 at 8:08 PM, Tom Herbert <therbert@google.com> wrote:
> Currently, for aRFS the index into the flow table is passed to
> ndo_rx_flow_steer as the flow ID of a connection. This is the skb->hash
> & the table mask. It looks like the backend can accept the full
> skb->hash as the flow ID which should reduce the number of collisions
> in the hardware tables.
>
> This patch provides the skb->hash to the driver for flow steering.
> Expiration of HW steered flows was also updated.
>
> With a hash collision in RFS, ndo_rx_flow_steer will continue to be
> called with different CPUs, but now with different flow_ids. If this
> is still too much device interaction, then it might make sense for the
> driver to do its own lookup in its structure to see if a matching
> filter is already installed for a given flow_id, an if it is just
> refresh a timestamp to avoid expiration (based on looking at sfc
> driver).
>
> I don't currently have any HW to test this, if someone could try this
> on hardware with aRFS and provide feedback that would be appreciated.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> net/core/dev.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1ab168e..cb1e06d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3057,7 +3057,8 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
> goto out;
> flow_id = skb_get_hash(skb) & flow_table->mask;
> rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
> - rxq_index, flow_id);
> + rxq_index,
> + skb_get_hash(skb));
Can gcc CSE this? Not that it matters that much.
> if (rc < 0)
> goto out;
> old_rflow = rflow;
> @@ -3195,8 +3196,8 @@ bool rps_may_expire_flow(struct net_device *dev, u16 rxq_index,
>
> rcu_read_lock();
> flow_table = rcu_dereference(rxqueue->rps_flow_table);
> - if (flow_table && flow_id <= flow_table->mask) {
> - rflow = &flow_table->flows[flow_id];
> + if (flow_table) {
> + rflow = &flow_table->flows[flow_id & flow_table->mask];
I think this is nicer, but why will it help? If there's a collision
in the low bits of the hash, we'll still think that the flow is
expired.
Is there a real LRU-ish hash table that could be subbed in easily?
There ought to be a data structure that's barely more complicated than
this kind of hash table but that gives much better behavior when the
number of flows is smaller than the table size.
--Andy
> cpu = ACCESS_ONCE(rflow->cpu);
> if (rflow->filter == filter_id && cpu != RPS_NO_CPU &&
> ((int)(per_cpu(softnet_data, cpu).input_queue_head -
> --
> 2.1.0.rc2.206.gedb03e5
>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCHv10 ovs 12/15] datapath: Add support for unique flow identifiers.
From: Joe Stringer @ 2014-11-20 0:20 UTC (permalink / raw)
To: Pravin Shelar; +Cc: dev@openvswitch.org, netdev
In-Reply-To: <CALnjE+owCheaKK8=JBmW0whpC1_iP67XfnaGCghi4jSWYaDfkg@mail.gmail.com>
On Wednesday, November 19, 2014 15:34:24 Pravin Shelar wrote:
> On Thu, Nov 13, 2014 at 11:17 AM, Joe Stringer <joestringer@nicira.com>
wrote:
> > @@ -684,33 +691,43 @@ static size_t ovs_flow_cmd_msg_size(const struct
> > sw_flow_actions *acts)
> >
> > /* Called with ovs_mutex or RCU read lock. */
> > static int ovs_flow_cmd_fill_match(const struct sw_flow *flow,
> >
> > - struct sk_buff *skb)
> > + struct sk_buff *skb, u32 ufid_flags)
> >
> > {
> >
> > struct nlattr *nla;
> > int err;
> >
> > - /* Fill flow key. */
> > - nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
> > - if (!nla)
> > - return -EMSGSIZE;
> > -
> > - err = ovs_nla_put_flow(&flow->unmasked_key, &flow->unmasked_key,
> > skb, - false);
> > - if (err)
> > - return err;
> > -
> > - nla_nest_end(skb, nla);
> > + /* Fill flow key. If userspace didn't specify a UFID, then ignore
> > the + * OMIT_KEY flag. */
> > + if (!(ufid_flags & OVS_UFID_F_OMIT_KEY) ||
> > + !flow->index_by_ufid) {
>
> I am not sure about this check, userspace needs to send atleast ufid
> or the unmasked key as id for flow. otherwise we shld flag error. Here
> we can serialize flow->key.
> There could be another function which takes care of flow-id
> serialization where we serialize use ufid or unmasked key as flow id.
> Lets group ufid and unmasked key together rather than masked key and
> unmasked key which are not related.
Right, at flow setup time the flow key is always required, but the UFID is
optional. For most other cases, one of the two most be specified. For flow dump,
neither is required from userspace, but OMIT_KEY flag may be raised. That's the
particular case that this logic is trying to catch (dump all flows, including
those that were set up without UFID - in which case the OMIT_KEY flag doesn't
make sense, so treat the flag like a request rather than a command).
Happy to split the key/identifier out from the mask.
> > @@ -740,6 +757,32 @@ static int ovs_flow_cmd_fill_stats(const struct
> > sw_flow *flow,
> >
> > }
> >
> > /* Called with ovs_mutex or RCU read lock. */
> >
> > +static int ovs_flow_cmd_fill_ufid(const struct sw_flow *flow,
> > + struct sk_buff *skb)
> > +{
> > + struct nlattr *start;
> > + const struct sw_flow_id *sfid;
> > +
> > + if (!flow->index_by_ufid)
> > + return 0;
> > +
> > + sfid = &flow->index.ufid;
> > + start = nla_nest_start(skb, OVS_FLOW_ATTR_UFID);
> > + if (start) {
> > + int err;
> > +
> > + err = nla_put(skb, OVS_UFID_ATTR_ID, sfid->ufid_len,
> > + sfid->ufid);
> > + if (err)
> > + return err;
> > + nla_nest_end(skb, start);
> > + } else
> > + return -EMSGSIZE;
> > +
> > + return 0;
> > +}
> > +
>
> Can you change this function according to comments above?
OK.
> > @@ -795,18 +838,24 @@ static int ovs_flow_cmd_fill_info(const struct
> > sw_flow *flow, int dp_ifindex,
> >
> > ovs_header->dp_ifindex = dp_ifindex;
> >
> > - err = ovs_flow_cmd_fill_match(flow, skb);
> > + err = ovs_flow_cmd_fill_match(flow, skb, ufid_flags);
> >
> > if (err)
> >
> > goto error;
> >
> > - err = ovs_flow_cmd_fill_stats(flow, skb);
> > + err = ovs_flow_cmd_fill_ufid(flow, skb);
> >
> > if (err)
> >
> > goto error;
>
> Flow ID should go first in the netlink msg.
OK.
> > @@ -915,8 +980,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb,
> > struct genl_info *info)
> >
> > error = -ENODEV;
> > goto err_unlock_ovs;
> >
> > }
> >
> > +
> >
> > /* Check if this is a duplicate flow */
> >
> > - flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->unmasked_key);
> > + flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
>
> Need to check for ufid table to find duplicate ufid entry here.
OK.
> > diff --git a/datapath/flow.h b/datapath/flow.h
> > index 2bbf789..736e0eb 100644
> > --- a/datapath/flow.h
> > +++ b/datapath/flow.h
> > @@ -196,6 +196,13 @@ struct sw_flow_match {
> >
> > struct sw_flow_mask *mask;
> >
> > };
> >
> > +struct sw_flow_id {
> > + struct hlist_node node[2];
> > + u32 hash;
> > + u8 *ufid;
> > + u8 ufid_len;
> > +};
> > +
>
> Lets make ufid array of size 256, we can reject any key greater than
> this. current patch does not support key greater than 256 anyways.
Ok, that sounds reasonable.
> > struct sw_flow_actions {
> >
> > struct rcu_head rcu;
> > u32 actions_len;
> >
> > @@ -212,13 +219,20 @@ struct flow_stats {
> >
> > struct sw_flow {
> >
> > struct rcu_head rcu;
> >
> > - struct hlist_node hash_node[2];
> > - u32 hash;
> > + struct {
> > + struct hlist_node node[2];
> > + u32 hash;
> > + } flow_hash;
>
> This change does not look related to this work.
Right, this is unnecessary leftover from earlier iteration.
> > int stats_last_writer; /* NUMA-node id of the last
> > writer on
> >
> > * 'stats[0]'.
> > */
> >
> > struct sw_flow_key key;
> >
> > - struct sw_flow_key unmasked_key;
> > + bool index_by_ufid; /* Which of the below that
> > userspace + uses to index this
> > flow. */ + union {
> > + struct sw_flow_key unmasked_key;
> > + struct sw_flow_id ufid;
> > + } index;
>
> Rather than storing ufid or unmasked key inside struct flow we can
> keep pointer to these objects, that will save some memory.
OK. Do you still care about the union being there?
> > @@ -818,31 +914,65 @@ static int flow_mask_insert(struct flow_table *tbl,
> > struct sw_flow *flow,
> >
> > int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
> >
> > const struct sw_flow_mask *mask)
> >
> > {
> >
> > - struct table_instance *new_ti = NULL;
> > - struct table_instance *ti;
> > + struct table_instance *new_ti = NULL, *new_ufid_ti = NULL;
> > + struct table_instance *ti, *ufid_ti = NULL;
> >
> > int err;
> >
> > err = flow_mask_insert(table, flow, mask);
> > if (err)
> >
> > return err;
> >
> > - flow->hash = flow_hash(&flow->key, flow->mask->range.start,
> > - flow->mask->range.end);
> > + flow->flow_hash.hash = flow_hash(&flow->key,
> > flow->mask->range.start, +
> > flow->mask->range.end);
> >
> > ti = ovsl_dereference(table->ti);
> > table_instance_insert(ti, flow);
> > table->count++;
> >
> > + if (flow->index_by_ufid) {
> > + flow->index.ufid.hash = ufid_hash(&flow->index.ufid);
> > + ufid_ti = ovsl_dereference(table->ufid_ti);
> > + ufid_table_instance_insert(ufid_ti, flow);
> > + table->ufid_count++;
> > + }
> >
> > /* Expand table, if necessary, to make room. */
> > if (table->count > ti->n_buckets)
> >
> > - new_ti = table_instance_expand(ti);
> > + new_ti = flow_table_expand(ti, false);
> >
> > else if (time_after(jiffies, table->last_rehash +
> > REHASH_INTERVAL))
> >
> > - new_ti = table_instance_rehash(ti, ti->n_buckets);
> > + new_ti = flow_table_rehash(ti, ti->n_buckets, false);
> > + if (ufid_ti && table->ufid_count > ufid_ti->n_buckets)
> > + new_ufid_ti = flow_table_expand(ufid_ti, true);
> >
> > if (new_ti) {
> >
> > rcu_assign_pointer(table->ti, new_ti);
> >
> > - table_instance_destroy(ti, true);
> > + call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
> >
> > table->last_rehash = jiffies;
> >
> > }
> >
> > + if (new_ufid_ti) {
> > + rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
> > + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
> > + }
> > + return 0;
> > +}
>
> Insert function can be simplified by first updating flow-table and
> then updating ufid table.
Sure.
> > #define OVS_FLOW_ATTR_MAX (__OVS_FLOW_ATTR_MAX - 1)
> >
> > /**
> >
> > + * enum ovs_ufid_attr - Unique identifier types.
> > + *
> > + * @OVS_UFID_ATTR_FLAGS: A 32-bit value specifying changes to the
> > behaviour of + * the current %OVS_FLOW_CMD_* request. Optional for all
> > requests. + * @OVS_UFID_ATTR_ID: A unique identifier for a flow.
> > + */
> > +enum ovs_ufid_attr {
> > + OVS_UFID_ATTR_UNSPEC,
> > + OVS_UFID_ATTR_FLAGS, /* u32 of OVS_UFID_F_* */
> > + OVS_UFID_ATTR_ID, /* variable length identifier. */
> > + __OVS_UFID_ATTR_MAX
> > +};
> > +
> > +#define OVS_UFID_ATTR_MAX (__OVS_UFID_ATTR_MAX - 1)
> > +
> > +/**
> > + * Omit attributes for notifications.
> > + *
> > + * If a datapath request contains an OVS_UFID_F_OMIT_* flag, then the
> > datapath + * may omit the corresponding 'ovs_flow_attr' from the
> > response. + */
> > +#define OVS_UFID_F_OMIT_KEY (1 << 0)
> > +#define OVS_UFID_F_OMIT_MASK (1 << 1)
> > +#define OVS_UFID_F_OMIT_ACTIONS (1 << 2)
> > +
>
> These flags are related to flow operations. So OVS_UFID_ATTR_FLAGS
> should be part of enum ovs_flow_attr.
> This way we do not need to make UFID nested attr.
OK, I'll shift it out.
Thanks for the review, I'll work on a fresh revision.
^ permalink raw reply
* Re: [PATCHv10 ovs 12/15] datapath: Add support for unique flow identifiers.
From: Pravin Shelar @ 2014-11-20 0:39 UTC (permalink / raw)
To: Joe Stringer; +Cc: dev@openvswitch.org, netdev
In-Reply-To: <201411191620.32217.joestringer@nicira.com>
On Wed, Nov 19, 2014 at 4:20 PM, Joe Stringer <joestringer@nicira.com> wrote:
> On Wednesday, November 19, 2014 15:34:24 Pravin Shelar wrote:
>> On Thu, Nov 13, 2014 at 11:17 AM, Joe Stringer <joestringer@nicira.com>
> wrote:
>> > @@ -684,33 +691,43 @@ static size_t ovs_flow_cmd_msg_size(const struct
>> > sw_flow_actions *acts)
>> >
>> > /* Called with ovs_mutex or RCU read lock. */
>> > static int ovs_flow_cmd_fill_match(const struct sw_flow *flow,
>> >
>> > - struct sk_buff *skb)
>> > + struct sk_buff *skb, u32 ufid_flags)
>> >
>> > {
>> >
>> > struct nlattr *nla;
>> > int err;
>> >
>> > - /* Fill flow key. */
>> > - nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
>> > - if (!nla)
>> > - return -EMSGSIZE;
>> > -
>> > - err = ovs_nla_put_flow(&flow->unmasked_key, &flow->unmasked_key,
>> > skb, - false);
>> > - if (err)
>> > - return err;
>> > -
>> > - nla_nest_end(skb, nla);
>> > + /* Fill flow key. If userspace didn't specify a UFID, then ignore
>> > the + * OMIT_KEY flag. */
>> > + if (!(ufid_flags & OVS_UFID_F_OMIT_KEY) ||
>> > + !flow->index_by_ufid) {
>>
>> I am not sure about this check, userspace needs to send atleast ufid
>> or the unmasked key as id for flow. otherwise we shld flag error. Here
>> we can serialize flow->key.
>> There could be another function which takes care of flow-id
>> serialization where we serialize use ufid or unmasked key as flow id.
>> Lets group ufid and unmasked key together rather than masked key and
>> unmasked key which are not related.
>
> Right, at flow setup time the flow key is always required, but the UFID is
> optional. For most other cases, one of the two most be specified. For flow dump,
> neither is required from userspace, but OMIT_KEY flag may be raised. That's the
> particular case that this logic is trying to catch (dump all flows, including
> those that were set up without UFID - in which case the OMIT_KEY flag doesn't
> make sense, so treat the flag like a request rather than a command).
>
How do you handle overlapping flows without the flow id in dump operation?
> Happy to split the key/identifier out from the mask.
>
>> > @@ -740,6 +757,32 @@ static int ovs_flow_cmd_fill_stats(const struct
>> > sw_flow *flow,
>> >
>> > }
>> >
>> > /* Called with ovs_mutex or RCU read lock. */
>> >
>> > +static int ovs_flow_cmd_fill_ufid(const struct sw_flow *flow,
>> > + struct sk_buff *skb)
>> > +{
>> > + struct nlattr *start;
>> > + const struct sw_flow_id *sfid;
>> > +
>> > + if (!flow->index_by_ufid)
>> > + return 0;
>> > +
>> > + sfid = &flow->index.ufid;
>> > + start = nla_nest_start(skb, OVS_FLOW_ATTR_UFID);
>> > + if (start) {
>> > + int err;
>> > +
>> > + err = nla_put(skb, OVS_UFID_ATTR_ID, sfid->ufid_len,
>> > + sfid->ufid);
>> > + if (err)
>> > + return err;
>> > + nla_nest_end(skb, start);
>> > + } else
>> > + return -EMSGSIZE;
>> > +
>> > + return 0;
>> > +}
>> > +
>>
>> Can you change this function according to comments above?
>
> OK.
>
>> > @@ -795,18 +838,24 @@ static int ovs_flow_cmd_fill_info(const struct
>> > sw_flow *flow, int dp_ifindex,
>> >
>> > ovs_header->dp_ifindex = dp_ifindex;
>> >
>> > - err = ovs_flow_cmd_fill_match(flow, skb);
>> > + err = ovs_flow_cmd_fill_match(flow, skb, ufid_flags);
>> >
>> > if (err)
>> >
>> > goto error;
>> >
>> > - err = ovs_flow_cmd_fill_stats(flow, skb);
>> > + err = ovs_flow_cmd_fill_ufid(flow, skb);
>> >
>> > if (err)
>> >
>> > goto error;
>>
>> Flow ID should go first in the netlink msg.
>
> OK.
>
>> > @@ -915,8 +980,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb,
>> > struct genl_info *info)
>> >
>> > error = -ENODEV;
>> > goto err_unlock_ovs;
>> >
>> > }
>> >
>> > +
>> >
>> > /* Check if this is a duplicate flow */
>> >
>> > - flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->unmasked_key);
>> > + flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
>>
>> Need to check for ufid table to find duplicate ufid entry here.
>
> OK.
>
>> > diff --git a/datapath/flow.h b/datapath/flow.h
>> > index 2bbf789..736e0eb 100644
>> > --- a/datapath/flow.h
>> > +++ b/datapath/flow.h
>> > @@ -196,6 +196,13 @@ struct sw_flow_match {
>> >
>> > struct sw_flow_mask *mask;
>> >
>> > };
>> >
>> > +struct sw_flow_id {
>> > + struct hlist_node node[2];
>> > + u32 hash;
>> > + u8 *ufid;
>> > + u8 ufid_len;
>> > +};
>> > +
>>
>> Lets make ufid array of size 256, we can reject any key greater than
>> this. current patch does not support key greater than 256 anyways.
>
> Ok, that sounds reasonable.
>
>> > struct sw_flow_actions {
>> >
>> > struct rcu_head rcu;
>> > u32 actions_len;
>> >
>> > @@ -212,13 +219,20 @@ struct flow_stats {
>> >
>> > struct sw_flow {
>> >
>> > struct rcu_head rcu;
>> >
>> > - struct hlist_node hash_node[2];
>> > - u32 hash;
>> > + struct {
>> > + struct hlist_node node[2];
>> > + u32 hash;
>> > + } flow_hash;
>>
>> This change does not look related to this work.
>
> Right, this is unnecessary leftover from earlier iteration.
>
>
>> > int stats_last_writer; /* NUMA-node id of the last
>> > writer on
>> >
>> > * 'stats[0]'.
>> > */
>> >
>> > struct sw_flow_key key;
>> >
>> > - struct sw_flow_key unmasked_key;
>> > + bool index_by_ufid; /* Which of the below that
>> > userspace + uses to index this
>> > flow. */ + union {
>> > + struct sw_flow_key unmasked_key;
>> > + struct sw_flow_id ufid;
>> > + } index;
>>
>> Rather than storing ufid or unmasked key inside struct flow we can
>> keep pointer to these objects, that will save some memory.
>
> OK. Do you still care about the union being there?
>
Lets keep both pointer so that we can get rid of the index_by_fid flag.
>> > @@ -818,31 +914,65 @@ static int flow_mask_insert(struct flow_table *tbl,
>> > struct sw_flow *flow,
>> >
>> > int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
>> >
>> > const struct sw_flow_mask *mask)
>> >
>> > {
>> >
>> > - struct table_instance *new_ti = NULL;
>> > - struct table_instance *ti;
>> > + struct table_instance *new_ti = NULL, *new_ufid_ti = NULL;
>> > + struct table_instance *ti, *ufid_ti = NULL;
>> >
>> > int err;
>> >
>> > err = flow_mask_insert(table, flow, mask);
>> > if (err)
>> >
>> > return err;
>> >
>> > - flow->hash = flow_hash(&flow->key, flow->mask->range.start,
>> > - flow->mask->range.end);
>> > + flow->flow_hash.hash = flow_hash(&flow->key,
>> > flow->mask->range.start, +
>> > flow->mask->range.end);
>> >
>> > ti = ovsl_dereference(table->ti);
>> > table_instance_insert(ti, flow);
>> > table->count++;
>> >
>> > + if (flow->index_by_ufid) {
>> > + flow->index.ufid.hash = ufid_hash(&flow->index.ufid);
>> > + ufid_ti = ovsl_dereference(table->ufid_ti);
>> > + ufid_table_instance_insert(ufid_ti, flow);
>> > + table->ufid_count++;
>> > + }
>> >
>> > /* Expand table, if necessary, to make room. */
>> > if (table->count > ti->n_buckets)
>> >
>> > - new_ti = table_instance_expand(ti);
>> > + new_ti = flow_table_expand(ti, false);
>> >
>> > else if (time_after(jiffies, table->last_rehash +
>> > REHASH_INTERVAL))
>> >
>> > - new_ti = table_instance_rehash(ti, ti->n_buckets);
>> > + new_ti = flow_table_rehash(ti, ti->n_buckets, false);
>> > + if (ufid_ti && table->ufid_count > ufid_ti->n_buckets)
>> > + new_ufid_ti = flow_table_expand(ufid_ti, true);
>> >
>> > if (new_ti) {
>> >
>> > rcu_assign_pointer(table->ti, new_ti);
>> >
>> > - table_instance_destroy(ti, true);
>> > + call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
>> >
>> > table->last_rehash = jiffies;
>> >
>> > }
>> >
>> > + if (new_ufid_ti) {
>> > + rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
>> > + call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
>> > + }
>> > + return 0;
>> > +}
>>
>> Insert function can be simplified by first updating flow-table and
>> then updating ufid table.
>
> Sure.
>
>
>> > #define OVS_FLOW_ATTR_MAX (__OVS_FLOW_ATTR_MAX - 1)
>> >
>> > /**
>> >
>> > + * enum ovs_ufid_attr - Unique identifier types.
>> > + *
>> > + * @OVS_UFID_ATTR_FLAGS: A 32-bit value specifying changes to the
>> > behaviour of + * the current %OVS_FLOW_CMD_* request. Optional for all
>> > requests. + * @OVS_UFID_ATTR_ID: A unique identifier for a flow.
>> > + */
>> > +enum ovs_ufid_attr {
>> > + OVS_UFID_ATTR_UNSPEC,
>> > + OVS_UFID_ATTR_FLAGS, /* u32 of OVS_UFID_F_* */
>> > + OVS_UFID_ATTR_ID, /* variable length identifier. */
>> > + __OVS_UFID_ATTR_MAX
>> > +};
>> > +
>> > +#define OVS_UFID_ATTR_MAX (__OVS_UFID_ATTR_MAX - 1)
>> > +
>> > +/**
>> > + * Omit attributes for notifications.
>> > + *
>> > + * If a datapath request contains an OVS_UFID_F_OMIT_* flag, then the
>> > datapath + * may omit the corresponding 'ovs_flow_attr' from the
>> > response. + */
>> > +#define OVS_UFID_F_OMIT_KEY (1 << 0)
>> > +#define OVS_UFID_F_OMIT_MASK (1 << 1)
>> > +#define OVS_UFID_F_OMIT_ACTIONS (1 << 2)
>> > +
>>
>> These flags are related to flow operations. So OVS_UFID_ATTR_FLAGS
>> should be part of enum ovs_flow_attr.
>> This way we do not need to make UFID nested attr.
>
> OK, I'll shift it out.
>
> Thanks for the review, I'll work on a fresh revision.
^ permalink raw reply
* Re: [PATCH net-next] net: use skb_get_hash_raw instead of skb_get_hash in set_rps_cpu
From: Tom Herbert @ 2014-11-20 0:51 UTC (permalink / raw)
To: David Miller; +Cc: _govind, Linux Netdev List
In-Reply-To: <20141119.191610.1447608233879904907.davem@davemloft.net>
On Wed, Nov 19, 2014 at 4:16 PM, David Miller <davem@davemloft.net> wrote:
> From: Govindarajulu Varadarajan <_govind@gmx.com>
> Date: Wed, 19 Nov 2014 15:30:44 +0530
>
>> In set_rps_cpu, we call skb_get_hash to get the hash of skb. The caller
>> get_rps_cpu has already determined the hash of skb by calling skb_get_hash.
>>
>> Since get_rps_cpu is the only caller of set_rps_cpu, we can use skb_get_hash_raw
>>
>> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
>
> Tom, please review. And if you agree with this change, you'll need to
> be spinning your flow_id change relative to this I guess since it overlaps.
>
We should probably pass hash as an argument in set_rps_cpu. It's only
called from once place and the rflow is specifically chosen based on
hash computed by caller-- don't want any ambiguity there.
>> ---
>> net/core/dev.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 1ab168e..b4ad4d1 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3055,7 +3055,7 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>> flow_table = rcu_dereference(rxqueue->rps_flow_table);
>> if (!flow_table)
>> goto out;
>> - flow_id = skb_get_hash(skb) & flow_table->mask;
>> + flow_id = skb_get_hash_raw(skb) & flow_table->mask;
>> rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
>> rxq_index, flow_id);
>> if (rc < 0)
>> --
>> 2.1.0
>>
^ permalink raw reply
* [PATCH net-next v2] net: sctp: keep owned chunk in destructor_arg instead of skb->cb
From: Daniel Borkmann @ 2014-11-20 0:54 UTC (permalink / raw)
To: davem; +Cc: linux-sctp, netdev
It's just silly to hold the skb destructor argument around inside
skb->cb[] as we currently do in SCTP.
Nowadays, we're sort of cheating on data accounting in the sense
that due to commit 4c3a5bdae293 ("sctp: Don't charge for data in
sndbuf again when transmitting packet"), we orphan the skb already
in the SCTP output path, i.e. giving back charged data memory, and
use a different destructor only to make sure the sk doesn't vanish
on skb destruction time. Thus, cb[] is still valid here as we
operate within the SCTP layer. (It's generally actually a big
candidate for future rework, imho.)
However, storing the destructor in the cb[] can easily cause issues
should an non sctp_packet_set_owner_w()'ed skb ever escape the SCTP
layer, since cb[] may get overwritten by lower layers and thus can
corrupt the chunk pointer. There are no such issues at present,
but lets keep the chunk in destructor_arg, as this is the actual
purpose for it.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
v1->v2:
- Only reworded commit message to make it more clear
net/sctp/socket.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 2120292..85e0b65 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -162,7 +162,7 @@ static inline void sctp_set_owner_w(struct sctp_chunk *chunk)
chunk->skb->destructor = sctp_wfree;
/* Save the chunk pointer in skb for sctp_wfree to use later. */
- *((struct sctp_chunk **)(chunk->skb->cb)) = chunk;
+ skb_shinfo(chunk->skb)->destructor_arg = chunk;
asoc->sndbuf_used += SCTP_DATA_SNDSIZE(chunk) +
sizeof(struct sk_buff) +
@@ -6870,14 +6870,10 @@ static void sctp_wake_up_waiters(struct sock *sk,
*/
static void sctp_wfree(struct sk_buff *skb)
{
- struct sctp_association *asoc;
- struct sctp_chunk *chunk;
- struct sock *sk;
+ struct sctp_chunk *chunk = skb_shinfo(skb)->destructor_arg;
+ struct sctp_association *asoc = chunk->asoc;
+ struct sock *sk = asoc->base.sk;
- /* Get the saved chunk pointer. */
- chunk = *((struct sctp_chunk **)(skb->cb));
- asoc = chunk->asoc;
- sk = asoc->base.sk;
asoc->sndbuf_used -= SCTP_DATA_SNDSIZE(chunk) +
sizeof(struct sk_buff) +
sizeof(struct sctp_chunk);
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH RFC] net: Pass full skb hash to ndo_rx_flow_steer
From: Tom Herbert @ 2014-11-20 1:04 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Ben Hutchings, Network Development
In-Reply-To: <CALCETrU9QXUy5EK5JjmqXo50ZqdHRRbsz3eJzLuCJgadN2jPHw@mail.gmail.com>
On Wed, Nov 19, 2014 at 4:21 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Nov 18, 2014 at 8:08 PM, Tom Herbert <therbert@google.com> wrote:
>> Currently, for aRFS the index into the flow table is passed to
>> ndo_rx_flow_steer as the flow ID of a connection. This is the skb->hash
>> & the table mask. It looks like the backend can accept the full
>> skb->hash as the flow ID which should reduce the number of collisions
>> in the hardware tables.
>>
>> This patch provides the skb->hash to the driver for flow steering.
>> Expiration of HW steered flows was also updated.
>>
>> With a hash collision in RFS, ndo_rx_flow_steer will continue to be
>> called with different CPUs, but now with different flow_ids. If this
>> is still too much device interaction, then it might make sense for the
>> driver to do its own lookup in its structure to see if a matching
>> filter is already installed for a given flow_id, an if it is just
>> refresh a timestamp to avoid expiration (based on looking at sfc
>> driver).
>>
>> I don't currently have any HW to test this, if someone could try this
>> on hardware with aRFS and provide feedback that would be appreciated.
>>
>> Signed-off-by: Tom Herbert <therbert@google.com>
>> ---
>> net/core/dev.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 1ab168e..cb1e06d 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3057,7 +3057,8 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
>> goto out;
>> flow_id = skb_get_hash(skb) & flow_table->mask;
>> rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
>> - rxq_index, flow_id);
>> + rxq_index,
>> + skb_get_hash(skb));
>
> Can gcc CSE this? Not that it matters that much.
>
>> if (rc < 0)
>> goto out;
>> old_rflow = rflow;
>> @@ -3195,8 +3196,8 @@ bool rps_may_expire_flow(struct net_device *dev, u16 rxq_index,
>>
>> rcu_read_lock();
>> flow_table = rcu_dereference(rxqueue->rps_flow_table);
>> - if (flow_table && flow_id <= flow_table->mask) {
>> - rflow = &flow_table->flows[flow_id];
>> + if (flow_table) {
>> + rflow = &flow_table->flows[flow_id & flow_table->mask];
>
> I think this is nicer, but why will it help? If there's a collision
> in the low bits of the hash, we'll still think that the flow is
> expired.
>
> Is there a real LRU-ish hash table that could be subbed in easily?
I think this does exist in sfc, or at least could potentially be
updated to do it. When a filter is being inserted, there is a lookup
on the spec to see if a matching filter already exists. If it is
found, -EBUSY is returned. I "think" that instead of returning -EBUSY,
the filter index could be returned and a timestamp in the soft data
structure could be refreshed. In the driver expiration code, the
timestamp could be checked before deciding to call
rps_may_expire_flow. In the case of a collision in the RPS flow table,
ndo_rx_flow_steer would be alternately called for the flows which
should continuously refresh the timestamps of the filters for each
flow. This should keep both flow filters active in the device, without
needing to whack the device for each call to ndo_rx_flow_steer.
> There ought to be a data structure that's barely more complicated than
> this kind of hash table but that gives much better behavior when the
> number of flows is smaller than the table size.
>
> --Andy
>
>> cpu = ACCESS_ONCE(rflow->cpu);
>> if (rflow->filter == filter_id && cpu != RPS_NO_CPU &&
>> ((int)(per_cpu(softnet_data, cpu).input_queue_head -
>> --
>> 2.1.0.rc2.206.gedb03e5
>>
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH 1/1] netfilter: Deletion of unnecessary checks before two function calls
From: Simon Horman @ 2014-11-20 1:13 UTC (permalink / raw)
To: Julian Anastasov
Cc: SF Markus Elfring, David S. Miller, Jozsef Kadlecsik,
Pablo Neira Ayuso, Patrick McHardy, Wensong Zhang, netdev,
lvs-devel, netfilter-devel, coreteam, LKML, kernel-janitors,
Coccinelle
In-Reply-To: <alpine.LFD.2.11.1411200024280.6497@ja.home.ssi.bg>
On Thu, Nov 20, 2014 at 12:26:56AM +0200, Julian Anastasov wrote:
>
> Hello,
>
> On Tue, 18 Nov 2014, SF Markus Elfring wrote:
>
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Tue, 18 Nov 2014 20:37:05 +0100
> >
> > The functions free_percpu() and module_put() test whether their argument
> > is NULL and then return immediately. Thus the test around the call is
> > not needed.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>
> Pablo, the IPVS parts look ok to me,
>
> Acked-by: Julian Anastasov <ja@ssi.bg>
Acked-by: Simon Horman <horms@verge.net.au>
>
> > ---
> > net/netfilter/ipvs/ip_vs_ctl.c | 3 +--
> > net/netfilter/ipvs/ip_vs_pe.c | 3 +--
> > net/netfilter/ipvs/ip_vs_sched.c | 3 +--
> > net/netfilter/ipvs/ip_vs_sync.c | 3 +--
> > net/netfilter/nf_tables_api.c | 3 +--
> > 5 files changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> > index fd3f444..7c5e40a 100644
> > --- a/net/netfilter/ipvs/ip_vs_ctl.c
> > +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> > @@ -465,8 +465,7 @@ __ip_vs_bind_svc(struct ip_vs_dest *dest, struct ip_vs_service *svc)
> >
> > static void ip_vs_service_free(struct ip_vs_service *svc)
> > {
> > - if (svc->stats.cpustats)
> > - free_percpu(svc->stats.cpustats);
> > + free_percpu(svc->stats.cpustats);
> > kfree(svc);
> > }
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c
> > index 1a82b29..0df17ca 100644
> > --- a/net/netfilter/ipvs/ip_vs_pe.c
> > +++ b/net/netfilter/ipvs/ip_vs_pe.c
> > @@ -37,8 +37,7 @@ struct ip_vs_pe *__ip_vs_pe_getbyname(const char *pe_name)
> > rcu_read_unlock();
> > return pe;
> > }
> > - if (pe->module)
> > - module_put(pe->module);
> > + module_put(pe->module);
> > }
> > rcu_read_unlock();
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_sched.c b/net/netfilter/ipvs/ip_vs_sched.c
> > index 4dbcda6..199760c 100644
> > --- a/net/netfilter/ipvs/ip_vs_sched.c
> > +++ b/net/netfilter/ipvs/ip_vs_sched.c
> > @@ -104,8 +104,7 @@ static struct ip_vs_scheduler *ip_vs_sched_getbyname(const char *sched_name)
> > mutex_unlock(&ip_vs_sched_mutex);
> > return sched;
> > }
> > - if (sched->module)
> > - module_put(sched->module);
> > + module_put(sched->module);
> > }
> >
> > mutex_unlock(&ip_vs_sched_mutex);
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > index eadffb2..cafe28d 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -820,8 +820,7 @@ ip_vs_conn_fill_param_sync(struct net *net, int af, union ip_vs_sync_conn *sc,
> >
> > p->pe_data = kmemdup(pe_data, pe_data_len, GFP_ATOMIC);
> > if (!p->pe_data) {
> > - if (p->pe->module)
> > - module_put(p->pe->module);
> > + module_put(p->pe->module);
> > return -ENOMEM;
> > }
> > p->pe_data_len = pe_data_len;
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index deeb95f..b115f54 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -3472,8 +3472,7 @@ static int nf_tables_abort(struct sk_buff *skb)
> > break;
> > case NFT_MSG_NEWCHAIN:
> > if (nft_trans_chain_update(trans)) {
> > - if (nft_trans_chain_stats(trans))
> > - free_percpu(nft_trans_chain_stats(trans));
> > + free_percpu(nft_trans_chain_stats(trans));
> >
> > nft_trans_destroy(trans);
> > } else {
> > --
> > 2.1.3
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>
^ permalink raw reply
* Re: [patch net-next v4 1/9] openvswitch: actions: use skb_postpull_rcsum when possible
From: Simon Horman @ 2014-11-20 1:52 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, pshelar, therbert, edumazet, willemb,
dborkman, mst, fw, Paul.Durrant, tgraf, cwang
In-Reply-To: <1416402303-25341-2-git-send-email-jiri@resnulli.us>
On Wed, Nov 19, 2014 at 02:04:55PM +0100, Jiri Pirko wrote:
> Replace duplicated code by calling skb_postpull_rcsum
>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> Acked-by: Pravin B Shelar <pshelar@nicira.com>
I believe this may have originally been my handiwork:
Acked-by: Simon Horman <simon.horman@netronome.com>
> ---
> net/openvswitch/actions.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 394efa6..749a301 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -175,10 +175,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> if (unlikely(err))
> return err;
>
> - if (skb->ip_summed == CHECKSUM_COMPLETE)
> - skb->csum = csum_sub(skb->csum,
> - csum_partial(skb_mpls_header(skb),
> - MPLS_HLEN, 0));
> + skb_postpull_rcsum(skb, skb_mpls_header(skb), MPLS_HLEN);
>
> memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
> skb->mac_len);
> @@ -230,9 +227,7 @@ static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
> if (unlikely(err))
> return err;
>
> - if (skb->ip_summed == CHECKSUM_COMPLETE)
> - skb->csum = csum_sub(skb->csum, csum_partial(skb->data
> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>
> vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> *current_tci = vhdr->h_vlan_TCI;
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net-next] net: use skb_get_hash_raw instead of skb_get_hash in set_rps_cpu
From: David Miller @ 2014-11-20 1:57 UTC (permalink / raw)
To: therbert; +Cc: _govind, netdev
In-Reply-To: <CA+mtBx9K61KmguzQBWNbnmesCvWtzh_SjjbjedkgxN6p4Oay4Q@mail.gmail.com>
From: Tom Herbert <therbert@google.com>
Date: Wed, 19 Nov 2014 16:51:40 -0800
> On Wed, Nov 19, 2014 at 4:16 PM, David Miller <davem@davemloft.net> wrote:
>> From: Govindarajulu Varadarajan <_govind@gmx.com>
>> Date: Wed, 19 Nov 2014 15:30:44 +0530
>>
>>> In set_rps_cpu, we call skb_get_hash to get the hash of skb. The caller
>>> get_rps_cpu has already determined the hash of skb by calling skb_get_hash.
>>>
>>> Since get_rps_cpu is the only caller of set_rps_cpu, we can use skb_get_hash_raw
>>>
>>> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
>>
>> Tom, please review. And if you agree with this change, you'll need to
>> be spinning your flow_id change relative to this I guess since it overlaps.
>>
> We should probably pass hash as an argument in set_rps_cpu. It's only
> called from once place and the rflow is specifically chosen based on
> hash computed by caller-- don't want any ambiguity there.
Yep, that sounds a lot less error prone.
^ permalink raw reply
* [PATCH net-next v3 0/2] r8152: adjust rx functions
From: Hayes Wang @ 2014-11-20 2:29 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1394712342-15778-88-Taiwan-albertk@realtek.com>
v3:
For patch #1, remove unnecessary initialization for ret and
unnecessary blank line in r8152_submit_rx().
v2:
For patch #1, set actual_length to 0 before adding the rx to the
list, when a error occurs.
For patch #2, change the flow. Stop submitting the rx if a error
occurs, and add the remaining rx to the list for submitting later.
v1:
Adjust some flows and codes which are relative to r8152_submit_rx()
and rtl_start_rx().
Hayes Wang (2):
r8152: adjust r8152_submit_rx
r8152: adjust rtl_start_rx
drivers/net/usb/r8152.c | 60 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 40 insertions(+), 20 deletions(-)
--
1.9.3
^ permalink raw reply
* [PATCH net-next v3 1/2] r8152: adjust r8152_submit_rx
From: Hayes Wang @ 2014-11-20 2:29 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1394712342-15778-98-Taiwan-albertk@realtek.com>
The behavior of handling the returned status from r8152_submit_rx()
is almost same, so let r8152_submit_rx() deal with the error
directly. This could avoid the duplicate code.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0a30fd3..3b89229 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1032,7 +1032,6 @@ static void read_bulk_callback(struct urb *urb)
int status = urb->status;
struct rx_agg *agg;
struct r8152 *tp;
- int result;
agg = urb->context;
if (!agg)
@@ -1083,16 +1082,7 @@ static void read_bulk_callback(struct urb *urb)
break;
}
- result = r8152_submit_rx(tp, agg, GFP_ATOMIC);
- if (result == -ENODEV) {
- set_bit(RTL8152_UNPLUG, &tp->flags);
- netif_device_detach(tp->netdev);
- } else if (result) {
- spin_lock(&tp->rx_lock);
- list_add_tail(&agg->list, &tp->rx_done);
- spin_unlock(&tp->rx_lock);
- tasklet_schedule(&tp->tl);
- }
+ r8152_submit_rx(tp, agg, GFP_ATOMIC);
}
static void write_bulk_callback(struct urb *urb)
@@ -1680,7 +1670,6 @@ static void rx_bottom(struct r8152 *tp)
int len_used = 0;
struct urb *urb;
u8 *rx_data;
- int ret;
list_del_init(cursor);
@@ -1733,13 +1722,7 @@ find_next_rx:
}
submit:
- ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
- if (ret && ret != -ENODEV) {
- spin_lock_irqsave(&tp->rx_lock, flags);
- list_add_tail(&agg->list, &tp->rx_done);
- spin_unlock_irqrestore(&tp->rx_lock, flags);
- tasklet_schedule(&tp->tl);
- }
+ r8152_submit_rx(tp, agg, GFP_ATOMIC);
}
}
@@ -1806,11 +1789,28 @@ static void bottom_half(unsigned long data)
static
int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
{
+ int ret;
+
usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
agg->head, agg_buf_sz,
(usb_complete_t)read_bulk_callback, agg);
- return usb_submit_urb(agg->urb, mem_flags);
+ ret = usb_submit_urb(agg->urb, mem_flags);
+ if (ret == -ENODEV) {
+ set_bit(RTL8152_UNPLUG, &tp->flags);
+ netif_device_detach(tp->netdev);
+ } else if (ret) {
+ struct urb *urb = agg->urb;
+ unsigned long flags;
+
+ urb->actual_length = 0;
+ spin_lock_irqsave(&tp->rx_lock, flags);
+ list_add_tail(&agg->list, &tp->rx_done);
+ spin_unlock_irqrestore(&tp->rx_lock, flags);
+ tasklet_schedule(&tp->tl);
+ }
+
+ return ret;
}
static void rtl_drop_queued_tx(struct r8152 *tp)
--
1.9.3
^ permalink raw reply related
* [PATCH net-next v3 2/2] r8152: adjust rtl_start_rx
From: Hayes Wang @ 2014-11-20 2:29 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1394712342-15778-98-Taiwan-albertk@realtek.com>
If there is a error for r8152_submit_rx(), add the remaining rx
buffers to the list. Then the remaining rx buffers could be
submitted later.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3b89229..4a9ece0 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2001,6 +2001,25 @@ static int rtl_start_rx(struct r8152 *tp)
break;
}
+ if (ret && ++i < RTL8152_MAX_RX) {
+ struct list_head rx_queue;
+ unsigned long flags;
+
+ INIT_LIST_HEAD(&rx_queue);
+
+ do {
+ struct rx_agg *agg = &tp->rx_info[i++];
+ struct urb *urb = agg->urb;
+
+ urb->actual_length = 0;
+ list_add_tail(&agg->list, &rx_queue);
+ } while (i < RTL8152_MAX_RX);
+
+ spin_lock_irqsave(&tp->rx_lock, flags);
+ list_splice_tail(&rx_queue, &tp->rx_done);
+ spin_unlock_irqrestore(&tp->rx_lock, flags);
+ }
+
return ret;
}
--
1.9.3
^ permalink raw reply related
* ANNOUNCE: Netdev 0.1 conference
From: Jamal Hadi Salim @ 2014-11-20 2:47 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: info, David Miller, Stephen Jaworski, Richard Guy Briggs
Please forward to all places you think are relevant.
Netdev 0.1 (year 0, conference 1) is a community-driven conference
geared towards Linux netheads. Linux kernel networking and user
space utilization of the interfaces to the Linux kernel networking
subsystem are the focus.
If you are using Linux as a boot system for proprietary networking,
then this conference _may not be for you_.
The netdev conference this year is structured to be 50/50
by-invitation and talk submission. We are making sure that we
reach out to speakers who have interesting relevant topics because
we recognize most of these folks would typically not be submitting
papers to a conference. The invitation will be made by the technical
committee to the individual speakers both for paper and tutorial
sessions.
The call for papers is for the 50% submission portion of the
conference for both paper submission as well as tutorials.
We *highly discourage* submission of recycled talks.
Current topics include
- wireless
- performance analysis and improvement
- networking hardware and offload
- netfilter
- traffic control
- different networking layers (L2/3, etc)
- internet of things
- security
- additional topics can be suggested
We encourage submission of papers and tutorials. Unlike other
conferences, we are going to try and accommodate as many submissions
as possible - but please stay within the relevant topic focus and
tie to Linux networking to make it easier for the technical committee
to provide quick feedback. In order to give a talk you must be
registered. If your proposal is accepted you will not be charged
a conference fee or your conference fee will be refunded to you
when your talk gets accepted.
We will be posting more updates on how to submit in the near future.
We expect about 3-4 parallel tracks both during tutorials and main
talks. Tutorials will be on the first day and talks on subsequent days.
Why you should register
-----------------------
If you yearn for the old community tech driven conferences where
you mingle with fellow geeks (only these would be Linux networking
geeks) then this would be it. There will be no marketing flashy
openings. There will just be a pure feed of Linux networking.
netdev 0.1 will be held back to back with netconf 2015, the
by-invite Linux kernel networking workshop
(http://vger.kernel.org/netconf2015.html).
So gurus of all sorts will be there mingling and giving talks.
While there will be heavy Linux kernel influence we expect a lot
of user space presence as well.
Location:
---------
Downtown Ottawa, Canada. Exact location to be announced.
www.netdev01.org
Important Dates:
----------------
November 26, 2014 Call for Papers opens
December 10, 2014 Registration opens
January 10, 2015 Call for sessions deadline
January 20, 2015 Conference schedule announced
February 14-17, 2015 Conference days
Please register as soon as registration opens up on December 10.
Registering helps us plan properly for numbers of attendees,
ensuring venue sizes and supplies are appropriate without
wasting resources.
contact:
--------
info@netdev.info
^ permalink raw reply
* Resend - ANNOUNCE: Netdev 0.1 conference
From: Jamal Hadi Salim @ 2014-11-20 2:53 UTC (permalink / raw)
To: netdev; +Cc: info, davem, jaws, rgb
Please forward to all places you think are relevant.
[original email i sent had the wrong contact info at the
bottom should be: info@netdev01.info]
Netdev 0.1 (year 0, conference 1) is a community-driven conference
geared towards Linux netheads. Linux kernel networking and user
space utilization of the interfaces to the Linux kernel networking
subsystem are the focus.
If you are using Linux as a boot system for proprietary networking,
then this conference _may not be for you_.
The netdev conference this year is structured to be 50/50
by-invitation and talk submission. We are making sure that we
reach out to speakers who have interesting relevant topics because
we recognize most of these folks would typically not be submitting
papers to a conference. The invitation will be made by the technical
committee to the individual speakers both for paper and tutorial
sessions.
The call for papers is for the 50% submission portion of the
conference for both paper submission as well as tutorials.
We *highly discourage* submission of recycled talks.
Current topics include
- wireless
- performance analysis and improvement
- networking hardware and offload
- netfilter
- traffic control
- different networking layers (L2/3, etc)
- internet of things
- security
- additional topics can be suggested
We encourage submission of papers and tutorials. Unlike other
conferences, we are going to try and accommodate as many submissions
as possible - but please stay within the relevant topic focus and
tie to Linux networking to make it easier for the technical committee
to provide quick feedback. In order to give a talk you must be
registered. If your proposal is accepted you will not be charged
a conference fee or your conference fee will be refunded to you
when your talk gets accepted.
We will be posting more updates on how to submit in the near future.
We expect about 3-4 parallel tracks both during tutorials and main
talks. Tutorials will be on the first day and talks on subsequent days.
Why you should register
-----------------------
If you yearn for the old community tech driven conferences where
you mingle with fellow geeks (only these would be Linux networking
geeks) then this would be it. There will be no marketing flashy
openings. There will just be a pure feed of Linux networking.
netdev 0.1 will be held back to back with netconf 2015, the
by-invite Linux kernel networking workshop
(http://vger.kernel.org/netconf2015.html).
So gurus of all sorts will be there mingling and giving talks.
While there will be heavy Linux kernel influence we expect a lot
of user space presence as well.
Location:
---------
Downtown Ottawa, Canada. Exact location to be announced.
www.netdev01.org
Important Dates:
----------------
November 26, 2014 Call for Papers opens
December 10, 2014 Registration opens
January 10, 2015 Call for sessions deadline
January 20, 2015 Conference schedule announced
February 14-17, 2015 Conference days
Please register as soon as registration opens up on December 10.
Registering helps us plan properly for numbers of attendees,
ensuring venue sizes and supplies are appropriate without
wasting resources.
contact:
--------
info@netdev01.info
^ permalink raw reply
* Re: [PATCH net-next v2] net: sctp: keep owned chunk in destructor_arg instead of skb->cb
From: Vlad Yasevich @ 2014-11-20 3:02 UTC (permalink / raw)
To: Daniel Borkmann, davem; +Cc: linux-sctp, netdev
In-Reply-To: <1416444888-12778-1-git-send-email-dborkman@redhat.com>
On 11/19/2014 07:54 PM, Daniel Borkmann wrote:
> It's just silly to hold the skb destructor argument around inside
> skb->cb[] as we currently do in SCTP.
>
> Nowadays, we're sort of cheating on data accounting in the sense
> that due to commit 4c3a5bdae293 ("sctp: Don't charge for data in
> sndbuf again when transmitting packet"), we orphan the skb already
> in the SCTP output path, i.e. giving back charged data memory, and
> use a different destructor only to make sure the sk doesn't vanish
> on skb destruction time. Thus, cb[] is still valid here as we
> operate within the SCTP layer. (It's generally actually a big
> candidate for future rework, imho.)
>
> However, storing the destructor in the cb[] can easily cause issues
> should an non sctp_packet_set_owner_w()'ed skb ever escape the SCTP
> layer, since cb[] may get overwritten by lower layers and thus can
> corrupt the chunk pointer. There are no such issues at present,
> but lets keep the chunk in destructor_arg, as this is the actual
> purpose for it.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Good clean-up.
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
-vlad
> ---
> v1->v2:
> - Only reworded commit message to make it more clear
>
> net/sctp/socket.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 2120292..85e0b65 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -162,7 +162,7 @@ static inline void sctp_set_owner_w(struct sctp_chunk *chunk)
>
> chunk->skb->destructor = sctp_wfree;
> /* Save the chunk pointer in skb for sctp_wfree to use later. */
> - *((struct sctp_chunk **)(chunk->skb->cb)) = chunk;
> + skb_shinfo(chunk->skb)->destructor_arg = chunk;
>
> asoc->sndbuf_used += SCTP_DATA_SNDSIZE(chunk) +
> sizeof(struct sk_buff) +
> @@ -6870,14 +6870,10 @@ static void sctp_wake_up_waiters(struct sock *sk,
> */
> static void sctp_wfree(struct sk_buff *skb)
> {
> - struct sctp_association *asoc;
> - struct sctp_chunk *chunk;
> - struct sock *sk;
> + struct sctp_chunk *chunk = skb_shinfo(skb)->destructor_arg;
> + struct sctp_association *asoc = chunk->asoc;
> + struct sock *sk = asoc->base.sk;
>
> - /* Get the saved chunk pointer. */
> - chunk = *((struct sctp_chunk **)(skb->cb));
> - asoc = chunk->asoc;
> - sk = asoc->base.sk;
> asoc->sndbuf_used -= SCTP_DATA_SNDSIZE(chunk) +
> sizeof(struct sk_buff) +
> sizeof(struct sctp_chunk);
>
^ permalink raw reply
* Re: [PATCH net] net/mlx4_en: Add VXLAN ndo calls to the PF net device ops too
From: Or Gerlitz @ 2014-11-20 5:16 UTC (permalink / raw)
To: David Miller; +Cc: netdev, fw, amirv, saeedm
In-Reply-To: <20141119.151155.2119587385505880097.davem@davemloft.net>
On 11/19/2014 10:11 PM, David Miller wrote:
> Applied, -stable material? If so for what releases? In the future, if
> you add an appropriated Fixes: tag I can figure most of this out
> without asking you. Thanks.
Yes, please! let the below commits two swim for a while in 3.18-rc and
later push
them to -stable >= 3.15, got it re the fixes: tag, thanks for the heads-up.
9737c6a net/mlx4_en: Add VXLAN ndo calls to the PF net device ops too
f4a1edd net/mlx4_en: Advertize encapsulation offloads features only when
VXLAN tunnel is set
Or.
^ permalink raw reply
* Re: [PATCH net] virtio-net: validate features during probe
From: Jason Wang @ 2014-11-20 5:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20141119093901.GB26119@redhat.com>
On 11/19/2014 05:39 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 19, 2014 at 05:33:26PM +0800, Jason Wang wrote:
>> On 11/19/2014 04:59 PM, Michael S. Tsirkin wrote:
>>> On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
>>>> This patch validates feature dependencies during probe and fail the probing
>>>> if a dependency is missed. This fixes the issues of hitting BUG()
>>>> when qemu fails to advertise features correctly. One example is booting
>>>> guest with ctrl_vq=off through qemu.
>>>>
>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>> drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 93 insertions(+)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index ec2a8b4..4a0ad46 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>>>> };
>>>> #endif
>>>>
>>>> +static int virtnet_validate_features(struct virtio_device *dev,
>>>> + unsigned int *table,
>>>> + int table_size,
>>>> + unsigned int feature)
>>>> +{
>>>> + int i;
>>>> +
>>>> + if (!virtio_has_feature(dev, feature)) {
>>>> + for (i = 0; i < table_size; i++) {
>>>> + unsigned int f = table[i];
>>>> +
>>>> + if (virtio_has_feature(dev, f)) {
>>>> + dev_err(&dev->dev,
>>>> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
>>> This line's way too long.
>> Yes. (Anyway it pass checkpatch.pl since it forbids quoted string to be
>> split)
[...]
>>>> +
>>>> static int virtnet_probe(struct virtio_device *vdev)
>>>> {
>>>> int i, err;
>>>> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>> struct virtnet_info *vi;
>>>> u16 max_queue_pairs;
>>>>
>>>> + err = virtnet_check_features(vdev);
>>>> + if (err)
>>>> + return -EINVAL;
>>>> +
>>>> /* Find if host supports multiqueue virtio_net device */
>>>> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>>>> struct virtio_net_config,
>>> The API seems too complex, and you still had to open-code ECN logic.
>>> Just open-code most of it.
>> Yes, the ECN could be done through the same way as ctrl_vq did.
>>
>> How about move those checking into virtio core by just letting device
>> export its dependency table?
> So far we only have this for net, let's not add
> one-off APIs.
>
>>> You can use a helper macro to output a
>>> friendly message without code duplication.
>>> For example like the below (completely untested)?
>>>
>>>
>>> I would also like to split things: dependencies on
>>> VIRTIO_NET_F_CTRL_VQ might go into this kernel,
>>> since they help fix BUG.
>>>
>>> Others should wait since they don't fix any crashes, and there's a small
>>> chance of a regression for some hypervisor in the field.
>> Probably ok but not sure, since the rest features are all related to
>> csum and offloading, we are in fact depends on network core to fix them.
> Well it does fix them so ... there's no bug, is there?
>
No.
>>> -->
>>>
>>> virtio-net: friendlier handling of misconfigured hypervisors
>>>
>>> We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
>>> is not set but one of features depending on it is.
>>> That's not a friendly way to report errors to
>>> hypervisors.
>>> Let's check, and fail probe instead.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> ---
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 26e1330..7a7d1a3 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>>> };
>>> #endif
>>>
>>> +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
>>> + const char *fname)
>>> +{
>>> + if (!virtio_has_feature(vdev, fbit))
>>> + return false;
>>> +
>>> + dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
>>> + fbit, fname);
>>> +
>>> + return true;
>>> +}
>>> +
>>> +#define VIRTNET_FAIL_ON(vdev, fbit) \
>>> + __virtnet_fail_on_feature(vdev, fbit, #fbit)
>>> +
>>> static int virtnet_probe(struct virtio_device *vdev)
>>> {
>>> int i, err;
>>> @@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>>> struct virtnet_info *vi;
>>> u16 max_queue_pairs;
>>>
>>> + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
>>> + (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
>>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
>>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
>>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
>>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
>>> + return -EINVAL;
>>> +
>>> /* Find if host supports multiqueue virtio_net device */
>>> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>>> struct virtio_net_config,
>>>
>> Patch looks good, but consider we may check more dependencies in the
>> future, may need a helper instead. Or just use this and switch to
>> dependency table in 3.19.
> Pls note this is just pseudo-code - I didn't even compile it.
> If you want something like this merged, go ahead and
> post it, probably addressing Cornelia's request to
> print the dependency too. Maybe:
Ok, will post v3.
>
>>> (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX, "ctrl_vq") ||
>>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN, "ctrl_vq") ||
>>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE, "ctrl_vq") ||
>>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ, "ctrl_vq") ||
>>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR, "ctrl_vq")))
^ permalink raw reply
* [PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe
From: Zhu Yanjun @ 2014-11-20 6:04 UTC (permalink / raw)
To: netdev, w, zyjzyj2000, khandelwal.deepak.1987, vyasevich, tuexen,
dborkman
Cc: Zhu Yanjun, David S. Miller
2.6.x kernels require a similar logic change as commit 2c0d6ac894a
[sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe]
introduces for newer kernels.
Since the transport has always been in state SCTP_UNCONFIRMED, it
therefore wasn't active before and hasn't been used before, and it
always has been, so it is unnecessary to bug the user with a
notification.
Reported-by: Deepak Khandelwal <khandelwal.deepak.1987@gmail.com>
Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
Suggested-by: Michael Tuexen <tuexen@fh-muenster.de>
Suggested-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
Acked-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/sctp/associola.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 7eed77a..0f63396 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -824,6 +824,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
struct sctp_ulpevent *event;
struct sockaddr_storage addr;
int spc_state = 0;
+ bool ulp_notify = true;
/* Record the transition on the transport. */
switch (command) {
@@ -850,6 +851,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
else {
dst_release(transport->dst);
transport->dst = NULL;
+ ulp_notify = false;
}
spc_state = SCTP_ADDR_UNREACHABLE;
@@ -862,12 +864,14 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
/* Generate and send a SCTP_PEER_ADDR_CHANGE notification to the
* user.
*/
- memset(&addr, 0, sizeof(struct sockaddr_storage));
- memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
- event = sctp_ulpevent_make_peer_addr_change(asoc, &addr,
+ if (ulp_notify) {
+ memset(&addr, 0, sizeof(struct sockaddr_storage));
+ memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
+ event = sctp_ulpevent_make_peer_addr_change(asoc, &addr,
0, spc_state, error, GFP_ATOMIC);
- if (event)
- sctp_ulpq_tail_event(&asoc->ulpq, event);
+ if (event)
+ sctp_ulpq_tail_event(&asoc->ulpq, event);
+ }
/* Select new active and retran paths. */
--
1.9.1
^ permalink raw reply related
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