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

Jesse Gross jesse at nicira.com
Tue Feb 1 04:50:05 UTC 2011


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.




More information about the dev mailing list