On Fri, Dec 15, 2023 at 8:01 AM Eelco Chaudron <echau...@redhat.com> 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 <m...@redhat.com>
> > ---
> > 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 @@ process_upcall(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.
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.
-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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
