[ovs-dev] [PATCH V2] ofproto-dpif-upcall: Remove the dispatcher thread.

[Zhou, Han]

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