Thanks Daniele, in this patch we moved the reset of batches[i].flow->batch from dp_netdev_input__() into packet_batch_per_flow_execute(). So before calling dp_netdev_execute_actions() the flow->batch is already NULL.
I think this should prevent recirculation, or am I missing some detail? Antonio > -----Original Message----- > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Daniele Di > Proietto > Sent: Friday, October 7, 2016 11:46 PM > To: Jarno Rajahalme <ja...@ovn.org> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH 05/12] dpif-netdev: Clear flow batches > inside packet_batch_execute. > > This patch basically reverts 603f2ce04d00("dpif-netdev: Clear flow batches > before execute.") > > As explained in that commit message the problem is that > packet_batch_per_flow_execute() can trigger recirculation. This means > that > we will call recursively dp_netdev_input__(). Here's a stack frame: > > dp_netdev_input__() > { > struct packet_batch_per_flow batches[PKT_ARRAY_SIZE]; > /*...*/ > packet_batch_per_flow_execute() > { > dp_execute_cb() > { > case OVS_ACTION_ATTR_RECIRC: > dp_netdev_input__() { > struct packet_batch_per_flow > batches[PKT_ARRAY_SIZE]; > /*...*/ > } > } > > } > > In the inner dp_netdev_input__() a lookup could theoretically find the > same > flows that it finds in the outer. > If we don't clear _all_ the batch pointers before calling the inner > dp_netdev_input__() we could find a flow that still has a pointer to a > batch in the outer dp_netdev_input__(). Then we will append packets to the > outer batch, which is clearly wrong. > > 2016-10-07 14:09 GMT-07:00 Jarno Rajahalme <ja...@ovn.org>: > > > Daniele had a comment on this, I believe? > > > > > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy < > > bhanuprakash.bodire...@intel.com> wrote: > > > > > > There is a slight negative performance impact, by zeroing out the flow > > > batch pointers in dp_netdev_input__ ahead of packet_batch_execute(). > The > > > issue has been observed with multiple batches test scenario. > > > > > > This patch fixes the problem by removing the extra for loop and clear > > > the flow batches inside the packet_batch_per_flow_execute(). Also the > > > vtune analysis showed that the overall no. of instructions retired for > > > dp_netdev_input__ reduced by 1,273,800,000 with this patch. > > > > > > Fixes: 603f2ce04d00 ("dpif-netdev: Clear flow batches before > execute.") > > > Signed-off-by: Bhanuprakash Bodireddy > <bhanuprakash.bodire...@intel.com> > > > Signed-off-by: Antonio Fischetti <antonio.fische...@intel.com> > > > --- > > > lib/dpif-netdev.c | 5 +---- > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > > index c002dd3..d0bb191 100644 > > > --- a/lib/dpif-netdev.c > > > +++ b/lib/dpif-netdev.c > > > @@ -3878,6 +3878,7 @@ packet_batch_per_flow_execute(struct > > packet_batch_per_flow *batch, > > > { > > > struct dp_netdev_actions *actions; > > > struct dp_netdev_flow *flow = batch->flow; > > > + flow->batch = NULL; > > > > > > dp_netdev_flow_used(flow, batch->array.count, batch->byte_count, > > > batch->tcp_flags, now); > > > @@ -4173,10 +4174,6 @@ dp_netdev_input__(struct dp_netdev_pmd_thread > > *pmd, > > > } > > > > > > for (i = 0; i < n_batches; i++) { > > > - batches[i].flow->batch = NULL; > > > - } > > > - > > > - for (i = 0; i < n_batches; i++) { > > > packet_batch_per_flow_execute(&batches[i], pmd, now); > > > } > > > } > > > -- > > > 2.4.11 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > http://openvswitch.org/mailman/listinfo/dev > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
