Hi Chris,
Noticed your v8, will try to fully review it before the year is over ;)
//Eelco
On 14 Dec 2020, at 10:33, Chris Mi wrote:
<SNIP>
diff --git a/lib/dpif.h b/lib/dpif.h
index ed4257210..150162034 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -878,6 +878,7 @@ void dpif_register_upcall_cb(struct dpif *,
upcall_callback *, void *aux);
struct dpif_sflow_attr {
const struct nlattr *sflow; /* sFlow action */
size_t sflow_len; /* Length of 'sflow'
in bytes. */
+ // Why do we need to store sflow_len, it's part of the nlattr
data?
We need to copy and compare the dpif_sflow_attr. So sflow_len is used
for that.
But you could just use sflow->nla_len?
void *userdata; /* struct
user_action_cookie */
size_t userdata_len; /* struct
user_action_cookie length */
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 75744aa3c..84991ff6a 100644
static int
-parse_action_userspace(const struct nlattr *actions,
- struct dpif_sflow_attr
*sflow_attr)
+parse_sample_actions(const struct nlattr *actions,
+ struct dpif_sflow_attr
*sflow_attr)
{
const struct nlattr *nla;
unsigned int left;
+ int err = EINVAL;
NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE)
{
- return parse_userspace_userdata(nla,
sflow_attr);
+ err = parse_userspace_userdata(nla,
sflow_attr);
+ } else {
+ /* We can't offload other nested actions */
+ VLOG_DBG_RL(&error_rl,
+ "%s: other than
OVS_ACTION_ATTR_USERSPACE attribute",
+ __func__);
+ return EINVAL;
}
}
I think we should not log in this function. We may be overwhelmed for
various usespace actions.
It’s a debug message, and it might be useful to figure out why offload
is failing, especially as this is a corner case.
- VLOG_ERR_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE
attribute",
- __func__);
- return EINVAL;
+ if (err) {
+ //THIS IS NOT AN ERROR CASE, FROM A DATAPATH IT'S
VALID TO DO ANY ACTION
But sample action expects a userspace attribute.
This is the typical use case, but I think you can add one without it and
just redirect the packet to a different port. I don’t have a setup
right now, but I will try some examples when I’ll do the v8 review.
+ VLOG_DBG_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE
attribute",
+ __func__);
+ }
+ return err;
}
<SNIP>
One other general command, how would we guarantee that the group ID
value used by tc-sample is unique and not being used by any other
application than OVS?
Please see the comment:
239 /* Allocate a unique group id for the given set of flow
metadata.
240 * The ID space is 2^^32, so there should never be a
situation in which all
241 * the IDs are used up. We loop until we find a free one.
*/
242 static struct gid_node *
243 gid_alloc__(const struct dpif_sflow_attr *sflow, uint32_t
hash)
This id is similar to recirc id.
Do we assume tc-sample is elusively assigned to OVS?
Yes, I think so.
This might not be the case in customer environments, so we might
need to be able to disable offloading this action.
If we can offload, why not disable it. If we can't offload, tc will
return EOPNOTSUPP and fail back to ovs datapath.
So I don't think we need to be able to disable it.
Ack all three above, but I was referring to someone that uses sflow
kmod on his management interface already, and now all of a sudden OVS
will use it too. Which could cause a problem as the u32 space is
started between all user space application. So I do think we need an
option to disable it in OVS.
Sorry, what do you mean by sflow kmod?
I mean the psample kmod, tc-sample feature.
One final question, do you know if this will be supported in your
NICs as hardware offload?
Sure. We submitted the first version of the OVS patch set in
September. We support offload before that.
I believe the driver change will be in linux net-next branch soon.
Cool, looking forward to seeing the change…
If you do send out a new revision of this patchset, I’ll try to
make some time and do a full review and some testing.
V8 will be sent for the full review.
Thanks,
Chris
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev