[ovs-dev] [PATCH] dpif-linux: Don't reset kernel upcall_pids unintentionally.

[Jesse Gross]

On Fri, Oct 7, 2011 at 7:54 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Fri, Oct 07, 2011 at 06:25:57PM -0700, Jesse Gross wrote:
>> On Fri, Oct 7, 2011 at 5:57 PM, Ben Pfaff <blp at nicira.com> wrote:
>> > On Fri, Oct 07, 2011 at 05:05:15PM -0700, Jesse Gross wrote:
>> >> On Fri, Oct 7, 2011 at 4:42 PM, Ben Pfaff <blp at nicira.com> wrote:
>> >> > Commit b063d9f0 "datapath: Use unicast Netlink sockets for upcalls" that
>> >> > introduced an 'upcall_pid' member into struct dpif_linux_vport, struct
>> >> > dpif_linux_dp, and struct dpif_linux_flow neglected to do so only if the
>> >> > member was nonzero. ??This caused every datapath, vport, and flow operation
>> >> > to supply an upcall_pid. ??In particular, the netdev_set_config() called at
>> >> > startup when a vport already existed caused the upcall_pid for that vport
>> >> > to be reset to 0, which in turn caused all packets received on the vport to
>> >> > be dropped instead of forwarded to ovs-vswitchd.
>> >> >
>> >> > Reported-by: Shih-Hao Li <shli at nicira.com>
>> >>
>> >> I think we actually want to distinguish between unset and zero. ??When
>> >> the listen_mask indicates that a packet type shouldn't be received
>> >> then we intentionally generate an upcall_pid of 0 to shut off those
>> >> types of upcalls. ??Most of dpif-linux.c deals with this by simply
>> >> always including the appropriate upcall_pid but that was missed for
>> >> the calls in netdev-vport. ??At this point, nothing ever turns off
>> >> parts of listen_mask, so it doesn't really matter but that was the
>> >> intention.
>> >
>> > I actually understood these two cases as I wrote up the commit, but I
>> > didn't see anything that currently needed to take advantage of it so I
>> > ignored it.
>> >
>> > I can fix it up to separate "no change" and "set to zero", though, if
>> > you prefer.
>>
>> I guess it seems better to separate them out, otherwise the code is
>> confusing because it's doing something in one place but ignoring it in
>> another.
>
> OK, here's v2.  Unlike the previous version, this one is compile-tested
> only because I'm away from my desk.

Looks good, thanks.