This patch seems to work fine. Please feel free to push.
I have two concerns mainly on the style side:
1. In the same file flow.c, we are mixing the _deferred_ functions with the
style of passing 'deferred' as a parameter in the same file. The
correctness of caller in setting 'deferred' value does not look obviously
correct.
2. In function ovw_flow_free()
...
ovs_sw_flow_mask_del_ref((struct sw_flow_mask __force *)flow->mask,
deferred);
...
The type cast to (flow->mask) does not look good to me. It also defeats
some useful lockdep checks.
I am sure you are aware of those trade-offs and have determined that the
benefits out weights any style concerns, right?
--andy
On Wed, Jun 19, 2013 at 6:31 PM, Jesse Gross <je...@nicira.com> wrote:
> OK, that idea turned out to be a bad one since masks really do have a
> lifetime that is separate from an individual flow. However, here's a
> patch that does the unification a different way.
>
> What do you think?
>
> On Wed, Jun 19, 2013 at 4:32 PM, Jesse Gross <je...@nicira.com> wrote:
> > I think at this point, the use of a separate RCU callback for the
> > masks is hurting more than it is helping and it would be easier to
> > just piggyback off the flow/table destroy RCU. I'll try to write a
> > patch to see if I can get it to work.
> >
> > On Wed, Jun 19, 2013 at 4:23 PM, Andy Zhou <az...@nicira.com> wrote:
> >> When table is destroyed, there is really no need to keep track of the
> mask's
> >> reference counting -- The masks will have to be destroyed any ways.
> >>
> >> During run time, the mask_list deletion needs to be protected by the
> >> ovs_lock, But we should not require ovs_lock to be held during table
> >> destroy. right? So it seems we will have two ways of doing
> ovs_flow_free()
> >> any ways,
> >>
> >> I will continue to think about how to merge these two cases. Please let
> me
> >> know if you have any suggestions.
> >>
> >> --andy
> >>
> >>
> >> On Wed, Jun 19, 2013 at 4:09 PM, Jesse Gross <je...@nicira.com> wrote:
> >>>
> >>> What is the reason need for iterating through the mask list itself
> >>> when destroying the table instead of doing it as the flows are
> >>> deleted? Doing it that way would both avoid the need to have multiple
> >>> list deletion functions and the differences between the RCU and
> >>> non-RCU versions of ovs_flow_free().
> >>>
> >>> One issue that comes to mind is the RCU barrier when the module is
> >>> unloaded but there are other ways to fix that.
> >>>
> >>> On Wed, Jun 19, 2013 at 2:09 PM, Andy Zhou <az...@nicira.com> wrote:
> >>> > Jesse,
> >>> >
> >>> > The following patch is an incremental patch on top of yours.
> >>> > -- Fix a few small bugs introduced by the cleanup patch.
> >>> > -- Make mask list per table.
> >>> >
> >>> > I believe this implementation will address the rcu lock issues we
> >>> > discussed
> >>> > off line.
> >>> >
> >>> > Would you please review?
> >>> >
> >>> > Thanks,
> >>> >
> >>> > --andy
> >>> >
> >>> > On Tue, Jun 18, 2013 at 4:35 PM, Jesse Gross <je...@nicira.com>
> wrote:
> >>> >>
> >>> >> On Tue, Jun 18, 2013 at 4:15 PM, Andy Zhou <az...@nicira.com>
> wrote:
> >>> >> > Add wildcarded flow support in kernel datapath.
> >>> >> >
> >>> >> > Wildcarded flow can improve OVS flow set up performance by avoid
> >>> >> > sending
> >>> >> > matching new flows to the user space program. The exact
> performance
> >>> >> > boost
> >>> >> > will largely dependent on wildcarded flow hit rate.
> >>> >> >
> >>> >> > In case all new flows hits wildcard flows, the flow set up rate is
> >>> >> > within 5% of that of linux bridge module.
> >>> >> >
> >>> >> > Pravin has made significant contributions to this patch. Including
> >>> >> > API
> >>> >> > clean ups and bug fixes.
> >>> >> >
> >>> >> > Co-authored-by: Pravin B Shelar <pshe...@nicira.com>
> >>> >> > Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
> >>> >> > Signed-off-by: Andy Zhou <az...@nicira.com>
> >>> >>
> >>> >> I made some incremental changes, can you take a look to see if they
> >>> >> seem reasonable to you?
> >>> >>
> >>> >> Here are the main blocks:
> >>> >> - Additional interface documentation.
> >>> >> - Fix memory leak when installing a new flow if there is an
> >>> >> allocation failure for the mask.
> >>> >> - Require Ethernet addresses to be present in the key, as before.
> >>> >> - Make SNAP handling a little more integrated with the rest of the
> >>> >> parsing and validation code.
> >>> >> - Make ovs_flow_free() and ovs_deferred_flow_free() have exactly
> the
> >>> >> same behavior, just with the addition of RCU, to avoid possible
> >>> >> confusion.
> >>> >> - Return errors directly rather than through an intermediate
> variable
> >>> >> in ovs_flow_extract() and children.
> >>> >> - Enforce an exact match for the outer EtherType for vlan packets,
> >>> >> similar to what we do for other protocols.
> >>> >> - Miscellaneous style fixes.
> >>> >
> >>> >
> >>
> >>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
