Pravin, It may be good to add a comment pointing out that we don't expect flow->mask to be valid when calling from the packet execute() context.
On Wed, Jan 29, 2014 at 7:18 PM, Jesse Gross <je...@nicira.com> wrote: > On Wed, Jan 29, 2014 at 4:42 PM, Pravin Shelar <pshe...@nicira.com> wrote: > > On Wed, Jan 29, 2014 at 4:38 PM, Jesse Gross <je...@nicira.com> wrote: > >> On Wed, Jan 29, 2014 at 4:35 PM, Pravin Shelar <pshe...@nicira.com> > wrote: > >>> On Wed, Jan 29, 2014 at 4:21 PM, Jesse Gross <je...@nicira.com> wrote: > >>>> On Wed, Jan 29, 2014 at 11:22 AM, Pravin Shelar <pshe...@nicira.com> > wrote: > >>>>> On Wed, Jan 29, 2014 at 10:44 AM, Jesse Gross <je...@nicira.com> > wrote: > >>>>>> On Wed, Jan 29, 2014 at 9:20 AM, <pshe...@nicira.com> wrote: > >>>>>>> From: Pravin <pshe...@nicira.com> > >>>>>>> > >>>>>>> ovs_flow_free() is not called under ovs-lock during packet > >>>>>>> execute path. Since packet execute does not touch flow->mask, > >>>>>>> there is no need to take that lock either. So move assert in > >>>>>>> case where flow->mask is checked. > >>>>>>> > >>>>>>> Found by code inspection. > >>>>>>> > >>>>>>> Signed-off-by: Pravin B Shelar <pshe...@nicira.com> > >>>>>> > >>>>>> The idea for putting it here is that callers should always hold OVS > >>>>>> mutex and by putting the check earlier it increases the chances that > >>>>>> problems will be caught sooner. > >>>>> > >>>>> But it is not protecting any data outside of that flow->mask case, so > >>>>> it is not required. > >>>> > >>>> I agree that it's not required but I don't understand the benefit to > >>>> removing it. Are there cases where we want to call this function where > >>>> OVS mutex isn't held because there is no flow mask? > >>> > >>> I kept it because it was very recently added. I am ok with removing > >>> it. I will send patch. > >> > >> I think we are saying the opposite - I don't understand why we want to > >> remove it/make it trigger less often. > > > > yes, from ovs_packet_cmd_execute(). > > Ah, OK. When I read packet execute in the commit message, I thought > that you were referring to general packet processing. I understand the > problem now - can you update the commit message with the specific > function name? > > Acked-by: Jesse Gross <je...@nicira.com> > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
