On 17/02/2017 23:15, Marcelo Ricardo Leitner wrote:
On Wed, Feb 08, 2017 at 05:29:26PM +0200, Roi Dayan wrote:
From: Paul Blakey <pa...@mellanox.com>
Flower classifer requires a different priority per mask,
so we hash the mask and generate a new priority for
each new mask used.
Signed-off-by: Paul Blakey <pa...@mellanox.com>
Reviewed-by: Roi Dayan <r...@mellanox.com>
---
lib/netdev-tc-offloads.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index f88e7ce..48e452a 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -176,6 +176,44 @@ add_ufid_tc_mapping(const ovs_u128 *ufid, int prio, int
handle,
return replace;
}
+struct prio_map_data {
+ struct hmap_node node;
+ struct tc_flower_key mask;
+ ovs_be16 protocol;
+ uint16_t prio;
^^^^^^^^
+};
+
+static uint16_t
^^^^^^^^
+get_prio_for_tc_flower(struct tc_flower *flower)
+{
+ static struct hmap prios = HMAP_INITIALIZER(&prios);
+ static struct ovs_mutex prios_lock = OVS_MUTEX_INITIALIZER;
+ static int last_prio = 0;
^^^
+ size_t key_len = sizeof(struct tc_flower_key);
+ size_t hash = hash_bytes(&flower->mask, key_len,
+ (OVS_FORCE uint32_t) flower->key.eth_type);
+ struct prio_map_data *data;
+ struct prio_map_data *new_data;
+
+ ovs_mutex_lock(&prios_lock);
+ HMAP_FOR_EACH_WITH_HASH(data, node, hash, &prios) {
+ if (!memcmp(&flower->mask, &data->mask, key_len)
+ && data->protocol == flower->key.eth_type) {
+ ovs_mutex_unlock(&prios_lock);
+ return data->prio;
+ }
+ }
+
+ new_data = xzalloc(sizeof *new_data);
+ memcpy(&new_data->mask, &flower->mask, key_len);
+ new_data->prio = ++last_prio;
^^^^^^^^^^^^^^^^^^
I don't think this type difference is an issue, but it would be nice if
it's avoided.
More importantly, last_prio will overflow sooner or later here, and I
don't see any check on whether the new value is actually free to use. As
you explained on the other email, if there is a mask with another
protocol already using this priority, flower will reject it.
Considering the useful range is only 16 bit wide, this may happen quite
often I'm afraid.
Marcelo
There isn't a check because a priority is generated if we see a new
masks/eth_type combination and this mapping is never removed and is
reused every time we see this combination again.
What we end up with is a map like this:
eth/mask combination 1 -> 1
eth/mask combination 2 -> 2
eth/mask combination 3 -> 3
eth/mask combination 4 -> 4
....
and so it can't already be used if OVS is the only controller of TC
for OVS added devices.
And to overflow it, you'll need to come up with many different kinds of
flow, which with megaflows I don't see this happening organically.
But sure we can add a check.
Thanks.
+ new_data->protocol = flower->key.eth_type;
+ hmap_insert(&prios, &new_data->node, hash);
+ ovs_mutex_unlock(&prios_lock);
+
+ return new_data->prio;
+}
+
int
netdev_tc_flow_flush(struct netdev *netdev)
{
--
2.7.4
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
