| Message ID | 0f0989dedc00b85af688df35ab0d444c09093a5c.1581611906.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] pinctrl: fix IP buffering with connection-tracking | expand |
Bleep bloop. Greetings Lorenzo Bianconi, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Numan Siddique <numans@ovn.org> Lines checked: 62, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Thu, Feb 13, 2020 at 05:49:37PM +0100, Lorenzo Bianconi wrote: > Whenever we need to reinject an IP packet buffered during L2 address > resolution we need to preserve ovs ofport in order to let ovs > connection tracking to properly SNAT/DNAT the packet. > Do not overwrite the MFF_IN_PORT in consider_port_binding routine > > Signed-off-by: Numan Siddique <numans@ovn.org> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> The sign-offs are a bit odd here. Should one be an Acked-by?
On Fri, Feb 14, 2020 at 12:43 AM Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Feb 13, 2020 at 05:49:37PM +0100, Lorenzo Bianconi wrote: > > Whenever we need to reinject an IP packet buffered during L2 address > > resolution we need to preserve ovs ofport in order to let ovs > > connection tracking to properly SNAT/DNAT the packet. > > Do not overwrite the MFF_IN_PORT in consider_port_binding routine > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > The sign-offs are a bit odd here. Should one be an Acked-by? Hi Ben, I had suggested him this approach to solve this problem. Probably he can use the tag - "Suggested-by: Numan Siddique <numans@ovn.org>" Thanks Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> > On Fri, Feb 14, 2020 at 12:43 AM Ben Pfaff <blp@ovn.org> wrote: > > > > On Thu, Feb 13, 2020 at 05:49:37PM +0100, Lorenzo Bianconi wrote: > > > Whenever we need to reinject an IP packet buffered during L2 address > > > resolution we need to preserve ovs ofport in order to let ovs > > > connection tracking to properly SNAT/DNAT the packet. > > > Do not overwrite the MFF_IN_PORT in consider_port_binding routine > > > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > The sign-offs are a bit odd here. Should one be an Acked-by? > > Hi Ben, > > I had suggested him this approach to solve this problem. Probably he > can use the tag - "Suggested-by: Numan Siddique <numans@ovn.org>" > > Thanks > Numan yes, sorry for the noise Ben. Do I need to resubmit or is it ok to fix this patch? Regards, Lorenzo > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On Fri, Feb 14, 2020 at 12:38:54AM +0100, Lorenzo Bianconi wrote: > > > > On Fri, Feb 14, 2020 at 12:43 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Thu, Feb 13, 2020 at 05:49:37PM +0100, Lorenzo Bianconi wrote: > > > > Whenever we need to reinject an IP packet buffered during L2 address > > > > resolution we need to preserve ovs ofport in order to let ovs > > > > connection tracking to properly SNAT/DNAT the packet. > > > > Do not overwrite the MFF_IN_PORT in consider_port_binding routine > > > > > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > > > The sign-offs are a bit odd here. Should one be an Acked-by? > > > > Hi Ben, > > > > I had suggested him this approach to solve this problem. Probably he > > can use the tag - "Suggested-by: Numan Siddique <numans@ovn.org>" > > > > Thanks > > Numan > > yes, sorry for the noise Ben. > Do I need to resubmit or is it ok to fix this patch? No need, I can fix that bit.
On 2/13/20 5:49 PM, Lorenzo Bianconi wrote: > Whenever we need to reinject an IP packet buffered during L2 address > resolution we need to preserve ovs ofport in order to let ovs > connection tracking to properly SNAT/DNAT the packet. > Do not overwrite the MFF_IN_PORT in consider_port_binding routine > > Signed-off-by: Numan Siddique <numans@ovn.org> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Hi Lorenzo, The code looks good to me. I couldn't find any side effects due to not resetting MFF_IN_PORT when entering the router pipeline but I'm wondering if it's possible to add a test case for the issue you're fixing. This way, if we did miss a case when the now non-zero in_port is causing problems in the router pipeline we at least know that zeroing it out will trigger the new test to fail. Thanks, Dumitru > --- > controller/physical.c | 1 - > controller/pinctrl.c | 5 ++++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/controller/physical.c b/controller/physical.c > index a7edaddac..144aeb7bd 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -905,7 +905,6 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > for (int i = 0; i < MFF_N_LOG_REGS; i++) { > put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p); > } > - put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p); > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone); > ofpacts_p->header = clone; > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 09b0f7bf7..d06915a65 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -559,6 +559,7 @@ set_actions_and_enqueue_msg(struct rconn *swconn, > > struct buffer_info { > struct ofpbuf ofpacts; > + ofp_port_t ofp_port; > struct dp_packet *p; > }; > > @@ -629,6 +630,8 @@ buffered_push_packet(struct buffered_packets *bp, > ofpbuf_init(&bi->ofpacts, 4096); > > reload_metadata(&bi->ofpacts, md); > + bi->ofp_port = md->flow.in_port.ofp_port; > + > struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&bi->ofpacts); > resubmit->in_port = OFPP_CONTROLLER; > resubmit->table_id = OFTABLE_REMOTE_OUTPUT; > @@ -663,7 +666,7 @@ buffered_send_packets(struct rconn *swconn, struct buffered_packets *bp, > .ofpacts = bi->ofpacts.data, > .ofpacts_len = bi->ofpacts.size, > }; > - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); > + match_set_in_port(&po.flow_metadata, bi->ofp_port); > queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); > > ofpbuf_uninit(&bi->ofpacts); >
On Feb 19, Dumitru Ceara wrote: > On 2/13/20 5:49 PM, Lorenzo Bianconi wrote: > > Whenever we need to reinject an IP packet buffered during L2 address > > resolution we need to preserve ovs ofport in order to let ovs > > connection tracking to properly SNAT/DNAT the packet. > > Do not overwrite the MFF_IN_PORT in consider_port_binding routine > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Hi Lorenzo, Hi Dumitru, thanks for the review :) > > The code looks good to me. > > I couldn't find any side effects due to not resetting MFF_IN_PORT when > entering the router pipeline but I'm wondering if it's possible to add a > test case for the issue you're fixing. I guess test-cases are already in system-ovn.at: - ovn -- DNAT and SNAT on distributed router - N/S - ovn -- DNAT and SNAT on distributed router - N/S - IPv6 - ovn -- DNAT and SNAT on distributed router - E/W - ovn -- DNAT and SNAT on distributed router - E/W - IPv6 but they actually run a single device. Regards, Lorenzo > > This way, if we did miss a case when the now non-zero in_port is causing > problems in the router pipeline we at least know that zeroing it out > will trigger the new test to fail. > > Thanks, > Dumitru > > > --- > > controller/physical.c | 1 - > > controller/pinctrl.c | 5 ++++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/controller/physical.c b/controller/physical.c > > index a7edaddac..144aeb7bd 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -905,7 +905,6 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > for (int i = 0; i < MFF_N_LOG_REGS; i++) { > > put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p); > > } > > - put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p); > > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > > clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone); > > ofpacts_p->header = clone; > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 09b0f7bf7..d06915a65 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -559,6 +559,7 @@ set_actions_and_enqueue_msg(struct rconn *swconn, > > > > struct buffer_info { > > struct ofpbuf ofpacts; > > + ofp_port_t ofp_port; > > struct dp_packet *p; > > }; > > > > @@ -629,6 +630,8 @@ buffered_push_packet(struct buffered_packets *bp, > > ofpbuf_init(&bi->ofpacts, 4096); > > > > reload_metadata(&bi->ofpacts, md); > > + bi->ofp_port = md->flow.in_port.ofp_port; > > + > > struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&bi->ofpacts); > > resubmit->in_port = OFPP_CONTROLLER; > > resubmit->table_id = OFTABLE_REMOTE_OUTPUT; > > @@ -663,7 +666,7 @@ buffered_send_packets(struct rconn *swconn, struct buffered_packets *bp, > > .ofpacts = bi->ofpacts.data, > > .ofpacts_len = bi->ofpacts.size, > > }; > > - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); > > + match_set_in_port(&po.flow_metadata, bi->ofp_port); > > queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); > > > > ofpbuf_uninit(&bi->ofpacts); > > >
On 2/19/20 11:02 AM, Lorenzo Bianconi wrote: > On Feb 19, Dumitru Ceara wrote: >> On 2/13/20 5:49 PM, Lorenzo Bianconi wrote: >>> Whenever we need to reinject an IP packet buffered during L2 address >>> resolution we need to preserve ovs ofport in order to let ovs >>> connection tracking to properly SNAT/DNAT the packet. >>> Do not overwrite the MFF_IN_PORT in consider_port_binding routine >>> >>> Signed-off-by: Numan Siddique <numans@ovn.org> >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >> >> Hi Lorenzo, > > Hi Dumitru, > > thanks for the review :) > >> >> The code looks good to me. >> >> I couldn't find any side effects due to not resetting MFF_IN_PORT when >> entering the router pipeline but I'm wondering if it's possible to add a >> test case for the issue you're fixing. > > I guess test-cases are already in system-ovn.at: > > - ovn -- DNAT and SNAT on distributed router - N/S > - ovn -- DNAT and SNAT on distributed router - N/S - IPv6 > - ovn -- DNAT and SNAT on distributed router - E/W > - ovn -- DNAT and SNAT on distributed router - E/W - IPv6 > > but they actually run a single device. > > Regards, > Lorenzo I see, thanks. Acked-by: Dumitru Ceara <dceara@redhat.com> > >> >> This way, if we did miss a case when the now non-zero in_port is causing >> problems in the router pipeline we at least know that zeroing it out >> will trigger the new test to fail. >> >> Thanks, >> Dumitru >> >>> --- >>> controller/physical.c | 1 - >>> controller/pinctrl.c | 5 ++++- >>> 2 files changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/controller/physical.c b/controller/physical.c >>> index a7edaddac..144aeb7bd 100644 >>> --- a/controller/physical.c >>> +++ b/controller/physical.c >>> @@ -905,7 +905,6 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, >>> for (int i = 0; i < MFF_N_LOG_REGS; i++) { >>> put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p); >>> } >>> - put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p); >>> put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); >>> clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone); >>> ofpacts_p->header = clone; >>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >>> index 09b0f7bf7..d06915a65 100644 >>> --- a/controller/pinctrl.c >>> +++ b/controller/pinctrl.c >>> @@ -559,6 +559,7 @@ set_actions_and_enqueue_msg(struct rconn *swconn, >>> >>> struct buffer_info { >>> struct ofpbuf ofpacts; >>> + ofp_port_t ofp_port; >>> struct dp_packet *p; >>> }; >>> >>> @@ -629,6 +630,8 @@ buffered_push_packet(struct buffered_packets *bp, >>> ofpbuf_init(&bi->ofpacts, 4096); >>> >>> reload_metadata(&bi->ofpacts, md); >>> + bi->ofp_port = md->flow.in_port.ofp_port; >>> + >>> struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&bi->ofpacts); >>> resubmit->in_port = OFPP_CONTROLLER; >>> resubmit->table_id = OFTABLE_REMOTE_OUTPUT; >>> @@ -663,7 +666,7 @@ buffered_send_packets(struct rconn *swconn, struct buffered_packets *bp, >>> .ofpacts = bi->ofpacts.data, >>> .ofpacts_len = bi->ofpacts.size, >>> }; >>> - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); >>> + match_set_in_port(&po.flow_metadata, bi->ofp_port); >>> queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); >>> >>> ofpbuf_uninit(&bi->ofpacts); >>> >>
On Wed, Feb 19, 2020 at 4:29 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On 2/19/20 11:02 AM, Lorenzo Bianconi wrote: > > On Feb 19, Dumitru Ceara wrote: > >> On 2/13/20 5:49 PM, Lorenzo Bianconi wrote: > >>> Whenever we need to reinject an IP packet buffered during L2 address > >>> resolution we need to preserve ovs ofport in order to let ovs > >>> connection tracking to properly SNAT/DNAT the packet. > >>> Do not overwrite the MFF_IN_PORT in consider_port_binding routine > >>> > >>> Signed-off-by: Numan Siddique <numans@ovn.org> > >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > >> > >> Hi Lorenzo, > > > > Hi Dumitru, > > > > thanks for the review :) > > > >> > >> The code looks good to me. > >> > >> I couldn't find any side effects due to not resetting MFF_IN_PORT when > >> entering the router pipeline but I'm wondering if it's possible to add a > >> test case for the issue you're fixing. > > > > I guess test-cases are already in system-ovn.at: > > > > - ovn -- DNAT and SNAT on distributed router - N/S > > - ovn -- DNAT and SNAT on distributed router - N/S - IPv6 > > - ovn -- DNAT and SNAT on distributed router - E/W > > - ovn -- DNAT and SNAT on distributed router - E/W - IPv6 > > > > but they actually run a single device. > > > > Regards, > > Lorenzo > > I see, thanks. > > Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks for the review Dumitru. I applied this patch to master and branch-20.03 Thanks Numan > > > > >> > >> This way, if we did miss a case when the now non-zero in_port is causing > >> problems in the router pipeline we at least know that zeroing it out > >> will trigger the new test to fail. > >> > >> Thanks, > >> Dumitru > >> > >>> --- > >>> controller/physical.c | 1 - > >>> controller/pinctrl.c | 5 ++++- > >>> 2 files changed, 4 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/controller/physical.c b/controller/physical.c > >>> index a7edaddac..144aeb7bd 100644 > >>> --- a/controller/physical.c > >>> +++ b/controller/physical.c > >>> @@ -905,7 +905,6 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, > >>> for (int i = 0; i < MFF_N_LOG_REGS; i++) { > >>> put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p); > >>> } > >>> - put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p); > >>> put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > >>> clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone); > >>> ofpacts_p->header = clone; > >>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c > >>> index 09b0f7bf7..d06915a65 100644 > >>> --- a/controller/pinctrl.c > >>> +++ b/controller/pinctrl.c > >>> @@ -559,6 +559,7 @@ set_actions_and_enqueue_msg(struct rconn *swconn, > >>> > >>> struct buffer_info { > >>> struct ofpbuf ofpacts; > >>> + ofp_port_t ofp_port; > >>> struct dp_packet *p; > >>> }; > >>> > >>> @@ -629,6 +630,8 @@ buffered_push_packet(struct buffered_packets *bp, > >>> ofpbuf_init(&bi->ofpacts, 4096); > >>> > >>> reload_metadata(&bi->ofpacts, md); > >>> + bi->ofp_port = md->flow.in_port.ofp_port; > >>> + > >>> struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&bi->ofpacts); > >>> resubmit->in_port = OFPP_CONTROLLER; > >>> resubmit->table_id = OFTABLE_REMOTE_OUTPUT; > >>> @@ -663,7 +666,7 @@ buffered_send_packets(struct rconn *swconn, struct buffered_packets *bp, > >>> .ofpacts = bi->ofpacts.data, > >>> .ofpacts_len = bi->ofpacts.size, > >>> }; > >>> - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); > >>> + match_set_in_port(&po.flow_metadata, bi->ofp_port); > >>> queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); > >>> > >>> ofpbuf_uninit(&bi->ofpacts); > >>> > >> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/physical.c b/controller/physical.c index a7edaddac..144aeb7bd 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -905,7 +905,6 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name, for (int i = 0; i < MFF_N_LOG_REGS; i++) { put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p); } - put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p); put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone); ofpacts_p->header = clone; diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 09b0f7bf7..d06915a65 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -559,6 +559,7 @@ set_actions_and_enqueue_msg(struct rconn *swconn, struct buffer_info { struct ofpbuf ofpacts; + ofp_port_t ofp_port; struct dp_packet *p; }; @@ -629,6 +630,8 @@ buffered_push_packet(struct buffered_packets *bp, ofpbuf_init(&bi->ofpacts, 4096); reload_metadata(&bi->ofpacts, md); + bi->ofp_port = md->flow.in_port.ofp_port; + struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&bi->ofpacts); resubmit->in_port = OFPP_CONTROLLER; resubmit->table_id = OFTABLE_REMOTE_OUTPUT; @@ -663,7 +666,7 @@ buffered_send_packets(struct rconn *swconn, struct buffered_packets *bp, .ofpacts = bi->ofpacts.data, .ofpacts_len = bi->ofpacts.size, }; - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); + match_set_in_port(&po.flow_metadata, bi->ofp_port); queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); ofpbuf_uninit(&bi->ofpacts);