On 22 Jan 2021, at 4:27, Chris Mi wrote:
On 1/13/2021 3:49 AM, Eelco Chaudron wrote:
On 15 Dec 2020, at 4:38, Chris Mi wrote:
<SNIP>
+
+static void
+gid_cleanup(void)
+{
+ long long int now = time_msec();
+ struct gid_node *node;
+
+ /* Do maintenance at most 4 times / sec. */
+ ovs_rwlock_rdlock(&gid_rwlock);
+ if (now - gid_last_run < GID_RUN_INTERVAL) {
+ ovs_rwlock_unlock(&gid_rwlock);
+ return;
+ }
+ ovs_rwlock_unlock(&gid_rwlock);
+
+ ovs_rwlock_wrlock(&gid_rwlock);
+ gid_last_run = now;
+
+ LIST_FOR_EACH_POP (node, exp_node, &gid_expired) {
+ cmap_remove(&gid_map, &node->id_node, node->id);
+ ovsrcu_postpone(gid_node_free, node);
+ }
+
+ if (!ovs_list_is_empty(&gid_expiring)) {
+ /* 'gid_expired' is now empty, move nodes in
+ * 'gid_expiring' to it. */
+ ovs_list_splice(&gid_expired,
+
ovs_list_front(&gid_expiring),
+ &gid_expiring);
+ }
Trying to understand why we have two list? Why can't we directly
cleanup from expiring list?
The code takes the recirc id management for example
(ofproto-dpif-rid.c).
I'm not 100% sure the two lists are really needed here. But the recirc
id code exists for a long time.
I think it is mature. And I don't want to re-invent something.
And when psample thread receives the sampled packets, it needs to
lookup the sgid_node
based on the group id. But the sgid_node could be deleted anytime by
the revalidator.
So I think two list can resolve this issue. We have 250 ms interval to
wait for the sampled packets
on the fly to be processed.
Just adding code because it exists somewhere else does not make sense to
me. If you do not have a valid reason, why add all this complexity?
I would like some other reviewer's comments on this, maybe I missed
something, Ilya?
+ ovs_rwlock_unlock(&gid_rwlock);
+}
+
<SNIP>
+
+static void
+gid_node_unref(const struct gid_node *node_)
+ OVS_EXCLUDED(gid_rwlock)
+{
+ struct gid_node *node = CONST_CAST(struct gid_node *,
node_);
+
+ ovs_rwlock_wrlock(&gid_rwlock);
+ if (node && ovs_refcount_unref(&node->refcount) == 1) {
+ /* Prevent re-use of this node by removing the node
from
+ * gid_metadata_map' */
+ cmap_remove(&gid_metadata_map, &node->metadata_node,
node->hash);
+ /* We keep the node in the 'gid_map' so that it can
be found as
+ * long as it lingers, and add it to the
'gid_expiring' list. */
+ ovs_list_insert(&gid_expiring, &node->exp_node);
Why do we do a delayed remove here? Any sflow packets in the pipeline
can be silently deleted, based on a failing sgid_find().
Please see my above comment for the two list issue.
The only thing concern might be ID reuse. But I think with 32^2 IDs,
and the current allocator schema, this is unlikely to happen.
It would make the code cleaner, and the rwlock can be a normal lock.
+ }
+ ovs_rwlock_unlock(&gid_rwlock);
+}
+
+static void
+gid_free(uint32_t id)
+{
+ const struct gid_node *node;
<SNIP>
@@ -398,6 +651,8 @@ netdev_tc_flow_dump_create(struct netdev
*netdev,
*dump_out = dump;
+ gid_cleanup();
+
I guess this can be removed if we can delete the pigs directly, if
not, we might want to put a comment here explaining why this is
called from this function.
OK, I'll put a comment here.
return 0;
}
@@ -1797,6 +2052,11 @@ netdev_tc_flow_put(struct netdev *netdev,
struct match *match,
action->type = TC_ACT_GOTO;
action->chain = 0; /* 0 is reserved and
not used by recirc. */
flower.action_count++;
+ } else if (nl_attr_type(nla) ==
OVS_ACTION_ATTR_SAMPLE) {
+ struct dpif_sflow_attr sflow_attr;
+
+ memset(&sflow_attr, 0, sizeof sflow_attr);
+ gid_alloc_ctx(&sflow_attr);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
