Hi Alex,
On Tue, 2014-02-18 at 10:26 -0800, Alex Wang wrote:
> Thanks a lot Han for the close review ;D
My pleasure :)
>
> In this function, you separated vport transaction for creating
> vports
> and setting channel pids to kernel. But it could lead to
> problem when
> kernel started sending upcalls to user space when vports are
> added but
> channel pids not updated. I think it should still be kept as a
> single
> transaction to avoid race condition.
>
>
>
>
>
> I think it is okay that we miss the upcalls for a short period.
>
>
> The way current ovs calls 'port_add()' is that ODPP_NONE is given for
> '*port_nop'.
> And the datapath will densely allocate the port number and notify it
> back to
> userspace. So, there is no way to specify channel in the same
> transaction in this
> case. Also, I don't want to separate socket creation from
> add_vport_channel().
>
>
OK, I see. But then the kernel code need to changed, because:
ovs_dp_process_received_packet() calls ovs_vport_find_pid(). When
upcall_pids are not set to datapath, ovs_vport_find_pid() returns 0 as a
nl pid:
if (!pids)
return 0;
and then it invokes ovs_dp_upcall() still. I think we need error
handling here to drop packets in such race condition.
>
> The logic of calling dpif_recv_set() here is somehow
> misleading. I
> understand that it is only for refreshing dpif->n_handlers,
> rather than
> enable/disable receiving. But would it be better to introduce
> a new
> interface in dpif to clearly update dpif->n_handler, which can
> be
> invoked in udpif_set_threads().
>
>
>
>
>
> I'm okay with either. will ask around then update with you.
>
May I add that there is one more benefit if invoking a new interface in
udpif_set_threads(): we can handle the update more gracefully in
udpif_set_threads() to minimize traffic interruption:
1. destroy old threads
2. update datapath pids via the new interface
3. create new threads
Best regards,
Han
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
