Hey Han, Again, thanks for your comments, ;D
On Tue, Feb 18, 2014 at 10:46 PM, Zhou, Han <hzh...@ebay.com> wrote: > > 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. > > Inside the current "ovs_dp_upcall()" we have the following code: 277<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l277>int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb, 278<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l278> const struct dp_upcall_info *upcall_info) 279<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l279>{ 280<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l280> struct dp_stats_percpu *stats; 281<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l281> int err; 282<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l282> 283<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l283> if (upcall_info->portid == 0) { 284<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l284> err = -ENOTCONN; 285<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l285> goto err; 286<http://git.openvswitch.org/cgi-bin/gitweb.cgi?p=openvswitch;a=blob;f=datapath/datapath.c;h=a7839a128cae7a6d481fc2b426282b85d4d70ff2;hb=09350a3de320e83517c943858acc397db784d583#l286> } So, we have done that already. > > > > 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 > I'll explain this in discussion, thanks,
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
