On Wed, Jul 11, 2018 at 05:30:13PM +0530, Sriharsha Basavapatna wrote:
> On Wed, Jul 11, 2018 at 3:33 AM, Ben Pfaff <b...@ovn.org> wrote:
> > On Tue, Jul 10, 2018 at 11:59:35AM +0530, Sriharsha Basavapatna via dev 
> > wrote:
> >> This is the first patch in the patch-set to support dynamic rebalancing
> >> of offloaded flows.
> >>
> >> The patch detects OOR condition on a netdev port when ENOSPC error is
> >> returned by TC-Flower while adding a flow rule. A new structure is added
> >> to the netdev called "netdev_hw_info", to store OOR related information
> >> required to perform dynamic offload-rebalancing.
> >>
> >> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapa...@broadcom.com>
> >> Co-authored-by: Venkat Duvvuru <venkatkumar.duvv...@broadcom.com>
> >> Signed-off-by: Venkat Duvvuru <venkatkumar.duvv...@broadcom.com>
> >> Reviewed-by: Sathya Perla <sathya.pe...@broadcom.com>
> >
> > Thanks for the patch.
> >
> > I spent some time adjusting the style to better fit what we usually do
> > these days in OVS, which puts declarations as close to first use as
> > practical.  I'm appending the incremental that I'd suggest folding in.
> >
> > However I noticed a potentially serious bug while doing it.  Before this
> > patch, the code looked for output actions and took the dst_port from the
> > last one it found that was a tunnel.  After this patch, it does the same
> > thing but it also leaks a netdev for every output action other than the
> > first.  I added a "break;" to mitigate the damage but I guess it's not a
> > correct solution.
> >
> > Thanks,
> >
> > Ben.
> 
> Hi Ben,
> 
> Thanks for catching this issue. This code was based on an earlier assumption
> that multiple output actions won't be specified with offloading; and looks
> like that assumption is no more valid with the fix:
>   "netdev-tc-offloads: Add offloading of multiple outputs"
>   (commit: 00a0a011d328dc7a9ef163ab2066abe697bd1490).
> 
> I've restored the original code for 'outdev' (along with variable declarations
> change that you suggested). I've also removed 'outdev' condition check in our
> code while setting OOR, since that is not really needed (we just need
> in_port/dev).
> 
> Please let me know if you want me to send out the updated patch-set or if I
> should wait for other comments.

Please send the revised patches.

Thanks,

Ben.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to