On 15 Dec 2023, at 19:22, Mike Pattrick wrote:
> On Fri, Dec 15, 2023 at 8:01 AM Eelco Chaudron <[email protected]> wrote: >> >> >> >> On 11 Dec 2023, at 16:39, Mike Pattrick wrote: >> >>> Currently dp_netdev_upcall() resolves checksums on all packets, but this >>> isn't strictly needed. The checksums will be resolved before >>> transmission. However, we do have to resolve the checksums before >>> sending a packet to the controller as offload flags aren't retained. >>> >>> Signed-off-by: Mike Pattrick <[email protected]> >>> --- >>> lib/dpif-netdev.c | 2 -- >>> ofproto/ofproto-dpif-upcall.c | 13 ++++++++++--- >>> 2 files changed, 10 insertions(+), 5 deletions(-) >>> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>> index 9a59a1b03..8f7aaffbb 100644 >>> --- a/lib/dpif-netdev.c >>> +++ b/lib/dpif-netdev.c >>> @@ -7980,8 +7980,6 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, >>> struct dp_packet *packet_, >>> ds_destroy(&ds); >>> } >>> >>> - dp_packet_ol_send_prepare(packet_, 0); >>> - >>> return dp->upcall_cb(packet_, flow, ufid, pmd->core_id, type, userdata, >>> actions, wc, put_actions, dp->upcall_aux); >>> } >>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >>> index cc10f57b5..e974d844a 100644 >>> --- a/ofproto/ofproto-dpif-upcall.c >>> +++ b/ofproto/ofproto-dpif-upcall.c >>> @@ -1574,15 +1574,20 @@(struct udpif *udpif, struct upcall *upcall, >>> const struct frozen_state *state = &recirc_node->state; >>> >>> struct ofproto_async_msg *am = xmalloc(sizeof *am); >>> + >>> + /* Resolve offloaded checksums, if any. */ >>> + struct dp_packet *packet_clone = dp_packet_clone(packet); >>> + dp_packet_ol_send_prepare(packet_clone, 0); >> >> Doing a clone (malloc/copy), and doing another malloc and copy below sound >> like a waste of resources. >> >> Can we maybe make this more smart in the dp_netdev_upcall()? >> Something like the below in dp_netdev_upcall(): > > The reason for moving this out of dp_netdev_upcall is resolving the > checksum will modify certain flags that interfere with the software > gso implementation. I could reset the flags for all packets in > dp-packet-gso, but this would not be needed in most packets and would > mean that dp_netdev_upcall packets don't take advantage of checksum > offload at all. I assume only the MISS_UPCALL could get away with its checksum not being correctly calculated for upcall handling. So maybe you can handle this in dp_netdev_upcall() with something like: if (type != MISS_UPCALL) { //store flags / checkums dp_packet_ol_send_prepare(packet_, 0); } rc = dp->upcall_cb(...) if (type != MISS_UPCALL) { //restore flags / checksums } return rc; But this might become a complex piece of code, and your below solution might be cleaner. > Another option would be to add a new function that sets the checksum > in a different buffer than the dp-packet. This would only be used > here, but shouldn't be too complex to implement. > > Probably the first one would be far more straightforward, so unless > you have a preference I'll do that one. Your first solution sounds the more straightforward but you might lose a bit of performance on the upcall packets. If this is fine, this is probably the best one. > > -M > >> >> if (type == CONTROLLER_UPCALL) >> dp_packet_ol_send_prepare(packet_, 0); >> >> >>> *am = (struct ofproto_async_msg) { >>> .controller_id = cookie->controller.controller_id, >>> .oam = OAM_PACKET_IN, >>> .pin = { >>> .up = { >>> .base = { >>> - .packet = xmemdup(dp_packet_data(packet), >>> - dp_packet_size(packet)), >>> - .packet_len = dp_packet_size(packet), >>> + .packet = xmemdup(dp_packet_data(packet_clone), >>> + >>> dp_packet_size(packet_clone)), >>> + .packet_len = dp_packet_size(packet_clone), >>> .reason = cookie->controller.reason, >>> .table_id = state->table_id, >>> .cookie = get_32aligned_be64( >>> @@ -1598,6 +1603,8 @@ process_upcall(struct udpif *udpif, struct upcall >>> *upcall, >>> }, >>> }; >>> >>> + dp_packet_delete(packet_clone); >>> + >>> if (cookie->controller.continuation) { >>> am->pin.up.stack = (state->stack_size >>> ? xmemdup(state->stack, state->stack_size) >>> -- >>> 2.39.3 >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
