[ovs-dev] [bugfixes 3/3] datapath: Dump flow actions only if there is room.
jesse at nicira.com
Tue Feb 1 17:07:42 UTC 2011
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>
More information about the dev