Message ID | 1470758480-133197-2-git-send-email-mark.b.kavanagh@intel.com |
---|---|
State | Accepted |
Delegated to: | Daniele Di Proietto |
Headers | show |
Hi, Daniele. Maybe I'm not the very right person to review such things, but I'm actually used this patch since it appeared in your github and it looks good to me. Also, I wanted to remove this non-working implementation of 'netdev_dpdk_set_mtu' for a long time. Acked-by: Ilya Maximets <i.maximets@samsung.com> On 09.08.2016 19:01, Mark Kavanagh wrote: > From: Daniele Di Proietto <diproiettod@vmware.com> > > The 'mtu_request' column can be used to set the MTU of a specific > interface. > > This column is useful because it will allow changing the MTU of DPDK > devices (implemented in a future commit), which are not accessible > outside the ovs-vswitchd process, but it can be used for kernel > interfaces as well. > > The current implementation of set_mtu() in netdev-dpdk is removed > because it's broken. It will be reintroduced by a subsequent commit on > this series. > > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > --- > NEWS | 2 ++ > lib/netdev-dpdk.c | 53 +--------------------------------------------- > vswitchd/bridge.c | 9 ++++++++ > vswitchd/vswitch.ovsschema | 10 +++++++-- > vswitchd/vswitch.xml | 52 +++++++++++++++++++++++++++++++++------------ > 5 files changed, 58 insertions(+), 68 deletions(-) > > diff --git a/NEWS b/NEWS > index c2ed71d..ce10982 100644 > --- a/NEWS > +++ b/NEWS > @@ -101,6 +101,8 @@ Post-v2.5.0 > - ovs-pki: Changed message digest algorithm from SHA-1 to SHA-512 because > SHA-1 is no longer secure and some operating systems have started to > disable it in OpenSSL. > + - Add 'mtu_request' column to the Interface table. It can be used to > + configure the MTU of non-internal ports. > > > v2.5.0 - 26 Feb 2016 > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index f37ec1c..60db568 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1639,57 +1639,6 @@ netdev_dpdk_get_mtu(const struct netdev *netdev, int *mtup) > } > > static int > -netdev_dpdk_set_mtu(const struct netdev *netdev, int mtu) > -{ > - struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > - int old_mtu, err, dpdk_mtu; > - struct dpdk_mp *old_mp; > - struct dpdk_mp *mp; > - uint32_t buf_size; > - > - ovs_mutex_lock(&dpdk_mutex); > - ovs_mutex_lock(&dev->mutex); > - if (dev->mtu == mtu) { > - err = 0; > - goto out; > - } > - > - buf_size = dpdk_buf_size(mtu); > - dpdk_mtu = FRAME_LEN_TO_MTU(buf_size); > - > - mp = dpdk_mp_get(dev->socket_id, dpdk_mtu); > - if (!mp) { > - err = ENOMEM; > - goto out; > - } > - > - rte_eth_dev_stop(dev->port_id); > - > - old_mtu = dev->mtu; > - old_mp = dev->dpdk_mp; > - dev->dpdk_mp = mp; > - dev->mtu = mtu; > - dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); > - > - err = dpdk_eth_dev_init(dev); > - if (err) { > - dpdk_mp_put(mp); > - dev->mtu = old_mtu; > - dev->dpdk_mp = old_mp; > - dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); > - dpdk_eth_dev_init(dev); > - goto out; > - } > - > - dpdk_mp_put(old_mp); > - netdev_change_seq_changed(netdev); > -out: > - ovs_mutex_unlock(&dev->mutex); > - ovs_mutex_unlock(&dpdk_mutex); > - return err; > -} > - > -static int > netdev_dpdk_get_carrier(const struct netdev *netdev, bool *carrier); > > static int > @@ -2964,7 +2913,7 @@ netdev_dpdk_vhost_cuse_reconfigure(struct netdev *netdev) > netdev_dpdk_set_etheraddr, \ > netdev_dpdk_get_etheraddr, \ > netdev_dpdk_get_mtu, \ > - netdev_dpdk_set_mtu, \ > + NULL, /* set_mtu */ \ > netdev_dpdk_get_ifindex, \ > GET_CARRIER, \ > netdev_dpdk_get_carrier_resets, \ > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index ddf1fe5..397be70 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -775,6 +775,15 @@ bridge_delete_or_reconfigure_ports(struct bridge *br) > goto delete; > } > > + if (iface->cfg->n_mtu_request == 1 > + && strcmp(iface->type, > + ofproto_port_open_type(br->type, "internal"))) { > + /* Try to set the MTU to the requested value. This is not done > + * for internal interfaces, since their MTU is decided by the > + * ofproto module, based on other ports in the bridge. */ > + netdev_set_mtu(iface->netdev, *iface->cfg->mtu_request); > + } > + > /* If the requested OpenFlow port for 'iface' changed, and it's not > * already the correct port, then we might want to temporarily delete > * this interface, so we can add it back again with the new OpenFlow > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema > index 32fdf28..8966803 100644 > --- a/vswitchd/vswitch.ovsschema > +++ b/vswitchd/vswitch.ovsschema > @@ -1,6 +1,6 @@ > {"name": "Open_vSwitch", > - "version": "7.13.0", > - "cksum": "889248633 22774", > + "version": "7.14.0", > + "cksum": "3974332717 22936", > "tables": { > "Open_vSwitch": { > "columns": { > @@ -321,6 +321,12 @@ > "mtu": { > "type": {"key": "integer", "min": 0, "max": 1}, > "ephemeral": true}, > + "mtu_request": { > + "type": { > + "key": {"type": "integer", > + "minInteger": 1}, > + "min": 0, > + "max": 1}}, > "error": { > "type": {"key": "string", "min": 0, "max": 1}}}, > "indexes": [["name"]]}, > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 65acdc7..780bd2d 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -2380,6 +2380,44 @@ > </column> > </group> > > + <group title="MTU"> > + <p> > + The MTU (maximum transmission unit) is the largest amount of data > + that can fit into a single Ethernet frame. The standard Ethernet > + MTU is 1500 bytes. Some physical media and many kinds of virtual > + interfaces can be configured with higher MTUs. > + </p> > + > + <p> > + A client may change a non-internal interface MTU by filling in > + <ref column="mtu_request"/>. Internal interfaces MTU, instead, is set > + by Open vSwitch to the minimum of non-internal interfaces MTU in the > + bridge. In any case, Open vSwitch then reports in <ref column="mtu"/> > + the currently configured value. > + </p> > + > + <column name="mtu"> > + <p> > + This column will be empty for an interface that does not > + have an MTU as, for example, some kinds of tunnels do not. > + </p> > + > + <p> > + Open vSwitch sets this column's value, so other clients should treat > + it as read-only. > + </p> > + </column> > + > + <column name="mtu_request" > + type='{"type": "integer", "minInteger": 1}'> > + <p> > + Requested MTU (Maximum Transmission Unit) for the interface. A client > + can fill this column to change the MTU of a non-internal interface. > + </p> > + </column> > + > + </group> > + > <group title="Interface Status"> > <p> > Status information about interfaces attached to bridges, updated every > @@ -2422,20 +2460,6 @@ > </p> > </column> > > - <column name="mtu"> > - <p> > - The MTU (maximum transmission unit); i.e. the largest > - amount of data that can fit into a single Ethernet frame. > - The standard Ethernet MTU is 1500 bytes. Some physical media > - and many kinds of virtual interfaces can be configured with > - higher MTUs. > - </p> > - <p> > - This column will be empty for an interface that does not > - have an MTU as, for example, some kinds of tunnels do not. > - </p> > - </column> > - > <column name="lacp_current"> > Boolean value indicating LACP status for this interface. If true, this > interface has current LACP information about its LACP partner. This >
On 09/08/2016 23:55, "Ilya Maximets" <i.maximets@samsung.com> wrote: >Hi, Daniele. >Maybe I'm not the very right person to review such things, but I'm >actually used this patch since it appeared in your github and it >looks good to me. Also, I wanted to remove this non-working >implementation of 'netdev_dpdk_set_mtu' for a long time. > >Acked-by: Ilya Maximets <i.maximets@samsung.com> I just wanted to give it a couple of more days to see if there were any objections for the database interface. Thanks for the review, I pushed this to master > >On 09.08.2016 19:01, Mark Kavanagh wrote: >> From: Daniele Di Proietto <diproiettod@vmware.com> >> >> The 'mtu_request' column can be used to set the MTU of a specific >> interface. >> >> This column is useful because it will allow changing the MTU of DPDK >> devices (implemented in a future commit), which are not accessible >> outside the ovs-vswitchd process, but it can be used for kernel >> interfaces as well. >> >> The current implementation of set_mtu() in netdev-dpdk is removed >> because it's broken. It will be reintroduced by a subsequent commit on >> this series. >> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >> --- >> NEWS | 2 ++ >> lib/netdev-dpdk.c | 53 +--------------------------------------------- >> vswitchd/bridge.c | 9 ++++++++ >> vswitchd/vswitch.ovsschema | 10 +++++++-- >> vswitchd/vswitch.xml | 52 +++++++++++++++++++++++++++++++++------------ >> 5 files changed, 58 insertions(+), 68 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index c2ed71d..ce10982 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -101,6 +101,8 @@ Post-v2.5.0 >> - ovs-pki: Changed message digest algorithm from SHA-1 to SHA-512 because >> SHA-1 is no longer secure and some operating systems have started to >> disable it in OpenSSL. >> + - Add 'mtu_request' column to the Interface table. It can be used to >> + configure the MTU of non-internal ports. >> >> >> v2.5.0 - 26 Feb 2016 >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index f37ec1c..60db568 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1639,57 +1639,6 @@ netdev_dpdk_get_mtu(const struct netdev *netdev, int *mtup) >> } >> >> static int >> -netdev_dpdk_set_mtu(const struct netdev *netdev, int mtu) >> -{ >> - struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> - int old_mtu, err, dpdk_mtu; >> - struct dpdk_mp *old_mp; >> - struct dpdk_mp *mp; >> - uint32_t buf_size; >> - >> - ovs_mutex_lock(&dpdk_mutex); >> - ovs_mutex_lock(&dev->mutex); >> - if (dev->mtu == mtu) { >> - err = 0; >> - goto out; >> - } >> - >> - buf_size = dpdk_buf_size(mtu); >> - dpdk_mtu = FRAME_LEN_TO_MTU(buf_size); >> - >> - mp = dpdk_mp_get(dev->socket_id, dpdk_mtu); >> - if (!mp) { >> - err = ENOMEM; >> - goto out; >> - } >> - >> - rte_eth_dev_stop(dev->port_id); >> - >> - old_mtu = dev->mtu; >> - old_mp = dev->dpdk_mp; >> - dev->dpdk_mp = mp; >> - dev->mtu = mtu; >> - dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); >> - >> - err = dpdk_eth_dev_init(dev); >> - if (err) { >> - dpdk_mp_put(mp); >> - dev->mtu = old_mtu; >> - dev->dpdk_mp = old_mp; >> - dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); >> - dpdk_eth_dev_init(dev); >> - goto out; >> - } >> - >> - dpdk_mp_put(old_mp); >> - netdev_change_seq_changed(netdev); >> -out: >> - ovs_mutex_unlock(&dev->mutex); >> - ovs_mutex_unlock(&dpdk_mutex); >> - return err; >> -} >> - >> -static int >> netdev_dpdk_get_carrier(const struct netdev *netdev, bool *carrier); >> >> static int >> @@ -2964,7 +2913,7 @@ netdev_dpdk_vhost_cuse_reconfigure(struct netdev *netdev) >> netdev_dpdk_set_etheraddr, \ >> netdev_dpdk_get_etheraddr, \ >> netdev_dpdk_get_mtu, \ >> - netdev_dpdk_set_mtu, \ >> + NULL, /* set_mtu */ \ >> netdev_dpdk_get_ifindex, \ >> GET_CARRIER, \ >> netdev_dpdk_get_carrier_resets, \ >> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >> index ddf1fe5..397be70 100644 >> --- a/vswitchd/bridge.c >> +++ b/vswitchd/bridge.c >> @@ -775,6 +775,15 @@ bridge_delete_or_reconfigure_ports(struct bridge *br) >> goto delete; >> } >> >> + if (iface->cfg->n_mtu_request == 1 >> + && strcmp(iface->type, >> + ofproto_port_open_type(br->type, "internal"))) { >> + /* Try to set the MTU to the requested value. This is not done >> + * for internal interfaces, since their MTU is decided by the >> + * ofproto module, based on other ports in the bridge. */ >> + netdev_set_mtu(iface->netdev, *iface->cfg->mtu_request); >> + } >> + >> /* If the requested OpenFlow port for 'iface' changed, and it's not >> * already the correct port, then we might want to temporarily delete >> * this interface, so we can add it back again with the new OpenFlow >> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema >> index 32fdf28..8966803 100644 >> --- a/vswitchd/vswitch.ovsschema >> +++ b/vswitchd/vswitch.ovsschema >> @@ -1,6 +1,6 @@ >> {"name": "Open_vSwitch", >> - "version": "7.13.0", >> - "cksum": "889248633 22774", >> + "version": "7.14.0", >> + "cksum": "3974332717 22936", >> "tables": { >> "Open_vSwitch": { >> "columns": { >> @@ -321,6 +321,12 @@ >> "mtu": { >> "type": {"key": "integer", "min": 0, "max": 1}, >> "ephemeral": true}, >> + "mtu_request": { >> + "type": { >> + "key": {"type": "integer", >> + "minInteger": 1}, >> + "min": 0, >> + "max": 1}}, >> "error": { >> "type": {"key": "string", "min": 0, "max": 1}}}, >> "indexes": [["name"]]}, >> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >> index 65acdc7..780bd2d 100644 >> --- a/vswitchd/vswitch.xml >> +++ b/vswitchd/vswitch.xml >> @@ -2380,6 +2380,44 @@ >> </column> >> </group> >> >> + <group title="MTU"> >> + <p> >> + The MTU (maximum transmission unit) is the largest amount of data >> + that can fit into a single Ethernet frame. The standard Ethernet >> + MTU is 1500 bytes. Some physical media and many kinds of virtual >> + interfaces can be configured with higher MTUs. >> + </p> >> + >> + <p> >> + A client may change a non-internal interface MTU by filling in >> + <ref column="mtu_request"/>. Internal interfaces MTU, instead, is set >> + by Open vSwitch to the minimum of non-internal interfaces MTU in the >> + bridge. In any case, Open vSwitch then reports in <ref column="mtu"/> >> + the currently configured value. >> + </p> >> + >> + <column name="mtu"> >> + <p> >> + This column will be empty for an interface that does not >> + have an MTU as, for example, some kinds of tunnels do not. >> + </p> >> + >> + <p> >> + Open vSwitch sets this column's value, so other clients should treat >> + it as read-only. >> + </p> >> + </column> >> + >> + <column name="mtu_request" >> + type='{"type": "integer", "minInteger": 1}'> >> + <p> >> + Requested MTU (Maximum Transmission Unit) for the interface. A client >> + can fill this column to change the MTU of a non-internal interface. >> + </p> >> + </column> >> + >> + </group> >> + >> <group title="Interface Status"> >> <p> >> Status information about interfaces attached to bridges, updated every >> @@ -2422,20 +2460,6 @@ >> </p> >> </column> >> >> - <column name="mtu"> >> - <p> >> - The MTU (maximum transmission unit); i.e. the largest >> - amount of data that can fit into a single Ethernet frame. >> - The standard Ethernet MTU is 1500 bytes. Some physical media >> - and many kinds of virtual interfaces can be configured with >> - higher MTUs. >> - </p> >> - <p> >> - This column will be empty for an interface that does not >> - have an MTU as, for example, some kinds of tunnels do not. >> - </p> >> - </column> >> - >> <column name="lacp_current"> >> Boolean value indicating LACP status for this interface. If true, this >> interface has current LACP information about its LACP partner. This >>
diff --git a/NEWS b/NEWS index c2ed71d..ce10982 100644 --- a/NEWS +++ b/NEWS @@ -101,6 +101,8 @@ Post-v2.5.0 - ovs-pki: Changed message digest algorithm from SHA-1 to SHA-512 because SHA-1 is no longer secure and some operating systems have started to disable it in OpenSSL. + - Add 'mtu_request' column to the Interface table. It can be used to + configure the MTU of non-internal ports. v2.5.0 - 26 Feb 2016 diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index f37ec1c..60db568 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1639,57 +1639,6 @@ netdev_dpdk_get_mtu(const struct netdev *netdev, int *mtup) } static int -netdev_dpdk_set_mtu(const struct netdev *netdev, int mtu) -{ - struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); - int old_mtu, err, dpdk_mtu; - struct dpdk_mp *old_mp; - struct dpdk_mp *mp; - uint32_t buf_size; - - ovs_mutex_lock(&dpdk_mutex); - ovs_mutex_lock(&dev->mutex); - if (dev->mtu == mtu) { - err = 0; - goto out; - } - - buf_size = dpdk_buf_size(mtu); - dpdk_mtu = FRAME_LEN_TO_MTU(buf_size); - - mp = dpdk_mp_get(dev->socket_id, dpdk_mtu); - if (!mp) { - err = ENOMEM; - goto out; - } - - rte_eth_dev_stop(dev->port_id); - - old_mtu = dev->mtu; - old_mp = dev->dpdk_mp; - dev->dpdk_mp = mp; - dev->mtu = mtu; - dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); - - err = dpdk_eth_dev_init(dev); - if (err) { - dpdk_mp_put(mp); - dev->mtu = old_mtu; - dev->dpdk_mp = old_mp; - dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); - dpdk_eth_dev_init(dev); - goto out; - } - - dpdk_mp_put(old_mp); - netdev_change_seq_changed(netdev); -out: - ovs_mutex_unlock(&dev->mutex); - ovs_mutex_unlock(&dpdk_mutex); - return err; -} - -static int netdev_dpdk_get_carrier(const struct netdev *netdev, bool *carrier); static int @@ -2964,7 +2913,7 @@ netdev_dpdk_vhost_cuse_reconfigure(struct netdev *netdev) netdev_dpdk_set_etheraddr, \ netdev_dpdk_get_etheraddr, \ netdev_dpdk_get_mtu, \ - netdev_dpdk_set_mtu, \ + NULL, /* set_mtu */ \ netdev_dpdk_get_ifindex, \ GET_CARRIER, \ netdev_dpdk_get_carrier_resets, \ diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index ddf1fe5..397be70 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -775,6 +775,15 @@ bridge_delete_or_reconfigure_ports(struct bridge *br) goto delete; } + if (iface->cfg->n_mtu_request == 1 + && strcmp(iface->type, + ofproto_port_open_type(br->type, "internal"))) { + /* Try to set the MTU to the requested value. This is not done + * for internal interfaces, since their MTU is decided by the + * ofproto module, based on other ports in the bridge. */ + netdev_set_mtu(iface->netdev, *iface->cfg->mtu_request); + } + /* If the requested OpenFlow port for 'iface' changed, and it's not * already the correct port, then we might want to temporarily delete * this interface, so we can add it back again with the new OpenFlow diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema index 32fdf28..8966803 100644 --- a/vswitchd/vswitch.ovsschema +++ b/vswitchd/vswitch.ovsschema @@ -1,6 +1,6 @@ {"name": "Open_vSwitch", - "version": "7.13.0", - "cksum": "889248633 22774", + "version": "7.14.0", + "cksum": "3974332717 22936", "tables": { "Open_vSwitch": { "columns": { @@ -321,6 +321,12 @@ "mtu": { "type": {"key": "integer", "min": 0, "max": 1}, "ephemeral": true}, + "mtu_request": { + "type": { + "key": {"type": "integer", + "minInteger": 1}, + "min": 0, + "max": 1}}, "error": { "type": {"key": "string", "min": 0, "max": 1}}}, "indexes": [["name"]]}, diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 65acdc7..780bd2d 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -2380,6 +2380,44 @@ </column> </group> + <group title="MTU"> + <p> + The MTU (maximum transmission unit) is the largest amount of data + that can fit into a single Ethernet frame. The standard Ethernet + MTU is 1500 bytes. Some physical media and many kinds of virtual + interfaces can be configured with higher MTUs. + </p> + + <p> + A client may change a non-internal interface MTU by filling in + <ref column="mtu_request"/>. Internal interfaces MTU, instead, is set + by Open vSwitch to the minimum of non-internal interfaces MTU in the + bridge. In any case, Open vSwitch then reports in <ref column="mtu"/> + the currently configured value. + </p> + + <column name="mtu"> + <p> + This column will be empty for an interface that does not + have an MTU as, for example, some kinds of tunnels do not. + </p> + + <p> + Open vSwitch sets this column's value, so other clients should treat + it as read-only. + </p> + </column> + + <column name="mtu_request" + type='{"type": "integer", "minInteger": 1}'> + <p> + Requested MTU (Maximum Transmission Unit) for the interface. A client + can fill this column to change the MTU of a non-internal interface. + </p> + </column> + + </group> + <group title="Interface Status"> <p> Status information about interfaces attached to bridges, updated every @@ -2422,20 +2460,6 @@ </p> </column> - <column name="mtu"> - <p> - The MTU (maximum transmission unit); i.e. the largest - amount of data that can fit into a single Ethernet frame. - The standard Ethernet MTU is 1500 bytes. Some physical media - and many kinds of virtual interfaces can be configured with - higher MTUs. - </p> - <p> - This column will be empty for an interface that does not - have an MTU as, for example, some kinds of tunnels do not. - </p> - </column> - <column name="lacp_current"> Boolean value indicating LACP status for this interface. If true, this interface has current LACP information about its LACP partner. This