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>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
