On 26 May 2023, at 10:18, Chris Mi wrote:
> On 5/10/2023 7:42 PM, Eelco Chaudron wrote:
>> On 26 Apr 2023, at 4:42, Chris Mi wrote:
>>
>> <SNIP>
>>>>> /* DPIF_UC_ACTION only. */
>>>>> struct nlattr *userdata; /* Argument to
>>>>> OVS_ACTION_ATTR_USERSPACE. */
>>>>> - struct nlattr *out_tun_key; /* Output tunnel key. */
>>>>> - struct nlattr *actions; /* Argument to OVS_ACTION_ATTR_USERSPACE.
>>>>> */
>>>>> + struct nlattr *out_tun_key; /* Output tunnel key. */
>>>>> + struct nlattr *actions; /* Argument to
>>>>> OVS_ACTION_ATTR_USERSPACE. */
>>>>> + struct flow flow; /* Caller provide 'flow' if 'key' is not
>>>>> + available. */
>>>> "Caller provided 'flow' if the 'key' is not available."
>> Did you fix this one?
> Done.
>>>> Also, multiline comments should start with a *.
>>> But it is not true for most of data struct member comments. For example,
>>>
>>> struct dpif_dp_stats {
>>> uint64_t n_hit; /* Number of flow table matches. */
>>> uint64_t n_missed; /* Number of flow table misses. */
>>> uint64_t n_lost; /* Number of misses not sent to userspace.
>>> */
>>> uint64_t n_flows; /* Number of flows present. */
>>> uint64_t n_cache_hit; /* Number of mega flow mask cache hits for
>>> flow table matches. */
>>> uint64_t n_mask_hit; /* Number of mega flow masks visited for
>>> flow table matches. */
>>> uint32_t n_masks; /* Number of mega flow masks. */
>>> };
>>>
>>> Could you please confirm with it? Thanks.
>> Looks like this file has both, and more without the starting *, so I assume
>> you can keep it as is.
> OK, thanks.
>> <SNIP>
>>
>>>>> +nl_parse_psample(struct offload_psample *psample, struct ofpbuf *buf)
>>>>> +{
>>>>> + static const struct nl_policy ovs_psample_policy[] = {
>>>>> + [PSAMPLE_ATTR_IIFINDEX] = { .type = NL_A_U16, .optional = true,
>>>>> },
>>>>> + [PSAMPLE_ATTR_SAMPLE_GROUP] = { .type = NL_A_U32 },
>>>>> + [PSAMPLE_ATTR_GROUP_SEQ] = { .type = NL_A_U32 },
>>>>> + [PSAMPLE_ATTR_DATA] = { .type = NL_A_UNSPEC },
>>>>> + };
>>>>> + struct nlattr *a[ARRAY_SIZE(ovs_psample_policy)];
>>>>> + struct genlmsghdr *genl;
>>>>> + struct nlmsghdr *nlmsg;
>>>>> + struct ofpbuf b;
>>>>> +
>>>>> + b = ofpbuf_const_initializer(buf->data, buf->size);
>>>>> + nlmsg = ofpbuf_try_pull(&b, sizeof *nlmsg);
>>>>> + genl = ofpbuf_try_pull(&b, sizeof *genl);
>>>>> + if (!nlmsg || !genl || nlmsg->nlmsg_type != psample_family
>>>>> + || !nl_policy_parse(&b, 0, ovs_psample_policy, a,
>>>>> + ARRAY_SIZE(ovs_psample_policy))) {
>>>>> + return EINVAL;
>>>>> + }
>>>>> +
>>>>> + if (a[PSAMPLE_ATTR_IIFINDEX]) {
>>>>> + psample->iifindex = nl_attr_get_u16(a[PSAMPLE_ATTR_IIFINDEX]);
>>>>> + }
>>>> Do } else { psample->iffindex = 0 }, to safe on the memset later.
>>> I removed psample->ifindex. New code will get input ifindex also via
>>> group_id mapping
>>> instead of from TC. TC only provides group id and sampled packets. We will
>>> get other info
>>> all from the group_id mapping, eg. output tunnel, input ifindex.
>> Can you elaborate a bit more on why you made this change? Is it because the
>> value is not returned by the kernel?
>>
>> <SNIP>
> I explained in patch #8. There is a limitation that ovs/tc can't get
> in_ifindex from namespace.
> And I think this change is a good design. It is extensible.
>>>>> + upcall->userdata = sample->userdata;
>>>>> + if (sample->tunnel) {
>>>>> + memcpy(&flow->tunnel, sample->tunnel, sizeof flow->tunnel);
>>>> Are we ok memcpy will work here? We might need to initialize the
>>>> sample->tunnel info correctly in the upcomming patches, see
>>>> flow_tnl_copy__().
>>> From the testing result, it seems it is ok. Maybe flow_tnl_copy__() is
>>> more accurate.
>>> But we need to change a lot of functions non-static. Not sure if we should
>>> do that.
>>> What do you think?
>> It’s already a static inline function in an included file, so maybe just
>> removing the __ is all that needs to be done.
>> This way we are sure we copy only the needed data (after memsetting the
>> destination), and are not introducing any hidden problem that does not show
>> up with the current tests.
> Yes, indeed. Done.
>>>>> + }
>>>>> + if (sample->userspace_actions) {
>>>>> + upcall->actions = sample->userspace_actions;
>>>>> + }
>>>>> + if (psample->iifindex) {
>>>>> + flow->in_port.odp_port =
>>>>> netdev_ifindex_to_odp_port(psample->iifindex);
>>>>> + }
>>>> If not provided I think the port should be set to ODPP_NONE (the default
>>>> value), please confirm.
>>>> If this is not true the above netdev_ifindex_to_odp_port() should cause
>>>> this function to fail if ODPP_NONE is returned.
>>> Now ifindex is got from netdev_get_ifindex(netdev) in netdev_tc_flow_put().
>>> It should be provided always.
>> ACK
>>
>> <SNIP>
>>
Thanks for fixing all.
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
