On 3/22/2023 6:11 PM, Eelco Chaudron wrote:
On 22 Mar 2023, at 7:31, Chris Mi wrote:
On 3/16/2023 5:23 PM, Eelco Chaudron wrote:
On 1 Mar 2023, at 8:22, Chris Mi wrote:
Create a unique group ID to map the sFlow info when offloading sample
action to TC. When showing the offloaded datapath flows, translate the
group ID from TC sample action to sFlow info using the mapping.
See some comments inline below.
//Eelco
<SNIP>
+static int
+parse_userspace_attributes(const struct nlattr *actions,
+ struct offload_sflow *sflow)
+{
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+ const struct nlattr *nla;
+ unsigned int left;
+
+ NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
+ if (nl_attr_type(nla) == OVS_USERSPACE_ATTR_USERDATA) {
+ struct user_action_cookie *cookie;
+
+ cookie = (struct user_action_cookie *) nl_attr_get(nla);
See Ilya's comment on misaligned data.
Ilya replied before. He said there is no problem for this case.
How about this diff to check the misaligned data:
Looks like it does not require any handling from Ilya’s analysis. So you could
just leave the code as is, or if you want to avoid any problems in the future
copy the data out, as done in other places.
For example see the following commit: git show e5162ac42
Done.
@@ -2024,8 +2024,12 @@ parse_userspace_attributes(const struct nlattr *actions,
NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
if (nl_attr_type(nla) == OVS_USERSPACE_ATTR_USERDATA) {
+ size_t userdata_len = nl_attr_get_size(nla);
struct user_action_cookie *cookie;
+ if (userdata_len != sizeof *cookie) {
+ continue;
+ }
cookie = (struct user_action_cookie *) nl_attr_get(nla);
if (cookie->type == USER_ACTION_COOKIE_SFLOW) {
sample->type = USER_ACTION_COOKIE_SFLOW;
Do we really need to allocate this with the additional NLA_DHRLEN? Anyway,
these are not the right set of actions.
These are the actions following the sflow specific attr, and are not of
interest to sflow offload they should/will be executed by tc in the datapath.
It is used to show output tunnel info.
Let’s discuss more as part of patch 3.
+ nullable_memcpy((char *) sflow->actions + NLA_HDRLEN, actions,
+ actions_len);
+ sflow->actions->nla_len = actions_len + NLA_HDRLEN;
+ }
+
+ return 0;
+}
+
+static int
+parse_sample_action(struct tc_flower *flower, struct tc_action *tc_action,
+ const struct nlattr *actions, size_t actions_len,
+ const struct nlattr *sample_action,
+ struct sample_param *param)
The actions and actions_len are the remaining actions to be processed, they are
not part of the sample action, and I do not understand why you pass them on.
All you need is the sample_action, which is what you were using in v18. You
execute those actions again in userspace, which is not what sflow should do.
This could lead to duplicate packets.
It is used to show output tunnel info.
Let’s discuss more as part of patch 3.
+{
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+ struct offload_sflow sflow;
+ const struct nlattr *nla;
+ unsigned int left;
+ int err;
+
+ err = offload_sflow_set(&sflow, actions, actions_len, flower->tunnel,
+ param);
+ if (err) {
+ return err;
+ }
+ sflow.action = (struct nlattr *) sample_action;
+
Here the err should be set to EINVAL, because if OVS_SAMPLE_ATTR_ACTIONS is
not found we should return an error, see v18 for correct behavior.
Do you mean offload_sflow_set() return value or something else?
The parse_sample_action() should return EINVAL if OVS_SAMPLE_ATTR_ACTIONS is
not found. So before calling NL_NESTED_FOR_EACH_UNSAFE, err should be set to
EINVAL.
+ err = EINVAL;
Done.
+ NL_NESTED_FOR_EACH_UNSAFE (nla, left, sample_action) {
+ if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_ACTIONS) {
+ err = parse_sample_actions_attribute(nla, &sflow);
+ } else if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_PROBABILITY) {
+ tc_action->type = TC_ACT_SAMPLE;
+ traction->sample.rate = UINT32_MAX / nl_attr_get_u32(nla);
+ } else {
+ err = EINVAL;
+ goto out;
+ }
+ }
<SNIP>
+static int
+parse_userspace_action(struct tc_flower *flower, struct tc_action *tc_action,
+ const struct nlattr *actions, size_t actions_len,
+ const struct nlattr *userspace_action,
+ struct sample_param *param)
+{
Same as above, the actions and actions_len are the remaining action to be
processed, they are not part of the sample action, and I do not understand why
you pass them on.
All you need is the userspace_action, which is what you were using in v18.
It is used to show output tunnel info.
Let’s discuss more as part of patch 3.
<SNIP>
static int
parse_check_pkt_len_action(struct netdev *netdev, struct tc_flower *flower,
@@ -2154,7 +2351,7 @@ parse_check_pkt_len_action(struct netdev *netdev, struct
tc_flower *flower,
nl_attr_get(nl_actions),
nl_attr_get_size(nl_actions),
recirc_act, !last_action,
- &ge_jump_update);
+ &ge_jump_update, NULL);
Would this be a problem if an sflow action is embedded in check_pkt_len?
Maybe will crash. sample_param should pass as argument.
Yes, see the suggested patch I added originally in my reply.
Done.
<SNIP>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
