+ } else {
Are you sure we should either include one or the other? Should we not always
present userspace_actions if present? Can you explain why not?
The original intention is to avoid unnecessary memcpy. I think it is right to
always set it.
Ok, also because if we use this API for other stuff (like sent to userspace) we
might need it.
+ sample->userspace_actions = xmalloc(next_actions_len + NLA_HDRLEN);
I think doing the malloc, copy, and doing this again in the clone actions does
not make sense, especially as the data is not used up until the clone.
Maybe you can think about another solution without the additional malloc/copy,
and we can discuss it before you send the next revisions...
Actually we should not copy here. We only need to copy it in
offload_sample_clone(). How about change it like this:
This looks ugly, as now we use the offload_sample structure to pass parameters
around.
I have not given this much thought, but what about a flag to clone that steals
the user_space actions (needs fixing in parse_userspace_action() and
parse_sample_action() error handling)?
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 14e5b2b60..5fb770bf4 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -170,14 +170,19 @@ sample_find(uint32_t id)
static void
offload_sample_clone(struct offload_sample *new,
- const struct offload_sample *old)
+ const struct offload_sample *old,
+ bool steal_userspace_actions)
{
new->type = old->type;
new->action = xmemdup(old->action, old->action->nla_len);
- new->userspace_actions = old->userspace_actions
- ? xmemdup(old->userspace_actions,
- old->userspace_actions->nla_len)
- : NULL;
+ if (steal_userspace_actions) {
+ new->userspace_action = old->userspace_actions;
+ } else {
+ new->userspace_actions = old->userspace_actions
+ ? xmemdup(old->userspace_actions,
+ old->userspace_actions->nla_len)
+ : NULL;
+ }
new->userdata = xmemdup(old->userdata, old->userdata->nla_len);
new->tunnel = old->tunnel
? xmemdup(old->tunnel, sizeof *old->tunnel)
@@ -191,7 +196,7 @@ sgid_alloc(const struct offload_sample *sample)
{
struct sgid_node *node = xzalloc(sizeof *node);
- offload_sample_clone(&node->sample, sample);
+ offload_sample_clone(&node->sample, sample, true);
ovs_mutex_lock(&sgid_lock);
if (!id_pool_alloc_id(sample_group_ids, &node->id)) {
VLOG_ERR("Can't find a free sample group ID");
This is also not the nicest solution, maybe you should give this some more
thought. Maybe there is a nicer way of doing this.
But offload_sample_clone() is only called by sgid_alloc(), I'm a little
confused why we need a flag.
Actually my below change is straightforward. In netdev_tc_flow_put(), the
next_actions doesn't have
a nla header, we just need to add a header for it. So sflow can get output
tunnel info from it..
The main problem I have with your solution below is that you add the
next_actions field only to pass it on to the clone action, it is not really
used later on.
The solution I gave above is cleaner, and the only reason for
steal_userspace_actions flag is to be sure that the clone API is not really
doing a clone for this one parameter. The compiler will optimize all of this
out, but we have a clean API implementation.
My preference is to either use my approach or find a cleaner way without miss
using the offload_sample structure to pass a variable around which is not
really needed in the sample processing.
OK, I see. Will send v27 for review.
Thanks,
Chris
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index c3eb8fefc..e54a3de34 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -117,6 +117,9 @@ struct offload_sample {
struct nlattr *action; /* Sample action. Used in flow_get. */
struct nlattr *userdata; /* Struct user_action_cookie. */
struct nlattr *userspace_actions; /* All actions to get output tunnel.
*/
+ const struct nlattr *next_actions; /* All actions pointer without nla
header. */
+ size_t next_actions_len /* All actions pointer without nla
header Length. */
struct flow_tnl *tunnel; /* Input tunnel. */
};
@@ -182,10 +185,14 @@ offload_sample_clone(struct offload_sample *dst,
dst->type = src->type;
dst->action = src->action ? xmemdup(src->action, src->action->nla_len)
: NULL;
- dst->userspace_actions = src->userspace_actions
- ? xmemdup(src->userspace_actions,
- src->userspace_actions->nla_len)
- : NULL;
+ if (src->next_actions) {
+ uint16_t len = src->next_actions_len;
+
+ dst->userspace_actions = xmalloc(len + NLA_HDRLEN);
+ nullable_memcpy((char *) dst->userspace_actions + NLA_HDRLEN,
+ src->next_actions, len);
+ dst->userspace_actions->nla_len = len + NLA_HDRLEN;
+ }
dst->userdata = src->userdata
? xmemdup(src->userdata, src->userdata->nla_len)
: NULL;
@@ -2096,12 +2103,9 @@ offload_sample_init(struct offload_sample *sample,
memset(sample, 0, sizeof *sample);
if (tunnel) {
sample->tunnel = CONST_CAST(struct flow_tnl *, tnl);
- } else {
- sample->userspace_actions = xmalloc(next_actions_len + NLA_HDRLEN);
- nullable_memcpy((char *) sample->userspace_actions + NLA_HDRLEN,
- next_actions, next_actions_len);
- sample->userspace_actions->nla_len = next_actions_len + NLA_HDRLEN;
}
+ sample->next_actions = next_actions;
+ sample->next_actions_len = next_actions_len;
return 0;
}
+ nullable_memcpy((char *) sample->userspace_actions + NLA_HDRLEN,
+ next_actions, next_actions_len);
+ sample->userspace_actions->nla_len = next_actions_len + NLA_HDRLEN;
+ }
+
+ return 0;
+}
<SNIP>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
