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.
>>> + } 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.
>
> 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
