| Message ID | 20220128161447.270575-2-aconole@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/5] ofproto-dpif: fix packet_execute_prepare | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Bleep bloop. Greetings Aaron Conole, 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: ERROR: Author Peng He <xnhp0320@gmail.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He <hepeng.0320@bytedance.com>, Aaron Conole <aconole@redhat.com> Lines checked: 35, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 1/28/22 17:14, Aaron Conole wrote: > From: Peng He <xnhp0320@gmail.com> > > In the commit 1df7f7aac8c976690167261fec9a50d832ef9e7e, the packet > metadata's in_port has been changed from ofp port into odp port, > however, the odp->flow->in_port is still ofp_port. This patch changes > the odp->flow->in_port into odp_port. > > Fixes: 1df7f7aac8 ("ofproto-dpif: Restore packet metadata when a > continuation is resumed.") Nit: please, don't wrap the tags. I'm sorry, but I'm getting trouble relating the mentioned commit with the code change here. Commit 1df7f7aac8 made only cosmetic change to the 'execution' path in order to split out the function. The real change was made only for the packet resume, which doesn't seem to be the case for the current patch. I managed to trace the in_port update in the 'execution' path down to commit 758c456df570 ("dpif: Use explicit packet metadata."). It significantly changed the way code is structured, so it's more likely a commit to blame. I don't think that commit message here makes sense. So, I'm not sure what exactly this patch is fixing. Am I missing something? I think, we need a new commit message that describes what actually the problem is and how it affects packet processing. And we definitely need a unit test for that patch. Best regards, Ilya Maximets. > Signed-off-by: Peng He <hepeng.0320@bytedance.com> > Acked-by: Mike Pattrick <mkp@redhat.com> > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > ofproto/ofproto-dpif.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 8143dd965f..fed037b8d9 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -5011,6 +5011,7 @@ packet_execute_prepare(struct ofproto *ofproto_, > ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port, > opo->packet); > > + opo->flow->in_port = opo->packet->md.in_port; > ofproto_dpif_packet_out_delete(aux); > opo->aux = execute; > }
Ilya Maximets <i.maximets@ovn.org> 于2022年2月12日周六 05:34写道： > On 1/28/22 17:14, Aaron Conole wrote: > > From: Peng He <xnhp0320@gmail.com> > > > > In the commit 1df7f7aac8c976690167261fec9a50d832ef9e7e, the packet > > metadata's in_port has been changed from ofp port into odp port, > > however, the odp->flow->in_port is still ofp_port. This patch changes > > the odp->flow->in_port into odp_port. > > > > Fixes: 1df7f7aac8 ("ofproto-dpif: Restore packet metadata when a > > continuation is resumed.") > > Nit: please, don't wrap the tags. > I'm sorry, but I'm getting trouble relating the mentioned commit > with the code change here. Commit 1df7f7aac8 made only cosmetic > change to the 'execution' path in order to split out the function. > The real change was made only for the packet resume, which doesn't > seem to be the case for the current patch. > In the commit 1df7f7aac8, it adds a function to set the correct in_port in the packet md: +static void +ofproto_dpif_set_packet_odp_port(const struct ofproto_dpif *ofproto, + ofp_port_t in_port, struct dp_packet *packet) +{ + if (in_port == OFPP_NONE) { + in_port = OFPP_LOCAL; + } + packet->md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port); +} + however it forgets to change the in_port field in the flow struct. This is ok when the ipf_ctx patch is not added. If the ipf_ctx is added, it will use flow's in_port as part of ipf_ctx. If this patch is not added, the ipf_ctx patch will break some testsuites which uses packetout to inject a test packet. > I managed to trace the in_port update in the 'execution' path down > to commit 758c456df570 ("dpif: Use explicit packet metadata."). > It significantly changed the way code is structured, so it's more > likely a commit to blame. > > I don't think that commit message here makes sense. So, I'm not > sure what exactly this patch is fixing. Am I missing something? > > I think, we need a new commit message that describes what actually > the problem is and how it affects packet processing. And we > definitely need a unit test for that patch. > > Best regards, Ilya Maximets. > > > Signed-off-by: Peng He <hepeng.0320@bytedance.com> > > Acked-by: Mike Pattrick <mkp@redhat.com> > > Signed-off-by: Aaron Conole <aconole@redhat.com> > > --- > > ofproto/ofproto-dpif.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 8143dd965f..fed037b8d9 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -5011,6 +5011,7 @@ packet_execute_prepare(struct ofproto *ofproto_, > > ofproto_dpif_set_packet_odp_port(ofproto, > opo->flow->in_port.ofp_port, > > opo->packet); > > > > + opo->flow->in_port = opo->packet->md.in_port; > > ofproto_dpif_packet_out_delete(aux); > > opo->aux = execute; > > } > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 2/13/22 03:32, Peng He wrote: > > > Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> 于2022年2月12日周六 05:34写道： > > On 1/28/22 17:14, Aaron Conole wrote: > > From: Peng He <xnhp0320@gmail.com <mailto:xnhp0320@gmail.com>> > > > > In the commit 1df7f7aac8c976690167261fec9a50d832ef9e7e, the packet > > metadata's in_port has been changed from ofp port into odp port, > > however, the odp->flow->in_port is still ofp_port. This patch changes > > the odp->flow->in_port into odp_port. > > > > Fixes: 1df7f7aac8 ("ofproto-dpif: Restore packet metadata when a > > continuation is resumed.") > > Nit: please, don't wrap the tags. > > > I'm sorry, but I'm getting trouble relating the mentioned commit > with the code change here. Commit 1df7f7aac8 made only cosmetic > change to the 'execution' path in order to split out the function. > The real change was made only for the packet resume, which doesn't > seem to be the case for the current patch. > > > In the commit 1df7f7aac8, it adds a function to set the correct in_port > in the packet md: Yes, but that is for 'resume' code path, not for 'execute'. Code change for the 'execute' path in the commit 1df7f7aac8 looks like this: @@ -3704,11 +3713,7 @@ ofproto_dpif_execute_actions__(struct ofproto_dpif *ofproto, execute.mtu = 0; /* Fix up in_port. */ - in_port = flow->in_port.ofp_port; - if (in_port == OFPP_NONE) { - in_port = OFPP_LOCAL; - } - execute.packet->md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port); + ofproto_dpif_set_packet_odp_port(ofproto, flow->in_port.ofp_port, packet); error = dpif_execute(ofproto->backer->dpif, &execute); out: --- Meaning, the in_port update was there already and was only replaced with a function to avoid code duplication. > > +static void > +ofproto_dpif_set_packet_odp_port(const struct ofproto_dpif *ofproto, > + ofp_port_t in_port, struct dp_packet *packet) > +{ > + if (in_port == OFPP_NONE) { > + in_port = OFPP_LOCAL; > + } > + packet->md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port); > +} > + > > however it forgets to change the in_port field in the flow struct. This is ok when > the ipf_ctx patch is not added. If the ipf_ctx is added, it will use flow's in_port as > part of ipf_ctx. I'm not sure if "forgets" is a correct word here. AFAICT, 'in_port' is an odp_port only when flow structure is constructed out of netlink attributes, i.e. from the datapath flow. And that is not the case here as we're dealing with the OpenFlow match provided by the OF controller. It might be better to not touch this structure and use the packet metadata instead in the datapath code. (If you have a batch instead of a single packet in the datapath, we can get the in_port from the metadata of the first packet in that batch. Datapath code already assumes that all the packets in the batch has the same origin.) > > If this patch is not added, the ipf_ctx patch will break some testsuites which uses > packetout to inject a test packet. > > > > I managed to trace the in_port update in the 'execution' path down > to commit 758c456df570 ("dpif: Use explicit packet metadata."). > It significantly changed the way code is structured, so it's more > likely a commit to blame. > > I don't think that commit message here makes sense. So, I'm not > sure what exactly this patch is fixing. Am I missing something? > > I think, we need a new commit message that describes what actually > the problem is and how it affects packet processing. And we > definitely need a unit test for that patch. > > Best regards, Ilya Maximets. > > > Signed-off-by: Peng He <hepeng.0320@bytedance.com <mailto:hepeng.0320@bytedance.com>> > > Acked-by: Mike Pattrick <mkp@redhat.com <mailto:mkp@redhat.com>> > > Signed-off-by: Aaron Conole <aconole@redhat.com <mailto:aconole@redhat.com>> > > --- > > ofproto/ofproto-dpif.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 8143dd965f..fed037b8d9 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -5011,6 +5011,7 @@ packet_execute_prepare(struct ofproto *ofproto_, > > ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port, > > opo->packet); > > > > + opo->flow->in_port = opo->packet->md.in_port; > > ofproto_dpif_packet_out_delete(aux); > > opo->aux = execute; > > } > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > > -- > hepeng
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 8143dd965f..fed037b8d9 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5011,6 +5011,7 @@ packet_execute_prepare(struct ofproto *ofproto_, ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port, opo->packet); + opo->flow->in_port = opo->packet->md.in_port; ofproto_dpif_packet_out_delete(aux); opo->aux = execute; }