Hi, thanks for the patch!
Generally we send patches to d...@openvswitch.org, it's easier for reviewers
to read them,
would you mind sending next version there?
Couple of comments inline, otherwise this looks good to me
2016-05-16 22:28 GMT-07:00 AZ <ak47izat...@gmail.com>:
> When we're adding the port into ovs bridge, it's mtu is updated to the
> minimal
>
s/it's/its/
> mtu of the included port. But when the port is getting removed, no such
> update
> is performed, which leads to bug. For example, when the port with minimal
> mtu
> is removed, bridge's mtu must adapt to new value, but it won't happen.
> How to reproduce the problem:
>
> # ovs-vsctl add-br testing
> # ip link add name gretap11 type gretap local 10.0.0.1 remote 10.0.0.100
> # ip link add name gretap12 type gretap local 10.0.0.1 remote 10.0.0.200
> # ip link set dev gretap12 mtu 1600
> # ovs-vsctl add-port testing gretap11
> # ovs-vsctl add-port testing gretap12
> # ip a sh testing
>
my git sees this as a comment as discards it from the commit message.
Can you use '$', perhaps?
> 16: testing: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN
> group default qlen 1
> link/ether 7a:42:95:00:96:40 brd ff:ff:ff:ff:ff:ff
>
> # ovs-vsctl del-port gretap11
> # ip a sh testing
> 16: testing: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN
> group default qlen 1
> link/ether 7a:42:95:00:96:40 brd ff:ff:ff:ff:ff:ff
>
> ### as we can see here, 'testing' bridge mtu is stuck, while it must
> adapt to new '1600' value,
> ### cause there is only one port 'gretap12' left, and it's mtu is '1600':
>
> # ip a sh gretap12
> 19: gretap12@NONE: <BROADCAST,MULTICAST> mtu 1600 qdisc noop master
> ovs-system state DOWN group default qlen 1000
> link/ether b2:c6:1d:9f:be:0d brd ff:ff:ff:ff:ff:ff
>
> My commit fixes this problem - mtu update is performed on port removal as
> well.
>
> Signed-off-by: wisd0me <ak47izat...@gmail.com>
> ---
> ofproto/ofproto.c | 35 ++++++++++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 89e75d5..9e03616 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -219,6 +219,7 @@ static void learned_cookies_flush(struct ofproto
> *, struct ovs_list *dead_cookie
> /* ofport. */
> static void ofport_destroy__(struct ofport *) OVS_EXCLUDED(ofproto_mutex);
> static void ofport_destroy(struct ofport *, bool del);
> +static inline bool ofport_is_internal(const struct ofport *);
>
> static int update_port(struct ofproto *, const char *devname);
> static int init_ports(struct ofproto *);
> @@ -320,6 +321,7 @@ static uint64_t pick_datapath_id(const struct ofproto
> *);
> static uint64_t pick_fallback_dpid(void);
> static void ofproto_destroy__(struct ofproto *);
> static void update_mtu(struct ofproto *, struct ofport *);
> +static void update_mtu_ofproto(struct ofproto *);
> static void meter_delete(struct ofproto *, uint32_t first, uint32_t last);
> static void meter_insert_rule(struct rule *);
>
> @@ -2390,9 +2392,14 @@ error:
> static void
> ofport_remove(struct ofport *ofport)
> {
> + struct ofproto *p = ofport->ofproto;
> + bool is_internal = ofport_is_internal(ofport);
> +
> connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp,
> OFPPR_DELETE);
> ofport_destroy(ofport, true);
> + if (!is_internal)
> + update_mtu_ofproto(p);
>
We always use brackets, even for single line blocks.
> }
>
> /* If 'ofproto' contains an ofport named 'name', removes it from
> 'ofproto' and
> @@ -2673,6 +2680,12 @@ init_ports(struct ofproto *p)
> return 0;
> }
>
> +static inline bool
> +ofport_is_internal(const struct ofport *port)
> +{
> + return strcmp(netdev_get_type(port->netdev), "internal") == 0;
>
I think I prefer !strcmp()
> +}
> +
> /* Find the minimum MTU of all non-datapath devices attached to 'p'.
> * Returns ETH_PAYLOAD_MAX or the minimum of the ports. */
> static int
> @@ -2687,7 +2700,7 @@ find_min_mtu(struct ofproto *p)
>
> /* Skip any internal ports, since that's what we're trying to
> * set. */
> - if (!strcmp(netdev_get_type(netdev), "internal")) {
> + if (ofport_is_internal(ofport)) {
> continue;
> }
>
> @@ -2707,15 +2720,14 @@ find_min_mtu(struct ofproto *p)
> static void
> update_mtu(struct ofproto *p, struct ofport *port)
> {
> - struct ofport *ofport;
> struct netdev *netdev = port->netdev;
> - int dev_mtu, old_min;
> + int dev_mtu;
>
> if (netdev_get_mtu(netdev, &dev_mtu)) {
> port->mtu = 0;
> return;
> }
> - if (!strcmp(netdev_get_type(port->netdev), "internal")) {
> + if (ofport_is_internal(port)) {
> if (dev_mtu > p->min_mtu) {
> if (!netdev_set_mtu(port->netdev, p->min_mtu)) {
> dev_mtu = p->min_mtu;
> @@ -2725,9 +2737,18 @@ update_mtu(struct ofproto *p, struct ofport *port)
> return;
> }
>
> - /* For non-internal port find new min mtu. */
> - old_min = p->min_mtu;
> port->mtu = dev_mtu;
> + /* For non-internal port find new min mtu. */
> + update_mtu_ofproto(p);
> +}
> +
> +static void
> +update_mtu_ofproto(struct ofproto *p)
> +{
> + /* For non-internal port find new min mtu. */
> + struct ofport *ofport;
> + int old_min = p->min_mtu;
> +
> p->min_mtu = find_min_mtu(p);
> if (p->min_mtu == old_min) {
> return;
> @@ -2736,7 +2757,7 @@ update_mtu(struct ofproto *p, struct ofport *port)
> HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
> struct netdev *netdev = ofport->netdev;
>
> - if (!strcmp(netdev_get_type(netdev), "internal")) {
> + if (ofport_is_internal(ofport)) {
> if (!netdev_set_mtu(netdev, p->min_mtu)) {
> ofport->mtu = p->min_mtu;
> }
> --
> 2.8.0
> _______________________________________________
> discuss mailing list
> discuss@openvswitch.org
> http://openvswitch.org/mailman/listinfo/discuss
>
_______________________________________________
discuss mailing list
discuss@openvswitch.org
http://openvswitch.org/mailman/listinfo/discuss
