On Mon, Dec 18, 2023 at 5:04 AM Eelco Chaudron <[email protected]> wrote:
>
>
>
> 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.
Initially I was worried about the performance in setting the flags,
but as we have to read/write the flags anyways while segmenting it
shouldn't noticeably impact things to do a second read/write. I'll
make that change for the v2.
And as Ilya points out, there could be other cases that I'm not
anticipating with upcall_cb().
-M
>
> >
> > -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
