On 26 May 2023, at 10:19, Chris Mi wrote:
> 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.
Thanks
>>>>> + } 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.
>>> 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
