[ovs-dev] [bugfixes 3/3] datapath: Dump flow actions only if there is room.

Ben Pfaff blp at nicira.com
Tue Feb 1 17:18:54 UTC 2011


On Tue, Feb 01, 2011 at 09:07:42AM -0800, Jesse Gross wrote:
> On Tue, Feb 1, 2011 at 8:12 AM, Ben Pfaff <blp at nicira.com> wrote:
> > On Mon, Jan 31, 2011 at 08:50:05PM -0800, Jesse Gross wrote:
> >> On Mon, Jan 31, 2011 at 8:25 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > On Mon, Jan 31, 2011 at 06:44:16PM -0800, Jesse Gross wrote:
> >> >> On Mon, Jan 31, 2011 at 4:48 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> >> > Expanding an skbuff in a netlink dump handler doesn't work well. ?First,
> >> >> > we weren't updating the truesize of the skb or the allocation within the
> >> >> > socket that netlink_dump() had put the skb in. ?Even when I fixed that, I
> >> >> > still got mysterious crashes and reboots. ?I don't know why these happened.
> >> >> >
> >> >> > This commit fixes the problem (in my tests, anyway) by avoiding expanding
> >> >> > the reply skbuff to fill in the actions. ?Instead, in such a case the
> >> >> > userspace client has to do a separate "get" action to get the actions.
> >> >> > This commit also updates userspace to do this automatically for dumps in
> >> >> > the cases where the caller cares (only "ovs-dpctl dump-flows" currently
> >> >> > cares).
> >> >> >
> >> >> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> >> >>
> >> >> This looks fine to me. ?However, I think that I see the problem in the
> >> >> original code that is causing this error.
> >> >
> >> > Gee, I was really dumb. ?How did I miss that?
> >> >
> >> > Would you prefer to fix the original problem or use this patch? ?I can
> >> > see arguments both ways. ?I have a feeling that this patch is more
> >> > upstream-able than the previous version.
> >>
> >> I agree that this patch is probably the more robust and correct way of
> >> doing it. ?It makes me feel better though that we understand the
> >> problem now - otherwise, the presence of a lot of actions on a small
> >> system made me think we had another problem.
> >>
> >> Should we use the same strategy that we (attempted) to do before and
> >> only not include the action if there are no other flows in the dump?
> >> That way we'll only hit this issue in extreme circumstances.
> >
> > Here's a version that works that way. ?I haven't tested it yet.
> 
> Thanks, this version feels a little less random to me in its behavior.
>  I suppose that if you're really concerned about exercising it we
> could write a test but I'm not sure that it is worth it.
> 
> Acked-by: Jesse Gross <jesse at nicira.com>

Thanks.  I'm testing it now and I'll push it soon.




More information about the dev mailing list