On Wed, Jul 6, 2022 at 9:02 AM David Marchand <[email protected]> wrote:
>
> On Mon, Jul 4, 2022 at 10:24 PM David Marchand
> <[email protected]> wrote:
> > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> > > index 57f94df54..164738503 100644
> > > --- a/ofproto/ofproto-dpif-upcall.c
> > > +++ b/ofproto/ofproto-dpif-upcall.c
> > > @@ -215,7 +215,7 @@ struct upcall {
> > > enum odp_key_fitness fitness; /* Fitness of 'flow' relative to ODP
> > > key. */
> > > const ovs_u128 *ufid; /* Unique identifier for 'flow'. */
> > > unsigned pmd_id; /* Datapath poll mode driver id. */
> > > - const struct dp_packet *packet; /* Packet associated with this
> > > upcall. */
> > > + struct dp_packet *packet; /* Packet associated with this
> > > upcall. */
> > > ofp_port_t ofp_in_port; /* OpenFlow in port, or OFPP_NONE. */
> > > uint16_t mru; /* If !0, Maximum receive unit of
> > > fragmented IP packet */
> > > @@ -395,7 +395,7 @@ static void delete_op_init(struct udpif *udpif,
> > > struct ukey_op *op,
> > > struct udpif_key *ukey);
> > >
> > > static int upcall_receive(struct upcall *, const struct dpif_backer *,
> > > - const struct dp_packet *packet, enum
> > > dpif_upcall_type,
> > > + struct dp_packet *packet, enum
> > > dpif_upcall_type,
> > > const struct nlattr *userdata, const struct
> > > flow *,
> > > const unsigned int mru,
> > > const ovs_u128 *ufid, const unsigned pmd_id);
> > > @@ -1140,7 +1140,7 @@ compose_slow_path(struct udpif *udpif, struct
> > > xlate_out *xout,
> > > * since the 'upcall->put_actions' remains uninitialized. */
> > > static int
> > > upcall_receive(struct upcall *upcall, const struct dpif_backer *backer,
> > > - const struct dp_packet *packet, enum dpif_upcall_type
> > > type,
> > > + struct dp_packet *packet, enum dpif_upcall_type type,
> > > const struct nlattr *userdata, const struct flow *flow,
> > > const unsigned int mru,
> > > const ovs_u128 *ufid, const unsigned pmd_id)
> > > @@ -1336,7 +1336,7 @@ should_install_flow(struct udpif *udpif, struct
> > > upcall *upcall)
> > > }
> > >
> > > static int
> > > -upcall_cb(const struct dp_packet *packet, const struct flow *flow,
> > > ovs_u128 *ufid,
> > > +upcall_cb(struct dp_packet *packet, const struct flow *flow, ovs_u128
> > > *ufid,
> > > unsigned pmd_id, enum dpif_upcall_type type,
> > > const struct nlattr *userdata, struct ofpbuf *actions,
> > > struct flow_wildcards *wc, struct ofpbuf *put_actions, void
> > > *aux)
> > > @@ -1446,7 +1446,7 @@ static int
> > > process_upcall(struct udpif *udpif, struct upcall *upcall,
> > > struct ofpbuf *odp_actions, struct flow_wildcards *wc)
> > > {
> > > - const struct dp_packet *packet = upcall->packet;
> > > + struct dp_packet *packet = upcall->packet;
> > > const struct flow *flow = upcall->flow;
> > > size_t actions_len = 0;
> > >
> > > @@ -1524,6 +1524,10 @@ process_upcall(struct udpif *udpif, struct upcall
> > > *upcall,
> > > break;
> > > }
> > >
> > > + /* The packet is going to be encapsulated and sent to
> > > + * the controller. */
> > > + dp_packet_ol_send_prepare(packet, 0);
> >
> > Having to touch the packet data this late is scary, but I don't know a
> > better place for now.
>
> We can move it to dp_netdev_upcall which only affects the userspace datapath.
> Then the upcall handling code gets "complete" packet data, the same as
> when it gets invoked in the kernel datapath case.
>
I think this solution also works; if it's preferred stylistically then
I will make the change for the v5.
Regarding the other comments, I generally agree with the areas you
identified for refactor but think that it would fit best as a later
patch. A lot of this code is modified throughout the patchset, so
applying it now would be difficult.
I propose the next version including:
- conntrack comment
- { on newline
- reset dpdk ol_flags
- relocating dp_packet_ol_send_prepare
And the remaining changes to be added later.
LMKWYT
Cheers,
M
>
> --
> David Marchand
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
