| Message ID | 20230502222024.9233-1-cfontain@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Add config option for vhost tx_steering default mode | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Christophe Fontaine, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line has non-spaces leading whitespace
#104 FILE: lib/dpif-netdev.c:5160:
txq_mode = TXQ_REQ_MODE_THREAD;
WARNING: Line has non-spaces leading whitespace
#110 FILE: lib/dpif-netdev.c:5166:
txq_mode = TXQ_REQ_MODE_THREAD;
Lines checked: 140, Warnings: 2, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 5/3/23 00:20, Christophe Fontaine wrote: > Current tx_steering mode has to be configured on each Interface. > The global config option "vhost-tx-steering-default" allows > the user to select a default mode (thread or hash) for > all dpdkvhost ports. > > Signed-off-by: Christophe Fontaine <cfontain@redhat.com> Hi, Christophe. Thanks for the patch! The idea is interesting. However, despite the name, the option in the current implementation will affect every port including physical ones. And I don't think that is intended behavior. Also, we should not have options specific to a particular port type in the generic datapath code. We have one ugly 'is_vhost' check, but it shouldn't really exist. One solution might be to have a generic default option like 'tx-steering-default', but it should be clearly documented that it affects all ports. Then users can set the 'thread' mode for ports they want to not use 'hash'. Best regards, Ilya Maximets.
On Fri, May 5, 2023 at 9:41 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 5/3/23 00:20, Christophe Fontaine wrote: > > Current tx_steering mode has to be configured on each Interface. > > The global config option "vhost-tx-steering-default" allows > > the user to select a default mode (thread or hash) for > > all dpdkvhost ports. > > > > Signed-off-by: Christophe Fontaine <cfontain@redhat.com> > > Hi, Christophe. Thanks for the patch! > > The idea is interesting. However, despite the name, the option > in the current implementation will affect every port including > physical ones. And I don't think that is intended behavior. > > Also, we should not have options specific to a particular port > type in the generic datapath code. We have one ugly 'is_vhost' > check, but it shouldn't really exist. > > One solution might be to have a generic default option like > 'tx-steering-default', but it should be clearly documented that > it affects all ports. Then users can set the 'thread' mode for > ports they want to not use 'hash'. Thanks for the review Ilya, I get your point that a global conf shouldn't affect only a specific type of ports. I'll abandon this patch, and submit another one, which will affect all ports. Christophe > > Best regards, Ilya Maximets. >
diff --git a/NEWS b/NEWS index cfd466663..c1d64cb29 100644 --- a/NEWS +++ b/NEWS @@ -34,6 +34,7 @@ Post-v3.1.0 * ovs-vswitchd will keep the CAP_SYS_RAWIO capability when started with the --hw-rawio-access command line option. This allows the process extra privileges when mapping physical interconnect memory. + * Added new global configuration option "vhost-tx-steering-default". - SRv6 Tunnel Protocol * Added support for userspace datapath (only). diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 70b953ae6..38c60de1c 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -246,6 +246,17 @@ enum sched_assignment_type { SCHED_GROUP }; +enum txq_req_mode { + TXQ_REQ_MODE_THREAD, + TXQ_REQ_MODE_HASH, +}; + +enum txq_mode { + TXQ_MODE_STATIC, + TXQ_MODE_XPS, + TXQ_MODE_XPS_HASH, +}; + /* Datapath based on the network device interface from netdev.h. * * @@ -334,6 +345,8 @@ struct dp_netdev { /* Bonds. */ struct ovs_mutex bond_mutex; /* Protects updates of 'tx_bonds'. */ struct cmap tx_bonds; /* Contains 'struct tx_bond'. */ + + enum txq_req_mode vhost_txq_requested_mode_default; }; static struct dp_netdev_port *dp_netdev_lookup_port(const struct dp_netdev *dp, @@ -451,17 +464,6 @@ struct dp_netdev_rxq { atomic_ullong cycles_intrvl[PMD_INTERVAL_MAX]; }; -enum txq_req_mode { - TXQ_REQ_MODE_THREAD, - TXQ_REQ_MODE_HASH, -}; - -enum txq_mode { - TXQ_MODE_STATIC, - TXQ_MODE_XPS, - TXQ_MODE_XPS_HASH, -}; - /* A port in a netdev-based datapath. */ struct dp_netdev_port { odp_port_t port_no; @@ -4993,6 +4995,14 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) } first_set_config = false; + + const char *vhost_tx_steering_default = + smap_get_def(other_config, "vhost-tx-steering-default", "thread"); + if (nullable_string_is_equal(vhost_tx_steering_default, "hash")) { + dp->vhost_txq_requested_mode_default = TXQ_REQ_MODE_HASH; + } else { + dp->vhost_txq_requested_mode_default = TXQ_REQ_MODE_THREAD; + } return 0; } @@ -5143,10 +5153,17 @@ dpif_netdev_port_set_config(struct dpif *dpif, odp_port_t port_no, dp_netdev_request_reconfigure(dp); } - if (nullable_string_is_equal(tx_steering_mode, "hash")) { + if (!strncmp(port->type, "dpdkvhost", 9) + && dp->vhost_txq_requested_mode_default == TXQ_REQ_MODE_HASH) { txq_mode = TXQ_REQ_MODE_HASH; } else { - txq_mode = TXQ_REQ_MODE_THREAD; + txq_mode = TXQ_REQ_MODE_THREAD; + } + + if (nullable_string_is_equal(tx_steering_mode, "hash")) { + txq_mode = TXQ_REQ_MODE_HASH; + } else if (nullable_string_is_equal(tx_steering_mode, "thread")) { + txq_mode = TXQ_REQ_MODE_THREAD; } if (txq_mode != port->txq_requested_mode) { diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index edb5eafa0..84655dfcd 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -3513,6 +3513,18 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ </p> </column> + <column name="other_config" key="vhost-tx-steering-default" + type='{"type": "string", + "enum": ["set", ["thread", "hash"]]}'> + <p> + Specifies the default Tx steering mode for dpdkvhost interfaces. + </p> + <p> + Defaults to <code>thread</code>, and is overriden per + Interface by the <code>tx-steering</code> option. + </p> + </column> + </group> <group title="EMC (Exact Match Cache) Configuration">
Current tx_steering mode has to be configured on each Interface. The global config option "vhost-tx-steering-default" allows the user to select a default mode (thread or hash) for all dpdkvhost ports. Signed-off-by: Christophe Fontaine <cfontain@redhat.com> --- NEWS | 1 + lib/dpif-netdev.c | 43 ++++++++++++++++++++++++++++++------------- vswitchd/vswitch.xml | 12 ++++++++++++ 3 files changed, 43 insertions(+), 13 deletions(-)