On 5/10/2023 8:20 PM, Eelco Chaudron wrote:
On 26 Apr 2023, at 4:44, Chris Mi wrote:
<SNIP>
+
+static int
+offload_sample_init(struct offload_sample *sample,
+ const struct nlattr *next_actions,
+ size_t next_actions_len, bool tunnel,
+ const struct flow_tnl *tnl)
+{
+ memset(sample, 0, sizeof *sample);
+ if (tunnel) {
+ sample->tunnel = CONST_CAST(struct flow_tnl *, tnl);
See previous comment on flow_tnl_copy__(), i.e. is the structure properly
initialized?
So shall we make flow_tnl_copy__() and related functions public?
I think so, as it’s in an include already, we just need to remove the trailing
__ and use it.
Done.
+ } 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..
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
