[ovs-dev] [PATCH] don't bother to ask dpif class delete ODPP_NONE.

[Ben Pfaff]

On Fri, Aug 30, 2013 at 11:42:02AM +0900, YAMAMOTO Takashi wrote:
> > On Wed, Aug 28, 2013 at 02:58:02PM +0900, YAMAMOTO Takashi wrote:
> >> > On Wed, Aug 28, 2013 at 10:38:06AM +0900, YAMAMOTO Takashi wrote:
> >> >> > On Tue, Aug 27, 2013 at 04:10:03PM +0900, YAMAMOTO Takashi wrote:
> >> >> >> this fixes ofp_port_status delivery on a patch port removal.
> >> >> > 
> >> >> > Can you explain further?  I don't see any problems with patch port
> >> >> > removal.  Example:
> >> >> > 
> >> >> > blp at sigsegv:~/ovs/_clang$ make sandbox
> >> >> > ...
> >> >> > ----------------------------------------------------------------------
> >> >> > You are running in a dummy Open vSwitch environment.  You can use
> >> >> > ovs-vsctl, ovs-ofctl, ovs-appctl, and other tools to work with the
> >> >> > dummy switch.  
> >> >> > 
> >> >> > Log files, pidfiles, and the configuration database are in the
> >> >> > "sandbox" subdirectory.
> >> >> > 
> >> >> > Exit the shell to kill the running daemons.
> >> >> > blp at sigsegv:~/ovs/tutorial$ ovs-vsctl add-br br0
> >> >> 
> >> >> i'm using datapath_type=netdev.
> >> >> 
> >> >> kuma% ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev
> >> >> kuma% ovs-ofctl monitor br0 128&
> >> >> [1] 17027
> >> >> kuma% ovs-vsctl add-port br0 patch1 -- set interface patch1 type=patch options:peer=patch1 -- add-port br0 patch2 -- set interface patch2 type=patch options:peer=patch2
> >> >> OFPT_PORT_STATUS (xid=0x0): ADD: 1(patch1): addr:aa:55:aa:55:00:02
> >> >>      config:     PORT_DOWN
> >> >>      state:      LINK_DOWN
> >> >>      speed: 0 Mbps now, 0 Mbps max
> >> >> OFPT_PORT_STATUS (xid=0x0): ADD: 2(patch2): addr:aa:55:aa:55:00:03
> >> >>      config:     PORT_DOWN
> >> >>      state:      LINK_DOWN
> >> >>      speed: 0 Mbps now, 0 Mbps max
> >> >> kuma% 
> >> > 
> >> > I don't understand yet.  If it is important to show me the output here,
> >> > then I guess your patch must change the output.  What change does your
> >> > patch make in the output?
> >> 
> >> sorry for confusing.
> >> 
> >> it seems that the problem doesn't happen on "make sandbox" environment.
> >> (i don't know why.  i'm not aware of how sandbox works)
> >> 
> >> i'm seeing the following message on "ovs-vsctl del-port patch1"
> >> and my controller doesn't receive ofp_port_status.
> >> 
> >> 2013-08-04T21:42:03Z|00016|dpif|WARN|netdev at ovs-netdev: port_del failed (Invalid argument)
> > 
> > I applied your other patch that stops overriding patch ports in the
> > sandbox.  That made the output of the above commands change (the port
> > updates disappeared), as shown below, but this patch didn't fix that.
> > 
> > ----------------------------------------------------------------------
> > You are running in a dummy Open vSwitch environment.  You can use
> > ovs-vsctl, ovs-ofctl, ovs-appctl, and other tools to work with the
> > dummy switch.  
> > 
> > Log files, pidfiles, and the configuration database are in the
> > "sandbox" subdirectory.
> > 
> > Exit the shell to kill the running daemons.
> > blp at blp:~/nicira/ovs/tutorial(0)$ ovs-vsctl add-br br0blp at blp:~/nicira/ovs/tutorial(0)$ ovs-ofctl monitor br0 128&
> > [1] 24807
> > blp at blp:~/nicira/ovs/tutorial(0)$ ovs-vsctl add-port br0 patch1 -- set interface patch1 type=patch options:peer=patch1 -- add-port br0 patch2 -- set interface patch2 type=patch options:peer=patch2
> 
> this command seems wrong.
> patch1's peer should be patch2, not patch1.  ditto for patch2.
> as it fails to add ports, you didn't even get PORT_STATUS for ADD.

It does help to get the syntax right.  Thank you for pointing out my
mistake.

> the following is today's master with my patch applied.
> 
> kuma% gmake sandbox
> <..snip..>
> kuma% ovs-vsctl add-br br0 -- set bridge br0 datapath_type=netdev
> kuma% ovs-ofctl monitor br0 128 &
> [1] 49
> kuma% ovs-vsctl add-port br0 patch1 -- set interface patch1 type=patch options:peer=patch2 -- add-port br0 patch2 -- set interface patch2 type=patch options:peer=patch1
> OFPT_PORT_STATUS (xid=0x0): ADD: 1(patch1): addr:ae:fd:f4:fe:8c:ec
>      config:     0
>      state:      0
>      speed: 0 Mbps now, 0 Mbps max
> OFPT_PORT_STATUS (xid=0x0): ADD: 2(patch2): addr:da:1c:2a:58:42:71
>      config:     0
>      state:      0
>      speed: 0 Mbps now, 0 Mbps max
> kuma% ovs-vsctl del-port patch1
> OFPT_PORT_STATUS (xid=0x0): DEL: 1(patch1): addr:ae:fd:f4:fe:8c:ec
>      config:     0
>      state:      0
>      speed: 0 Mbps now, 0 Mbps max
> kuma% ovs-vsctl del-port patch2
> OFPT_PORT_STATUS (xid=0x0): DEL: 2(patch2): addr:da:1c:2a:58:42:71
>      config:     0
>      state:      0
>      speed: 0 Mbps now, 0 Mbps max
> kuma% 
> 
> > blp at blp:~/nicira/ovs/tutorial(0)$ ovs-vsctl del-port patch1
> > blp at blp:~/nicira/ovs/tutorial(0)$ ovs-vsctl del-port patch2
> > blp at blp:~/nicira/ovs/tutorial(0)$ ovs-vsctl del-br br0
> > ovs-ofctl: vconn_recv (End of file)
> > [1]+  Exit 1                  ovs-ofctl monitor br0 128
> > blp at blp:~/nicira/ovs/tutorial(0)$ gitk&
> > [1] 24815
> > blp at blp:~/nicira/ovs/tutorial(0)$ exit
> > ----------------------------------------------------------------------

Thanks.

> > I suspect that the real problem here is that port_add() doesn't call
> > dpif_port_add() for patch ports so port_del() should not try to call
> > dpif_port_del() for them either, something like this.  Will you try the
> > following patch as an alternative?  (However, even with this patch I
> > don't get OpenFlow notifications for patch ports in the sandbox, so
> > something else is going on.)
> 
> it would fix the problem, yes.

OK, I see that too.

I will post my patch for official review.  Will you please review it?

> i don't know if the following bundle_remove call can be necessary
> for patch ports or not, though.

I don't think it's necessary at all anymore.  I will write up a separate
patch.