On 2/23/2023 9:04 PM, Ilya Maximets wrote:
On 2/23/23 12:24, Chris Mi wrote:
Hi Ilya,
Sorry for late response. Please see my comments in-line.
On 1/17/2023 11:00 PM, Ilya Maximets wrote:
On 3/17/22 02:12, Chris Mi wrote:
This patch set adds offload support for sFlow.
Psample is a genetlink channel for packet sampling. TC action act_sample
uses psample to send sampled packets to userspace.
When offloading sample action to TC, userspace creates a unique ID to
map sFlow action and tunnel info and passes this ID to kernel instead
of the sFlow info. psample will send this ID and sampled packet to
userspace. Using the ID, userspace can recover the sFlow info and send
sampled packet to the right sFlow monitoring host.
v2-v1:
- Fix robot errors.
v3-v2:
- Remove Gerrit Change-Id.
- Add patch #9 to fix older kernels build issue.
- Add travis test result.
v4-v3:
- Fix offload issue when sampling rate is 1.
v5-v4:
- Move polling thread from ofproto to netdev-offload-tc.
v6-v5:
- Rebase.
- Add GitHub Actions test result.
v7-v6:
- Remove Gerrit Change-Id.
- Fix "ERROR: Inappropriate spacing around cast"
v8-v7
- Address Eelco Chaudron's comment for patch #11.
v9-v8
- Remove sflow_len from struct dpif_sflow_attr.
- Log a debug message for other userspace actions.
v10-v9
- Address Eelco Chaudron's comments on v9.
v11-v10
- Fix a bracing error.
v12-v11
- Add duplicate sample group id check.
v13-v12
- Remove the psample poll thread from netdev-offload-tc and reuse
ofproto handler thread according to Ilya's new desgin.
- Add dpif-offload-provider layer according to Eli's suggestion.
v14-v13
- Fix a robot error.
v15-v14
- Address Eelco Chaudron's comments on v14.
v16-v15
- Address Eelco Chaudron's comments on v15.
- Add two test cases.
v17-v16
- Address Eelco Chaudron's comments on v16.
- Move struct dpif_offload_api from struct dpif_class to struct dpif.
v18-v17
- Rename dpif_offload_api to dpif_offload_class.
- Add init and destroy callbacks in dpif_offload_class. They are called
when registering dpif_offload_class.
v19-18
- Fix a bug that psample_sock is destroyed when last bridge is deleted.
v20-19
- Move buf_stub to struct dpif_offload_sflow avoid garbage values when
ofproto proceses the sampled packet.
v21-20
- Remove netdev dummy for dpif-offload according to Eelco's comment.
Chris Mi (8):
compat: Add psample and tc sample action defines for older kernels
ovs-kmod-ctl: Load kernel module psample
dpif-offload-provider: Introduce dpif-offload-provider layer
netdev-offload-tc: Introduce group ID management API
dpif-offload-netlink: Implement dpif-offload-provider API
ofproto: Introduce API to process sFlow offload packet
netdev-offload-tc: Add offload support for sFlow
system-offloads-traffic.at: Add sFlow offload test cases
NEWS | 1 +
include/linux/automake.mk | 4 +-
include/linux/psample.h | 62 ++++
include/linux/tc_act/tc_sample.h | 25 ++
lib/automake.mk | 3 +
lib/dpif-netdev.c | 3 +-
lib/dpif-netlink.c | 20 +-
lib/dpif-offload-netlink.c | 227 +++++++++++++++
lib/dpif-offload-provider.h | 94 +++++++
lib/dpif-offload.c | 147 ++++++++++
lib/dpif-provider.h | 10 +-
lib/dpif.c | 3 +-
lib/netdev-offload-tc.c | 466 +++++++++++++++++++++++++++++--
lib/netdev-offload.h | 2 +
lib/tc.c | 61 +++-
lib/tc.h | 16 +-
ofproto/ofproto-dpif-upcall.c | 73 +++++
tests/system-offloads-traffic.at | 101 +++++++
utilities/ovs-kmod-ctl.in | 14 +
19 files changed, 1299 insertions(+), 33 deletions(-)
create mode 100644 include/linux/psample.h
create mode 100644 include/linux/tc_act/tc_sample.h
create mode 100644 lib/dpif-offload-netlink.c
create mode 100644 lib/dpif-offload-provider.h
create mode 100644 lib/dpif-offload.c
Hi, Chris, others.
Following up with a summary of what we talked about with Majd during
the conference with some additional technical details.
The main issue with this patch set right now is an dpif-offload-provider
concept. It might make sense on it's own, but as I said before, we
need to take 'all or nothing' approach with it, otherwise we'll stuck
with a half-baked solution scattered among different parts of OVS and
that will be not beneficial from the architecture perspective further
complicating already fairly complex solution.
The full implementation of the dpif-offload-provider concept is clearly
out of scope for the sFlow offload feature, so if we need it, it should
be done as a separate architectural change re-working all offload
implementations including both tc and rte_flow.
What I'm suggesting for the current patch set is to drop the
dpif-offload-provider parts and go back to the simple schema with
receiving upcalls from the regular dpif-provider. This should make the
code simpler and better contained in modules it belongs to.
Implementation suggestions:
1. OVS handler thread:
recv_upcalls()
--> dpif_recv()
--> dpif_netlink_recv()
if (dpif_netlink_recv_cpu_dispatch() == EAGAIN):
--> * netdev_offload_recv()
for each registered offload API:
--> * flow_api->recv()
--> * netdev_offload_tc_recv()
if handler.id == 0:
Receive psample message,
fill the struct dpif_upcall, etc.
--> break on success (one of the APIs
can starve, but we only have one
implementation, so it's not a problem
for today.)
--> upcall_receive()
--> process_upcall()
* new functions to implement.
Similar call chain for the recv_wait() callback.
Done.
2. process_offload_sflow() seems to duplicate existing upcall_receive(),
I'm not sure why we need that.
Because I thought it is not good to touch this code to deal with offloads.
The point was that you should not need to touch this code.
The underlying dpif/netdev-offload should construct correct upcall structures,
so it will be transparent for the generic upcall_receive().
OK, I see your point. But we still changed a little. I added a comment
in patch 4 and 6.
Because netdev offload upcalls can't provide the key and key_len to
construct the flow.
We can provide something else to construct partial flow. And it is
enough to process
sFlow.
OK, process_offload_sflow() was removed and upcall_receive() is used.
3. lib/id-pool.h or lib/id-fpool.h should probably be used to allocate
sample group IDs instead of looping over 'next_sample_group_id'.
Done.
Starving of netdev_offload_recv() can be solved by storing a flag in
the dpif_handler structure and alternating the order of receiving
normal upcalls and offload upcalls based on that flag, switching it
on each call, e.g.:
handler = &dpif->handlers[handler_id];
if (handler->recv_offload_first) {
error = netdev_offload_recv(handler_id, upcall, buf);
if (error == EAGAIN) {
error = dpif_netlink_recv__(dpif, handler_id, upcall, buf);
}
} else {
error = dpif_netlink_recv__(dpif, handler_id, upcall, buf);
if (error == EAGAIN) {
error = netdev_offload_recv(handler_id, upcall, buf);
}
}
handler->recv_offload_first = !handler->recv_offload_first;
Done. Thanks for the sample code.
Thanks,
Chris
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
