Hi Dave
We are happy to announce SRIOV E-Switch offload and VF netdev representors.
Or Gerlitz says:
Currently, the way SR-IOV embedded switches are dealt with in Linux is limited
in its expressiveness and flexibility, but this is not necessarily due to
hardware limitations. The kernel software model for controlling the SR-IOV
switch simply does not allow the configuration of anything more complex than
MAC/VLAN based forwarding.
Hence the benefits brought by SRIOV come at a price of management flexibility,
when compared to software virtual switches which are used in Para-Virtual (PV)
schemes and allow implementing complex policies and virtual topologies. Such
SW switching typically involved a complex per-packet processing within the host
kernel using subsystems such as TC, Bridge, Netfilter and Open-vswitch.
We'd like to change that and get the best of both worlds: the performance of SR-IOV
with the management flexibility of software switches. This will eventually include
a richer model for controlling the SR-IOV switch for flow-based switching and
tunneling. Under this model, the e-switch is configured dynamically and a fallback
to software exists in case the hardware is unable to offload all required flows.
This series from Hadar Hen-Zion and myself, is the 1st step in that direction,
specfically, it provides full control on the SRIOV embedded switching by host
software and paves the way to offload switching rules and polices with downstream
patches.
To allow for host based SW control on the SRIOV HW switch, we introduce per VF
representor host netdevice. The VF representor plays the same role as TAP devices
in PV setup. A packet send through the VF representor on the host arrives to
the VF, and a packet sent through the VF is received by its representor. The
administrator can hook the representor netdev into a kernel switching component.
Once they do that, packets from the VF are subject to steering (matching and
actions) of that software component."
Doing so indeed hurts the performance benefits of SRIOV as it forces all the
traffic to go through the hypervisor. However, this SW representation is what
would eventually allow us to introduce hybrid model, where we offload steering
for some of the VF/VM traffic to the HW while keeping other VM traffic to go
through the hypervisor. Examples for the latter are first packet of flows which
are needed for SW switches learning and/or matching against policy database or
types of traffic for which offloading is not desired or not supported by the
current HW eswitch generation.
The embedded switch is managed through a PCI device driver. As such, we introduce
a devlink/pci based scheme for setting the mode of the e-switch. The current mode
(where steering is done based on mac/vlan, etc) is referred to as "legacy" and the
new mode as "offloads".
For the mlx5 driver / ConnectX4 HW case, the VF representors implement a functional
subset of mlx5e Ethernet netdevices using their own profile. This design buys us robust
implementation with code reuse and sharing.
The representors are created by the host PCI driver when (1) in SRIOV and (2) the
e-switch is set to offloads mode. Currently, in mlx5 the e-switch management is done
through the PF vport (0) and hence the VF representors along with the existing PF
netdev which represents the uplink share the PCI PF device instance.
The series is built from two major components, the first relates to the e-switch
management and the second to VF representors.
We start with a refactoring that treats the existing SRIOV e-switch code as of operating
in legacy mode. Next, we add the code for the offloads mode which programs the e-switch
to operate in a way which serves for software based switching:
1. miss rule which matches all packets that do not match any HW other switching rule
and forwards them to the e-switch management port (0) for further processing.
2. infrastructure for send-to-vport rules which conceptually bypass other "normal"
steering rules which present at the e-switch datapath. Such rules apply only for packets
that originate in the e-switch manager vport (0).
Since all the VF reps run over the same e-switch port, we use more logic in the host PCI
driver to do HW steering of missed packets into the HW queue opened by a the respective VF
representor. Finally here, we add the devlink APIs to configure the e-switch mode.
The second part from Hadar starts with some refactoring work which allow for multiple
mlx5e NIC instances to be created over the same PCI function, use common resources
and avoid wrong loopbacks.
Next comes the heart of the change which is a profile definition which allow to practically
have both "conventional" mlx5e NIC use cases such as native mode (non SRIOV), VF, PF and VF
representor to share the Ethernet driver code. This is done by a small surgery that ended up
with few internal callbacks that should be implemented by a profile instance. The profile
for the conventional NIC is implemented, to preserve the existing functionality.
The last two patches add e-switch registration API for the VF representors and the
implementation of the VF representors netdevice profile. Being an mlx5e instance, the
VF representor uses HW send/recv queues, completions queues and such. It currently doesn't
support NIC offloads but some of them could be added later on. The VF representor has
switchdev ops, where currently the only supported API is the one to the HW ID,
which is needed to identify multiple representors belonging to the same e-switch.
The architecture + solution (software and firmware) work were done by a team consisting
of Ilya Lesokhin, Haggai Eran, Rony Efraim, Tal Anker, Natan Oppenheimer, Saeed Mahameed,
Hadar and Or, thanks you all!
Thanks,
Or & Saeed.
Hadar Hen Zion (6):
net/mlx5e: Create NIC global resources only once
net/mlx5e: TIRs management refactoring
net/mlx5e: Mark enabled RQTs instances explicitly
net/mlx5e: Add support for multiple profiles
net/mlx5: Add Representors registration API
net/mlx5e: Introduce SRIOV VF representors
Or Gerlitz (10):
net/mlx5: E-Switch, Add operational mode to the SRIOV e-Switch
net/mlx5: E-Switch, Add support for the sriov offloads mode
net/mlx5: E-Switch, Add miss rule for offloads mode
net/mlx5: E-Switch, Add API to create send-to-vport rules
net/mlx5: Introduce offloads steering namespace
net/mlx5: E-Switch, Add offloads table
net/mlx5: E-Switch, Add API to create vport rx rules
net/devlink: Add E-Switch mode control
net/mlx5: Add devlink interface
net/mlx5e: Add devlink based SRIOV mode changes (legacy --> offloads)
drivers/net/ethernet/mellanox/mlx5/core/Kconfig | 1 +
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 8 +-
drivers/net/ethernet/mellanox/mlx5/core/en.h | 73 ++-
drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c | 14 +-
.../net/ethernet/mellanox/mlx5/core/en_common.c | 160 ++++++
.../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 4 +-
drivers/net/ethernet/mellanox/mlx5/core/en_fs.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 627 ++++++++++++---------
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 387 +++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 90 +--
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 78 ++-
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 549 ++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/fs_core.c | 11 +-
drivers/net/ethernet/mellanox/mlx5/core/main.c | 26 +-
drivers/net/ethernet/mellanox/mlx5/core/sriov.c | 5 +-
include/linux/mlx5/driver.h | 13 +
include/linux/mlx5/fs.h | 1 +
include/net/devlink.h | 3 +
include/uapi/linux/devlink.h | 9 +
net/core/devlink.c | 87 +++
20 files changed, 1817 insertions(+), 331 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_common.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
--
2.8.0
From: Or Gerlitz <redacted>
Define three modes for the SRIOV e-switch operation, none (SRIOV_NONE,
none of the VF vports are enabled), legacy (SRIOV_LEGACY, the current mode)
and sriov offloads (SRIOV_OFFLOADS). Currently, when in SRIOV, only the
legacy mode is supported, where steering rules are of the form:
destination mac --> VF vport
This patch does not change any functionality.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 51 +++++++++++++----------
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 19 +++++++--
drivers/net/ethernet/mellanox/mlx5/core/sriov.c | 5 ++-
3 files changed, 46 insertions(+), 29 deletions(-)
@@ -479,7 +479,7 @@ static int esw_create_fdb_table(struct mlx5_eswitch *esw, int nvports)esw_warn(dev,"Failed to create flow group err(%d)\n",err);gotoout;}-esw->fdb_table.addr_grp=g;+esw->fdb_table.legacy.addr_grp=g;/* Allmulti group : One rule that forwards any mcast traffic */MLX5_SET(create_flow_group_in,flow_group_in,match_criteria_enable,
@@ -494,7 +494,7 @@ static int esw_create_fdb_table(struct mlx5_eswitch *esw, int nvports)esw_warn(dev,"Failed to create allmulti flow group err(%d)\n",err);gotoout;}-esw->fdb_table.allmulti_grp=g;+esw->fdb_table.legacy.allmulti_grp=g;/* Promiscuous group :*Onerulethatforwardallunmatchedtrafficfrompreviousgroups
@@ -511,17 +511,17 @@ static int esw_create_fdb_table(struct mlx5_eswitch *esw, int nvports)esw_warn(dev,"Failed to create promisc flow group err(%d)\n",err);gotoout;}-esw->fdb_table.promisc_grp=g;+esw->fdb_table.legacy.promisc_grp=g;out:if(err){-if(!IS_ERR_OR_NULL(esw->fdb_table.allmulti_grp)){-mlx5_destroy_flow_group(esw->fdb_table.allmulti_grp);-esw->fdb_table.allmulti_grp=NULL;+if(!IS_ERR_OR_NULL(esw->fdb_table.legacy.allmulti_grp)){+mlx5_destroy_flow_group(esw->fdb_table.legacy.allmulti_grp);+esw->fdb_table.legacy.allmulti_grp=NULL;}-if(!IS_ERR_OR_NULL(esw->fdb_table.addr_grp)){-mlx5_destroy_flow_group(esw->fdb_table.addr_grp);-esw->fdb_table.addr_grp=NULL;+if(!IS_ERR_OR_NULL(esw->fdb_table.legacy.addr_grp)){+mlx5_destroy_flow_group(esw->fdb_table.legacy.addr_grp);+esw->fdb_table.legacy.addr_grp=NULL;}if(!IS_ERR_OR_NULL(esw->fdb_table.fdb)){mlx5_destroy_flow_table(esw->fdb_table.fdb);
@@ -1540,7 +1540,7 @@ static void esw_disable_vport(struct mlx5_eswitch *esw, int vport_num)}/* Public E-Switch API */-intmlx5_eswitch_enable_sriov(structmlx5_eswitch*esw,intnvfs)+intmlx5_eswitch_enable_sriov(structmlx5_eswitch*esw,intnvfs,intmode){interr;inti;
@@ -1561,11 +1561,14 @@ int mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs)if(!MLX5_CAP_ESW_EGRESS_ACL(esw->dev,ft_support))esw_warn(esw->dev,"E-Switch engress ACL is not supported by FW\n");-esw_info(esw->dev,"E-Switch enable SRIOV: nvfs(%d)\n",nvfs);+esw_info(esw->dev,"E-Switch enable SRIOV: nvfs(%d) mode (%d)\n",nvfs,mode);+if(mode!=SRIOV_LEGACY)+return-EINVAL;+esw->mode=mode;esw_disable_vport(esw,0);-err=esw_create_fdb_table(esw,nvfs+1);+err=esw_create_legacy_fdb_table(esw,nvfs+1);if(err)gotoabort;
@@ -1590,8 +1593,8 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw)MLX5_CAP_GEN(esw->dev,port_type)!=MLX5_CAP_PORT_TYPE_ETH)return;-esw_info(esw->dev,"disable SRIOV: active vports(%d)\n",-esw->enabled_vports);+esw_info(esw->dev,"disable SRIOV: active vports(%d) mode(%d)\n",+esw->enabled_vports,esw->mode);mc_promisc=esw->mc_promisc;
@@ -1601,8 +1604,9 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw)if(mc_promisc&&mc_promisc->uplink_rule)mlx5_del_flow_rule(mc_promisc->uplink_rule);-esw_destroy_fdb_table(esw);+esw_destroy_legacy_fdb_table(esw);+esw->mode=SRIOV_NONE;/* VPORT 0 (PF) must be enabled back with non-sriov configuration */esw_enable_vport(esw,0,UC_ADDR_CHANGE);}
@@ -1673,6 +1677,7 @@ int mlx5_eswitch_init(struct mlx5_core_dev *dev)esw->total_vports=total_vports;esw->enabled_vports=0;+esw->mode=SRIOV_NONE;dev->priv.eswitch=esw;esw_enable_vport(esw,0,UC_ADDR_CHANGE);
From: Or Gerlitz <redacted>
Unlike the legacy mode, here, forwarding rules are not learned by the
driver per events on macs set by VFs/VMs into their vports, but rather
should be programmed by higher-level SW entities.
Saying that, still, in the offloads mode (SRIOV_OFFLOADS), two flow
groups are created by the driver for management (slow path) purposes:
The first group will be used for sending packets over e-switch vports
from the host OS where the e-switch management code runs, to be
received by VFs.
The second group will be used by a miss rule which forwards packets toward
the e-switch manager. Further logic will trap these packets such that
the receiving net-device as seen by the networking stack is the representor
of the vport that sent the packet over the e-switch data-path.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 35 +++---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 16 +++
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 135 +++++++++++++++++++++
4 files changed, 168 insertions(+), 20 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -1562,18 +1555,19 @@ int mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs, int mode)esw_warn(esw->dev,"E-Switch engress ACL is not supported by FW\n");esw_info(esw->dev,"E-Switch enable SRIOV: nvfs(%d) mode (%d)\n",nvfs,mode);-if(mode!=SRIOV_LEGACY)-return-EINVAL;-esw->mode=mode;esw_disable_vport(esw,0);-err=esw_create_legacy_fdb_table(esw,nvfs+1);+if(mode==SRIOV_LEGACY)+err=esw_create_legacy_fdb_table(esw,nvfs+1);+else+err=esw_create_offloads_fdb_table(esw,nvfs+1);if(err)gotoabort;+enabled_events=(mode==SRIOV_LEGACY)?SRIOV_VPORT_EVENTS:UC_ADDR_CHANGE;for(i=0;i<=nvfs;i++)-esw_enable_vport(esw,i,SRIOV_VPORT_EVENTS);+esw_enable_vport(esw,i,enabled_events);esw_info(esw->dev,"SRIOV enabled: active vports(%d)\n",esw->enabled_vports);
@@ -1604,7 +1598,10 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw)if(mc_promisc&&mc_promisc->uplink_rule)mlx5_del_flow_rule(mc_promisc->uplink_rule);-esw_destroy_legacy_fdb_table(esw);+if(esw->mode==SRIOV_LEGACY)+esw_destroy_legacy_fdb_table(esw);+else+esw_destroy_offloads_fdb_table(esw);esw->mode=SRIOV_NONE;/* VPORT 0 (PF) must be enabled back with non-sriov configuration */
@@ -0,0 +1,135 @@+/*+*Copyright(c)2016,MellanoxTechnologies.Allrightsreserved.+*+*Thissoftwareisavailabletoyouunderachoiceofoneoftwo+*licenses.YoumaychoosetobelicensedunderthetermsoftheGNU+*GeneralPublicLicense(GPL)Version2,availablefromthefile+*COPYINGinthemaindirectoryofthissourcetree,orthe+*OpenIB.orgBSDlicensebelow:+*+*Redistributionanduseinsourceandbinaryforms,withor+*withoutmodification,arepermittedprovidedthatthefollowing+*conditionsaremet:+*+*-Redistributionsofsourcecodemustretaintheabove+*copyrightnotice,thislistofconditionsandthefollowing+*disclaimer.+*+*-Redistributionsinbinaryformmustreproducetheabove+*copyrightnotice,thislistofconditionsandthefollowing+*disclaimerinthedocumentationand/orothermaterials+*providedwiththedistribution.+*+*THESOFTWAREISPROVIDED"AS IS",WITHOUTWARRANTYOFANYKIND,+*EXPRESSORIMPLIED,INCLUDINGBUTNOTLIMITEDTOTHEWARRANTIESOF+*MERCHANTABILITY,FITNESSFORAPARTICULARPURPOSEAND+*NONINFRINGEMENT.INNOEVENTSHALLTHEAUTHORSORCOPYRIGHTHOLDERS+*BELIABLEFORANYCLAIM,DAMAGESOROTHERLIABILITY,WHETHERINAN+*ACTIONOFCONTRACT,TORTOROTHERWISE,ARISINGFROM,OUTOFORIN+*CONNECTIONWITHTHESOFTWAREORTHEUSEOROTHERDEALINGSINTHE+*SOFTWARE.+*/++#include<linux/etherdevice.h>+#include<linux/mlx5/driver.h>+#include<linux/mlx5/mlx5_ifc.h>+#include<linux/mlx5/vport.h>+#include<linux/mlx5/fs.h>+#include"mlx5_core.h"+#include"eswitch.h"++#define MAX_PF_SQ 256++intesw_create_offloads_fdb_table(structmlx5_eswitch*esw,intnvports)+{+intinlen=MLX5_ST_SZ_BYTES(create_flow_group_in);+structmlx5_core_dev*dev=esw->dev;+structmlx5_flow_namespace*root_ns;+structmlx5_flow_table*fdb=NULL;+structmlx5_flow_group*g;+u32*flow_group_in;+void*match_criteria;+inttable_size,ix,err=0;++flow_group_in=mlx5_vzalloc(inlen);+if(!flow_group_in)+return-ENOMEM;++root_ns=mlx5_get_flow_namespace(dev,MLX5_FLOW_NAMESPACE_FDB);+if(!root_ns){+esw_warn(dev,"Failed to get FDB flow namespace\n");+gotons_err;+}++esw_debug(dev,"Create offloads FDB table, log_max_size(%d)\n",+MLX5_CAP_ESW_FLOWTABLE_FDB(dev,log_max_ft_size));++table_size=nvports+MAX_PF_SQ+1;+fdb=mlx5_create_flow_table(root_ns,0,table_size,0);+if(IS_ERR(fdb)){+err=PTR_ERR(fdb);+esw_warn(dev,"Failed to create FDB Table err %d\n",err);+gotofdb_err;+}+esw->fdb_table.fdb=fdb;++/* create send-to-vport group */+memset(flow_group_in,0,inlen);+MLX5_SET(create_flow_group_in,flow_group_in,match_criteria_enable,+MLX5_MATCH_MISC_PARAMETERS);++match_criteria=MLX5_ADDR_OF(create_flow_group_in,flow_group_in,match_criteria);++MLX5_SET_TO_ONES(fte_match_param,match_criteria,misc_parameters.source_sqn);+MLX5_SET_TO_ONES(fte_match_param,match_criteria,misc_parameters.source_port);++ix=nvports+MAX_PF_SQ;+MLX5_SET(create_flow_group_in,flow_group_in,start_flow_index,0);+MLX5_SET(create_flow_group_in,flow_group_in,end_flow_index,ix-1);++g=mlx5_create_flow_group(fdb,flow_group_in);+if(IS_ERR(g)){+err=PTR_ERR(g);+esw_warn(dev,"Failed to create send-to-vport flow group err(%d)\n",err);+gotosend_vport_err;+}+esw->fdb_table.offloads.send_to_vport_grp=g;++/* create miss group */+memset(flow_group_in,0,inlen);+MLX5_SET(create_flow_group_in,flow_group_in,match_criteria_enable,0);++MLX5_SET(create_flow_group_in,flow_group_in,start_flow_index,ix);+MLX5_SET(create_flow_group_in,flow_group_in,end_flow_index,ix+1);++g=mlx5_create_flow_group(fdb,flow_group_in);+if(IS_ERR(g)){+err=PTR_ERR(g);+esw_warn(dev,"Failed to create miss flow group err(%d)\n",err);+gotomiss_err;+}+esw->fdb_table.offloads.miss_grp=g;++return0;++miss_err:+mlx5_destroy_flow_group(esw->fdb_table.offloads.send_to_vport_grp);+send_vport_err:+mlx5_destroy_flow_table(fdb);+fdb_err:+ns_err:+kvfree(flow_group_in);+returnerr;+}++voidesw_destroy_offloads_fdb_table(structmlx5_eswitch*esw)+{+if(!esw->fdb_table.fdb)+return;++esw_debug(esw->dev,"Destroy offloads FDB Table\n");+mlx5_destroy_flow_group(esw->fdb_table.offloads.send_to_vport_grp);+mlx5_destroy_flow_group(esw->fdb_table.offloads.miss_grp);++mlx5_destroy_flow_table(esw->fdb_table.fdb);+}
From: Or Gerlitz <redacted>
Add the API to create send-to-vport e-switch rules of the form
packet meta-data :: send-queue-number == $SQN and source-vport == 0 --> $VPORT
These rules are to be used for a send-to-vport logic which conceptually bypasses
the "normal" steering rules currently present at the e-switch datapath.
Such rule should apply only for packets that originate in the e-switch manager
vport (0) and are sent for a given SQN which is used by a given VF representor
device, and hence the matching logic.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 3 +-
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 39 ++++++++++++++++++++++
2 files changed, 41 insertions(+), 1 deletion(-)
@@ -38,6 +38,45 @@#include"mlx5_core.h"#include"eswitch.h"+structmlx5_flow_rule*+mlx5_eswitch_add_send_to_vport_rule(structmlx5_eswitch*esw,intvport,u32sqn)+{+structmlx5_flow_destinationdest;+structmlx5_flow_rule*flow_rule;+intmatch_header=MLX5_MATCH_MISC_PARAMETERS;+u32*match_v,*match_c;+void*misc;++match_v=kzalloc(MLX5_ST_SZ_BYTES(fte_match_param),GFP_KERNEL);+match_c=kzalloc(MLX5_ST_SZ_BYTES(fte_match_param),GFP_KERNEL);+if(!match_v||!match_c){+esw_warn(esw->dev,"FDB: Failed to alloc match parameters\n");+flow_rule=ERR_PTR(-ENOMEM);+gotoout;+}++misc=MLX5_ADDR_OF(fte_match_param,match_v,misc_parameters);+MLX5_SET(fte_match_set_misc,misc,source_sqn,sqn);+MLX5_SET(fte_match_set_misc,misc,source_port,0x0);/* source vport is 0 */++misc=MLX5_ADDR_OF(fte_match_param,match_c,misc_parameters);+MLX5_SET_TO_ONES(fte_match_set_misc,misc,source_sqn);+MLX5_SET_TO_ONES(fte_match_set_misc,misc,source_port);++dest.type=MLX5_FLOW_DESTINATION_TYPE_VPORT;+dest.vport_num=vport;++flow_rule=mlx5_add_flow_rule(esw->fdb_table.fdb,match_header,match_c,+match_v,MLX5_FLOW_CONTEXT_ACTION_FWD_DEST,+0,&dest);+if(IS_ERR(flow_rule))+esw_warn(esw->dev,"FDB: Failed to add send to vport rule err %ld\n",PTR_ERR(flow_rule));+out:+kfree(match_v);+kfree(match_c);+returnflow_rule;+}+staticintesw_add_fdb_miss_rule(structmlx5_eswitch*esw){structmlx5_flow_destinationdest;
From: Or Gerlitz <redacted>
In the sriov offloads mode, packets that are not matched by any other
rule should be sent towards the e-switch manager for further processing.
Add such "miss" rule which matches ANY packet as the last rule in the
e-switch FDB and programs the HW to send the packet to vport 0 where
the e-switch manager runs.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 1 +
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 42 ++++++++++++++++++++++
2 files changed, 43 insertions(+)
@@ -38,6 +38,41 @@#include"mlx5_core.h"#include"eswitch.h"+staticintesw_add_fdb_miss_rule(structmlx5_eswitch*esw)+{+structmlx5_flow_destinationdest;+structmlx5_flow_rule*flow_rule=NULL;+intmatch_header=0;+u32*match_v,*match_c;+interr=0;++match_v=kzalloc(MLX5_ST_SZ_BYTES(fte_match_param),GFP_KERNEL);+match_c=kzalloc(MLX5_ST_SZ_BYTES(fte_match_param),GFP_KERNEL);+if(!match_v||!match_c){+esw_warn(esw->dev,"FDB: Failed to alloc match parameters\n");+err=-ENOMEM;+gotoout;+}++dest.type=MLX5_FLOW_DESTINATION_TYPE_VPORT;+dest.vport_num=0;++flow_rule=mlx5_add_flow_rule(esw->fdb_table.fdb,match_header,match_c,+match_v,MLX5_FLOW_CONTEXT_ACTION_FWD_DEST,+0,&dest);+if(IS_ERR(flow_rule)){+err=PTR_ERR(flow_rule);+esw_warn(esw->dev,"FDB: Failed to add miss flow rule err %d\n",err);+gotoout;+}++esw->fdb_table.offloads.miss_rule=flow_rule;+out:+kfree(match_v);+kfree(match_c);+returnerr;+}+#define MAX_PF_SQ 256intesw_create_offloads_fdb_table(structmlx5_eswitch*esw,intnvports)
@@ -110,8 +145,14 @@ int esw_create_offloads_fdb_table(struct mlx5_eswitch *esw, int nvports)}esw->fdb_table.offloads.miss_grp=g;+err=esw_add_fdb_miss_rule(esw);+if(err)+gotomiss_rule_err;+return0;+miss_rule_err:+mlx5_destroy_flow_group(esw->fdb_table.offloads.miss_grp);miss_err:mlx5_destroy_flow_group(esw->fdb_table.offloads.send_to_vport_grp);send_vport_err:
From: Hadar Hen Zion <redacted>
The current refresh tirs self loopback mechanism, refreshes all the tirs
belonging to the same mlx5e instance to prevent self loopback by packets
sent over any ring of that instance. This mechanism relies on all the
tirs/tises of an instance to be created with the same transport domain
number (tdn).
Change the driver to refresh all the tirs created under the same tdn
regardless of which mlx5e netdev instance they belong to.
This behaviour is needed for introducing new mlx5e instances which serve
to represent SRIOV VFs. The representors and the PF share vport used for
E-Switch management, and we want to avoid NIC level HW loopback between
them, e.g when sending broadcast packets. To achieve that, both the
representors and the PF NIC will share the tdn.
This patch doesn't add any new functionality.
Signed-off-by: Hadar Hen Zion <redacted>
Reviewed-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 12 +++--
drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c | 14 +++---
.../net/ethernet/mellanox/mlx5/core/en_common.c | 48 +++++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en_fs.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 56 +++++-----------------
6 files changed, 77 insertions(+), 57 deletions(-)
From: Or Gerlitz <redacted>
Implement handlers for the devlink commands to get and set the SRIOV
E-Switch mode.
When turning to the offloads mode, we disable the e-switch and enable
it again in the new mode, create the NIC offloads table and create VF reps.
When turning to legacy mode, we remove the VF reps and the offloads
table, and re-initiate the e-switch in it's legacy mode.
The actual creation/removal of the VF reps is done in downstream patches.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 12 ++-
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 ++++++++++++++++++++-
2 files changed, 105 insertions(+), 9 deletions(-)
@@ -1561,7 +1561,7 @@ int mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs, int mode)if(mode==SRIOV_LEGACY)err=esw_create_legacy_fdb_table(esw,nvfs+1);else-err=esw_create_offloads_fdb_table(esw,nvfs+1);+err=esw_offloads_init(esw,nvfs+1);if(err)gotoabort;
@@ -1600,8 +1602,8 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw)if(esw->mode==SRIOV_LEGACY)esw_destroy_legacy_fdb_table(esw);-else-esw_destroy_offloads_fdb_table(esw);+elseif(esw->mode==SRIOV_OFFLOADS)+esw_offloads_cleanup(esw,nvports);esw->mode=SRIOV_NONE;/* VPORT 0 (PF) must be enabled back with non-sriov configuration */
@@ -331,12 +331,106 @@ out:returnflow_rule;}+staticintesw_offloads_start(structmlx5_eswitch*esw)+{+interr,num_vfs=esw->dev->priv.sriov.num_vfs;++if(esw->mode!=SRIOV_LEGACY){+esw_warn(esw->dev,"Can't set offloads mode, SRIOV legacy not enabled\n");+return-EINVAL;+}++mlx5_eswitch_disable_sriov(esw);+err=mlx5_eswitch_enable_sriov(esw,num_vfs,SRIOV_OFFLOADS);+if(err)+esw_warn(esw->dev,"Failed set eswitch to offloads, err %d\n",err);+returnerr;+}++intesw_offloads_init(structmlx5_eswitch*esw,intnvports)+{+interr;++err=esw_create_offloads_fdb_table(esw,nvports);+if(err)+returnerr;++err=esw_create_offloads_table(esw);+if(err)+gotocreate_ft_err;++err=esw_create_vport_rx_group(esw);+if(err)+gotocreate_fg_err;++return0;++create_fg_err:+esw_destroy_offloads_table(esw);++create_ft_err:+esw_destroy_offloads_fdb_table(esw);+returnerr;+}++staticintesw_offloads_stop(structmlx5_eswitch*esw)+{+interr,num_vfs=esw->dev->priv.sriov.num_vfs;++mlx5_eswitch_disable_sriov(esw);+err=mlx5_eswitch_enable_sriov(esw,num_vfs,SRIOV_LEGACY);+if(err)+esw_warn(esw->dev,"Failed set eswitch legacy mode. err %d\n",err);++returnerr;+}++voidesw_offloads_cleanup(structmlx5_eswitch*esw,intnvports)+{+esw_destroy_vport_rx_group(esw);+esw_destroy_offloads_table(esw);+esw_destroy_offloads_fdb_table(esw);+}+intmlx5_devlink_eswitch_mode_set(structdevlink*devlink,u16mode){-return-EOPNOTSUPP;+structmlx5_core_dev*dev;+u16cur_mode;++dev=devlink_priv(devlink);++if(!MLX5_CAP_GEN(dev,vport_group_manager))+return-EOPNOTSUPP;++cur_mode=dev->priv.eswitch->mode;++if(cur_mode==SRIOV_NONE||mode==SRIOV_NONE)+return-EOPNOTSUPP;++if(cur_mode==mode)+return0;++if(mode==SRIOV_OFFLOADS)/* current mode is legacy */+returnesw_offloads_start(dev->priv.eswitch);+elseif(mode==SRIOV_LEGACY)/* curreny mode is offloads */+returnesw_offloads_stop(dev->priv.eswitch);+else+return-EINVAL;}intmlx5_devlink_eswitch_mode_get(structdevlink*devlink,u16*mode){-return-EOPNOTSUPP;+structmlx5_core_dev*dev;++dev=devlink_priv(devlink);++if(!MLX5_CAP_GEN(dev,vport_group_manager))+return-EOPNOTSUPP;++if(dev->priv.eswitch->mode==SRIOV_NONE)+return-EOPNOTSUPP;++*mode=dev->priv.eswitch->mode;++return0;}
From: Hadar Hen Zion <redacted>
To allow support in representor netdevices where we create more than one
netdevice per NIC, add profiles to the mlx5e driver. The profiling
allows for creation of mlx5e instances with different characteristics.
Each profile implements its own behavior using set of function pointers
defined in struct mlx5e_profile. This is done to allow for avoiding complex
per profix branching in the code.
Currently only the profile for the conventional NIC is implemented,
which is of use when a netdev is created upon pci probe.
This patch doesn't add any new functionality.
Signed-off-by: Hadar Hen Zion <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 17 ++
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 341 ++++++++++++++--------
2 files changed, 240 insertions(+), 118 deletions(-)
From: Or Gerlitz <redacted>
Add the commands to set and show the mode of SRIOV E-Switch,
two modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows based) set by the host OS
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
include/net/devlink.h | 3 ++
include/uapi/linux/devlink.h | 9 +++++
net/core/devlink.c | 87 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 99 insertions(+)
@@ -57,6 +57,8 @@ enum devlink_command {DEVLINK_CMD_SB_OCC_SNAPSHOT,DEVLINK_CMD_SB_OCC_MAX_CLEAR,+DEVLINK_CMD_ESWITCH_MODE_GET,+DEVLINK_CMD_ESWITCH_MODE_SET,/* add new commands above here */__DEVLINK_CMD_MAX,
@@ -95,6 +97,12 @@ enum devlink_sb_threshold_type {#define DEVLINK_SB_THRESHOLD_TO_ALPHA_MAX 20+enumdevlink_eswitch_mode{+DEVLINK_ESWITCH_MODE_NONE,+DEVLINK_ESWITCH_MODE_LEGACY,+DEVLINK_ESWITCH_MODE_OFFLOADS,+};+enumdevlink_attr{/* don't change the order or add anything between, this is ABI! */DEVLINK_ATTR_UNSPEC,
@@ -125,6 +133,7 @@ enum devlink_attr {DEVLINK_ATTR_SB_TC_INDEX,/* u16 */DEVLINK_ATTR_SB_OCC_CUR,/* u32 */DEVLINK_ATTR_SB_OCC_MAX,/* u32 */+DEVLINK_ATTR_ESWITCH_MODE,/* u16 *//* add new attributes above here, update the policy in devlink.c */
From: Hadar Hen Zion <redacted>
To allow creating more than one netdev over the same PCI function, we
change the driver such that global NIC resources are created once and
later be shared amongst all the mlx5e netdevs running over that port.
Move the CQ UAR, PD (pdn), Transport Domain (tdn), MKey resources from
being kept in the mlx5e priv part to a new resources structure
(mlx5e_resources) placed under the mlx5_core device.
This patch doesn't add any new functionality.
Signed-off-by: Hadar Hen Zion <redacted>
Reviewed-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 6 +-
drivers/net/ethernet/mellanox/mlx5/core/en.h | 6 +-
.../net/ethernet/mellanox/mlx5/core/en_common.c | 112 +++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 124 +++++++--------------
include/linux/mlx5/driver.h | 13 +++
5 files changed, 171 insertions(+), 90 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_common.c
From: Or Gerlitz <redacted>
The devlink interface is initially used to set/get the mode of the SRIOV e-switch.
Currently, these are only stubs for get/set, down-stream patch will actually
fill them out.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/Kconfig | 1 +
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 4 ++++
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 10 +++++++++
drivers/net/ethernet/mellanox/mlx5/core/main.c | 26 ++++++++++++++++++----
4 files changed, 37 insertions(+), 4 deletions(-)
From: Hadar Hen Zion <redacted>
In the current driver implementation two types of receive queue
tables (RQTs) are in use - direct and indirect.
Change the driver to mark each new created RQT (direct or indirect)
as "enabled". This behaviour is needed for introducing new mlx5e
instances which serve to represent SRIOV VFs.
The VF representors will have only one type of RQTs (direct).
An "enabled" flag is added to each RQT to allow better handling
and code sharing between the representors and the nic netdevices.
This patch doesn't add any new functionality.
Signed-off-by: Hadar Hen Zion <redacted>
Reviewed-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 13 +++++--
.../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 45 +++++++++++++---------
3 files changed, 37 insertions(+), 23 deletions(-)
From: Hadar Hen Zion <redacted>
Implement the relevant profile functions to create mlx5e driver instance
serving as VF representor. When SRIOV offloads mode is enabled, each VF
will have a representor netdevice instance on the host.
To do that, we also export set of shared service functions from en_main.c,
such that they can be used by both NIC and repsresentors netdevs.
The newly created representor netdevice has a basic set of net_device_ops
which are the same ndo functions as the NIC netdevice and an ndo of it's
own for phys port name.
The profiling infrastructure allow sharing code between the NIC and the
vport representor even though the representor has only a subset of the
NIC functionality.
The VF reps and the PF which is used in that mode to represent the uplink,
expose switchdev ops. Currently the only op supposed is attr get for the
port parent ID which here serves to identify net-devices belonging to the
same HW E-Switch. Other than that, no offloading is implemented and hence
switching functionality is achieved if one sets SW switching rules, e.g
using tc, bridge or ovs.
Port phys name (ndo_get_phys_port_name) is implemented to allow exporting
to user-space the VF vport number and along with the switchdev port parent
id (phys_switch_id) enable a udev base consistent naming scheme:
SUBSYSTEM=="net", ACTION=="add", ATTR{phys_switch_id}=="<phys_switch_id>", \
ATTR{phys_port_name}!="", NAME="$PF_NIC$attr{phys_port_name}"
where phys_switch_id is exposed by the PF (and VF reps) and $PF_NIC is
the name of the PF netdevice.
Signed-off-by: Hadar Hen Zion <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en.h | 28 ++
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 53 ++-
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 387 +++++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 20 +-
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 96 ++++-
6 files changed, 567 insertions(+), 19 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -1806,6 +1812,7 @@ static int mlx5e_open(struct net_device *netdev)intmlx5e_close_locked(structnet_device*netdev){structmlx5e_priv*priv=netdev_priv(netdev);+structmlx5_core_dev*mdev=priv->mdev;/* May already be CLOSED in case a previous configuration operation*(e.gRX/TXqueuesizechange)thatinvolvesclose&openfailed.
@@ -1815,6 +1822,9 @@ int mlx5e_close_locked(struct net_device *netdev)clear_bit(MLX5E_STATE_OPENED,&priv->state);+if(MLX5_CAP_GEN(mdev,vport_group_manager))+mlx5e_remove_sqs_fwd_rules(priv);+mlx5e_timestamp_cleanup(priv);netif_carrier_off(priv->netdev);mlx5e_redirect_rqts(priv);
@@ -1823,7 +1833,7 @@ int mlx5e_close_locked(struct net_device *netdev)return0;}-staticintmlx5e_close(structnet_device*netdev)+intmlx5e_close(structnet_device*netdev){structmlx5e_priv*priv=netdev_priv(netdev);interr;
@@ -77,6 +77,63 @@ out:returnflow_rule;}+voidmlx5_eswitch_sqs2vport_stop(structmlx5_eswitch*esw,+structmlx5_eswitch_rep*rep)+{+structmlx5_esw_sq*esw_sq,*tmp;++if(esw->mode!=SRIOV_OFFLOADS)+return;++list_for_each_entry_safe(esw_sq,tmp,&rep->vport_sqs_list,list){+mlx5_del_flow_rule(esw_sq->send_to_vport_rule);+list_del(&esw_sq->list);+kfree(esw_sq);+}+}++intmlx5_eswitch_sqs2vport_start(structmlx5_eswitch*esw,+structmlx5_eswitch_rep*rep,+u16*sqns_array,intsqns_num)+{+structmlx5_flow_rule*flow_rule;+structmlx5_esw_sq*esw_sq;+intvport;+interr;+inti;++if(esw->mode!=SRIOV_OFFLOADS)+return0;++vport=rep->vport==0?+FDB_UPLINK_VPORT:rep->vport;++for(i=0;i<sqns_num;i++){+esw_sq=kzalloc(sizeof(*esw_sq),GFP_KERNEL);+if(!esw_sq){+err=-ENOMEM;+gotoout_err;+}++/* Add re-inject rule to the PF/representor sqs */+flow_rule=mlx5_eswitch_add_send_to_vport_rule(esw,+vport,+sqns_array[i]);+if(IS_ERR(flow_rule)){+err=PTR_ERR(flow_rule);+kfree(esw_sq);+gotoout_err;+}+esw_sq->send_to_vport_rule=flow_rule;+list_add(&esw_sq->list,&rep->vport_sqs_list);+}+return0;++out_err:+mlx5_eswitch_sqs2vport_stop(esw,rep);+returnerr;+}+staticintesw_add_fdb_miss_rule(structmlx5_eswitch*esw){structmlx5_flow_destinationdest;
@@ -349,6 +406,8 @@ static int esw_offloads_start(struct mlx5_eswitch *esw)intesw_offloads_init(structmlx5_eswitch*esw,intnvports){+structmlx5_eswitch_rep*rep;+intvport;interr;err=esw_create_offloads_fdb_table(esw,nvports);
@@ -363,8 +422,26 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)if(err)gotocreate_fg_err;+for(vport=0;vport<nvports;vport++){+rep=&esw->offloads.vport_reps[vport];+if(!rep->valid)+continue;++err=rep->load(esw,rep);+if(err)+gotoerr_reps;+}return0;+err_reps:+for(vport--;vport>=0;vport--){+rep=&esw->offloads.vport_reps[vport];+if(!rep->valid)+continue;+rep->unload(esw,rep);+}+esw_destroy_vport_rx_group(esw);+create_fg_err:esw_destroy_offloads_table(esw);
@@ -387,6 +464,16 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw)voidesw_offloads_cleanup(structmlx5_eswitch*esw,intnvports){+structmlx5_eswitch_rep*rep;+intvport;++for(vport=0;vport<nvports;vport++){+rep=&esw->offloads.vport_reps[vport];+if(!rep->valid)+continue;+rep->unload(esw,rep);+}+esw_destroy_vport_rx_group(esw);esw_destroy_offloads_table(esw);esw_destroy_offloads_fdb_table(esw);
From: Or Gerlitz <redacted>
Belongs to the NIC offloads name-space, and to be used as part of the
SRIOV offloads logic to steer packets that hit the e-switch miss rule
to the TIR of the relevant VF representor.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 5 ++++
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 31 ++++++++++++++++++++++
2 files changed, 36 insertions(+)
From: Or Gerlitz <redacted>
Add a new namespace (MLX5_FLOW_NAMESPACE_OFFLOADS) to be populated
with flow steering rules that deal with rules that have have to
be executed before the EN NIC steering rules are matched.
The namespace is located after the bypass name-space and before the
kernel name-space. Therefore, it precedes the HW processing done for
rules set for the kernel NIC name-space.
Under SRIOV, it would allow us to match on e-switch missed packet
and forward them to the relevant VF representor TIR.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Amir Vadai <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/fs_core.c | 11 ++++++++++-
include/linux/mlx5/fs.h | 1 +
2 files changed, 11 insertions(+), 1 deletion(-)
From: Or Gerlitz <redacted>
Add the API to create vport rx rules of the form
packet meta-data :: vport == $VPORT --> $TIR
where the TIR is opened by this VF representor.
This logic will by used for packets that didn't match any rule in the
e-switch datapath and should be received into the host OS through the
netdevice that represents the VF they were sent from.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 4 +
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 85 ++++++++++++++++++++++
2 files changed, 89 insertions(+)
From: Hadar Hen Zion <redacted>
Introduce E-Switch registration/unregister representors functions.
Those functions are called by the mlx5e driver when the PF NIC is
created upon pci probe action regardless of the E-Switch mode (NONE,
LEGACY or OFFLOADS).
Adding basic E-Switch database that will hold the vport represntors
upon creation.
This patch doesn't add any new functionality.
Signed-off-by: Hadar Hen Zion <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 3 +-
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 60 +++++++++++++++++++---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 10 ++++
drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 12 +++++
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 19 +++++++
5 files changed, 97 insertions(+), 7 deletions(-)
From: Sergei Shtylyov <hidden> Date: 2016-06-27 16:53:21
Hello.
On 06/27/2016 07:07 PM, Saeed Mahameed wrote:
From: Or Gerlitz <redacted>
In the sriov offloads mode, packets that are not matched by any other
rule should be sent towards the e-switch manager for further processing.
Add such "miss" rule which matches ANY packet as the last rule in the
e-switch FDB and programs the HW to send the packet to vport 0 where
the e-switch manager runs.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-28 05:58:20
On 16-06-27 09:07 AM, Saeed Mahameed wrote:
From: Or Gerlitz <redacted>
Add the commands to set and show the mode of SRIOV E-Switch,
two modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows based) set by the host OS
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
Hi,
Nice work overall also I really appreciated that the core networking
interfaces appear to able to support this without any change.
On this patch though do we really need modes like this? My concern with
modes is two fold. One its another knob that some controller will have
to get right which I would prefer to avoid. And two I suspect switching
between the two modes flushes the tables or leaves them in some
unexpected state? At least I can't figure out what the expected should
be off-hand.
Could we instead continue to use the "legacy" mode by default by just
populating the fdb table correctly and then if users want to enable
the "offloads" mode they can modify the fdb tables by deleting entries
or adding them or just extending the dmac/vf mapping via 'tc'. This
would seem natural to me. The flooding rules in fdb might need to be
exposed a bit more cleanly to get the right default flooding behavior
etc. But to me at least this would be much cleaner. Everything will be
nicely defined and we wont have issues with drivers doing slightly
and subtle different defaults between legacy/offload and the transitions
between the states or on resets or etc. If users need to discover the
current configuration then they just query fdb, query tc, and the state
is known no need for any magic toggle switch as best I can see.
Otherwise I didn't review the mlx code but read the commit msgs and
it looks good. I'll take a closer look in the morning.
Thanks,
John
From: Or Gerlitz <hidden> Date: 2016-06-28 12:00:14
On 6/28/2016 8:57 AM, John Fastabend wrote:
On 16-06-27 09:07 AM, Saeed Mahameed wrote:
quoted
Add the commands to set and show the mode of SRIOV E-Switch, two modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows based) set by the host OS
Nice work overall also I really appreciated that the core networking
interfaces appear to able to support this without any change.
thanks..
quoted
On this patch though do we really need modes like this? My concern with
modes is two fold. One its another knob that some controller will have
to get right which I would prefer to avoid. And two I suspect switching
between the two modes flushes the tables or leaves them in some
unexpected state? At least I can't figure out what the expected should
be off-hand.
Re the 1st concern (another knob), I think we do want that, see below
Re the 2nd concern, I will re-read the cover letter and change logs and
if needed clarify/improve: the transition is clean! When you are moving
from legacy to offloads or the other way around, nothing is left in
unexpected state, all HW forwarding tables as filled by the current
mode are flushed and next they are set as needed for the new mode.
quoted
Could we instead continue to use the "legacy" mode by default by just
populating the fdb table correctly and then if users want to enable
the "offloads" mode they can modify the fdb tables by deleting entries
or adding them or just extending the dmac/vf mapping via 'tc'. This
would seem natural to me. The flooding rules in fdb might need to be
exposed a bit more cleanly to get the right default flooding behavior
etc. But to me at least this would be much cleaner. Everything will be
nicely defined and we wont have issues with drivers doing slightly
and subtle different defaults between legacy/offload and the transitions
between the states or on resets or etc. If users need to discover the
current configuration then they just query fdb, query tc, and the state
is known no need for any magic toggle switch as best I can see.
Few comments here:
Each mode has it's own way of the driver doing setup for the HW tables
and how population of the HW tables is done.
The offloads mode needs to create a black hole miss rule and
send-to-vport rules and create the tables so they can contain later
rules set by the kernel in a way which is HW/driver dependent.
The legacy mode creates the tables differently and populates them later
with rule set by
the driver and not the kernel.
Even if we put the different table setup issue a side, I don't think it
would be correct for bridge/tc to remove rules they didn't add, which is
needed under your proposal when moving from legacy type rules to
offloads mode. Querying is problematic too, since legacy could (and
does) involve some default rules set by the FW, e.g that deals with
outer world (== not belonging to VM on this host) MACs which are
invisible to the driver.
That legacy was here and we can't avoid handling it properly for which
this knob is needed. Note that a vendor can choose to put their default
to be offloads, hopefully over time, we will all go there :)
quoted
Otherwise I didn't review the mlx code but read the commit msgs and
it looks good. I'll take a closer look in the morning.
Mon, Jun 27, 2016 at 06:07:21PM CEST, saeedm@mellanox.com wrote:
From: Or Gerlitz <redacted>
Add the commands to set and show the mode of SRIOV E-Switch,
two modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows based) set by the host OS
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
Acked-by: Jiri Pirko <redacted>
Looks fine to me. Usable for many drivers of devices containing embedded
switch. We need this for clean transition from legacy handling of embedded
switches we have in drivers currently to new switchdev model.
Thanks!
From: Andy Gospodarek <hidden> Date: 2016-06-28 13:42:17
On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote:
From: Or Gerlitz <redacted>
Implement handlers for the devlink commands to get and set the SRIOV
E-Switch mode.
When turning to the offloads mode, we disable the e-switch and enable
it again in the new mode, create the NIC offloads table and create VF reps.
When turning to legacy mode, we remove the VF reps and the offloads
table, and re-initiate the e-switch in it's legacy mode.
The actual creation/removal of the VF reps is done in downstream patches.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 12 ++-
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 ++++++++++++++++++++-
2 files changed, 105 insertions(+), 9 deletions(-)
int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode)
{
- return -EOPNOTSUPP;
+ struct mlx5_core_dev *dev;
+ u16 cur_mode;
+
+ dev = devlink_priv(devlink);
+
+ if (!MLX5_CAP_GEN(dev, vport_group_manager))
+ return -EOPNOTSUPP;
+
+ cur_mode = dev->priv.eswitch->mode;
+
+ if (cur_mode == SRIOV_NONE || mode == SRIOV_NONE)
+ return -EOPNOTSUPP;
+
+ if (cur_mode == mode)
+ return 0;
+
+ if (mode == SRIOV_OFFLOADS) /* current mode is legacy */
+ return esw_offloads_start(dev->priv.eswitch);
+ else if (mode == SRIOV_LEGACY) /* curreny mode is offloads */
+ return esw_offloads_stop(dev->priv.eswitch);
+ else
+ return -EINVAL;
}
int mlx5_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode)
{
- return -EOPNOTSUPP;
+ struct mlx5_core_dev *dev;
+
+ dev = devlink_priv(devlink);
+
+ if (!MLX5_CAP_GEN(dev, vport_group_manager))
+ return -EOPNOTSUPP;
+
+ if (dev->priv.eswitch->mode == SRIOV_NONE)
+ return -EOPNOTSUPP;
+
+ *mode = dev->priv.eswitch->mode;
+
+ return 0;
}
This is an _extremely_ minor nit, but I only bring it up since you are
leading the way here and your model may be one that other people
follow...
Internally you have a enum to track the SRIOV modes:
enum {
SRIOV_NONE,
SRIOV_LEGACY,
SRIOV_OFFLOADS
};
But patch 8 adds a new enum for devlink to track this as well.
enum devlink_eswitch_mode {
DEVLINK_ESWITCH_MODE_NONE,
DEVLINK_ESWITCH_MODE_LEGACY,
DEVLINK_ESWITCH_MODE_OFFLOADS,
};
Would it make sense at some point to use the devlink modes in the driver
so it's less to track?
Again, this is an extremely _minor_ concern. The rest of the set looks
great and I like the architectural decisions made here. Awesome work
all around!
From: Or Gerlitz <hidden> Date: 2016-06-28 14:25:22
On 6/28/2016 4:42 PM, Andy Gospodarek wrote:
On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote:
quoted
From: Or Gerlitz <redacted>
Implement handlers for the devlink commands to get and set the SRIOV
E-Switch mode.
When turning to the offloads mode, we disable the e-switch and enable
it again in the new mode, create the NIC offloads table and create VF reps.
When turning to legacy mode, we remove the VF reps and the offloads
table, and re-initiate the e-switch in it's legacy mode.
The actual creation/removal of the VF reps is done in downstream patches.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 12 ++-
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 ++++++++++++++++++++-
2 files changed, 105 insertions(+), 9 deletions(-)
int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode)
{
- return -EOPNOTSUPP;
+ struct mlx5_core_dev *dev;
+ u16 cur_mode;
+
+ dev = devlink_priv(devlink);
+
+ if (!MLX5_CAP_GEN(dev, vport_group_manager))
+ return -EOPNOTSUPP;
+
+ cur_mode = dev->priv.eswitch->mode;
+
+ if (cur_mode == SRIOV_NONE || mode == SRIOV_NONE)
+ return -EOPNOTSUPP;
+
+ if (cur_mode == mode)
+ return 0;
+
+ if (mode == SRIOV_OFFLOADS) /* current mode is legacy */
+ return esw_offloads_start(dev->priv.eswitch);
+ else if (mode == SRIOV_LEGACY) /* curreny mode is offloads */
+ return esw_offloads_stop(dev->priv.eswitch);
+ else
+ return -EINVAL;
}
This is an _extremely_ minor nit, but I only bring it up since you are
leading the way here and your model may be one that other people
follow...
Internally you have a enum to track the SRIOV modes:
enum {
SRIOV_NONE,
SRIOV_LEGACY,
SRIOV_OFFLOADS
};
But patch 8 adds a new enum for devlink to track this as well.
enum devlink_eswitch_mode {
DEVLINK_ESWITCH_MODE_NONE,
DEVLINK_ESWITCH_MODE_LEGACY,
DEVLINK_ESWITCH_MODE_OFFLOADS,
};
Andy,
In mlx5 we're having an eswitch driver instance also when not in sriov
mode where on that case the mlx5 eswitch mode is called sriov_none,
which is maybe not a very successful name, I'll look on that.
On the devlink/system level, the eswitch modes are relevant only for
SRIOV, you can see in the mlx5 set function that we return error when in
the none mode or asked to go there.
So... with your comment, I realize now that I forgot to remove
DEVLINK_ESWITCH_MODE_NONE value from the submission.
Would it make sense at some point to use the devlink modes in the driver
so it's less to track?
This makes it a bit problematic for mlx5 to use the
DEVLINK_ESWITCH_MODE_YYY values internally.
Again, this is an extremely _minor_ concern. The rest of the set looks
great and I like the architectural decisions made here. Awesome work
all around!
From: Andy Gospodarek <hidden> Date: 2016-06-28 14:49:34
On Tue, Jun 28, 2016 at 05:25:11PM +0300, Or Gerlitz wrote:
On 6/28/2016 4:42 PM, Andy Gospodarek wrote:
quoted
On Mon, Jun 27, 2016 at 07:07:23PM +0300, Saeed Mahameed wrote:
quoted
From: Or Gerlitz <redacted>
Implement handlers for the devlink commands to get and set the SRIOV
E-Switch mode.
When turning to the offloads mode, we disable the e-switch and enable
it again in the new mode, create the NIC offloads table and create VF reps.
When turning to legacy mode, we remove the VF reps and the offloads
table, and re-initiate the e-switch in it's legacy mode.
The actual creation/removal of the VF reps is done in downstream patches.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 12 ++-
.../ethernet/mellanox/mlx5/core/eswitch_offloads.c | 102 ++++++++++++++++++++-
2 files changed, 105 insertions(+), 9 deletions(-)
int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode)
{
- return -EOPNOTSUPP;
+ struct mlx5_core_dev *dev;
+ u16 cur_mode;
+
+ dev = devlink_priv(devlink);
+
+ if (!MLX5_CAP_GEN(dev, vport_group_manager))
+ return -EOPNOTSUPP;
+
+ cur_mode = dev->priv.eswitch->mode;
+
+ if (cur_mode == SRIOV_NONE || mode == SRIOV_NONE)
+ return -EOPNOTSUPP;
+
+ if (cur_mode == mode)
+ return 0;
+
+ if (mode == SRIOV_OFFLOADS) /* current mode is legacy */
+ return esw_offloads_start(dev->priv.eswitch);
+ else if (mode == SRIOV_LEGACY) /* curreny mode is offloads */
+ return esw_offloads_stop(dev->priv.eswitch);
+ else
+ return -EINVAL;
}
This is an _extremely_ minor nit, but I only bring it up since you are
leading the way here and your model may be one that other people
follow...
Internally you have a enum to track the SRIOV modes:
enum {
SRIOV_NONE,
SRIOV_LEGACY,
SRIOV_OFFLOADS
};
But patch 8 adds a new enum for devlink to track this as well.
enum devlink_eswitch_mode {
DEVLINK_ESWITCH_MODE_NONE,
DEVLINK_ESWITCH_MODE_LEGACY,
DEVLINK_ESWITCH_MODE_OFFLOADS,
};
Andy,
In mlx5 we're having an eswitch driver instance also when not in sriov mode
where on that case the mlx5 eswitch mode is called sriov_none, which is
maybe not a very successful name, I'll look on that.
On the devlink/system level, the eswitch modes are relevant only for SRIOV,
you can see in the mlx5 set function that we return error when in the none
mode or asked to go there.
So... with your comment, I realize now that I forgot to remove
DEVLINK_ESWITCH_MODE_NONE value from the submission.
quoted
Would it make sense at some point to use the devlink modes in the driver
so it's less to track?
This makes it a bit problematic for mlx5 to use the DEVLINK_ESWITCH_MODE_YYY
values internally.
If you planned to remove DEVLINK_ESWITCH_MODE_NONE then I could see how
using these in mlx5 would be problematic. Thinking about it for just a
minute, I can see the value dropping DEVLINK_ESWITCH_MODE_NONE. If the
driver supports the ability to set the eswitch mode, then it should
report an actual mode other than none.
If you remove DEVLINK_ESWITCH_MODE_NONE, then obviously
mlx5_devlink_eswitch_mode_set/get will need to change a bit as well as
there will need to be a mapping between the two values since the enums
would no longer be the same. Easy fix, though. :-)
quoted
Again, this is an extremely _minor_ concern. The rest of the set looks
great and I like the architectural decisions made here. Awesome work
all around!
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-28 16:20:12
On 16-06-28 03:25 AM, Or Gerlitz wrote:
On 6/28/2016 8:57 AM, John Fastabend wrote:
quoted
On 16-06-27 09:07 AM, Saeed Mahameed wrote:
quoted
Add the commands to set and show the mode of SRIOV E-Switch, two
modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows
based) set by the host OS
Nice work overall also I really appreciated that the core networking
interfaces appear to able to support this without any change.
thanks..
quoted
quoted
On this patch though do we really need modes like this? My concern with
modes is two fold. One its another knob that some controller will have
to get right which I would prefer to avoid. And two I suspect switching
between the two modes flushes the tables or leaves them in some
unexpected state? At least I can't figure out what the expected should
be off-hand.
Re the 1st concern (another knob), I think we do want that, see below
Re the 2nd concern, I will re-read the cover letter and change logs and
if needed clarify/improve: the transition is clean! When you are moving
from legacy to offloads or the other way around, nothing is left in
unexpected state, all HW forwarding tables as filled by the current
mode are flushed and next they are set as needed for the new mode.
OK if I had read the entire patch series maybe I would have caught this
:)
quoted
quoted
Could we instead continue to use the "legacy" mode by default by just
populating the fdb table correctly and then if users want to enable
the "offloads" mode they can modify the fdb tables by deleting entries
or adding them or just extending the dmac/vf mapping via 'tc'. This
would seem natural to me. The flooding rules in fdb might need to be
exposed a bit more cleanly to get the right default flooding behavior
etc. But to me at least this would be much cleaner. Everything will be
nicely defined and we wont have issues with drivers doing slightly
and subtle different defaults between legacy/offload and the transitions
between the states or on resets or etc. If users need to discover the
current configuration then they just query fdb, query tc, and the state
is known no need for any magic toggle switch as best I can see.
Few comments here:
Each mode has it's own way of the driver doing setup for the HW tables
and how population of the HW tables is done.
hmm so in the hardware I have there is actually a l2 table and various
other tables so I don't have any issue with doing table setup. I would
like to see a table_create/table_delete/table_show devlink commands at
some point though but I'm not there yet. This would allow users to
optimize the table slices if they cared to. But that is future work
IMO. Certainly not needed in this series at least. If you want I can
show you a patch I had for this against rocker but it was before devlink
so it would need some porting.
The offloads mode needs to create a black hole miss rule and
send-to-vport rules and create the tables so they can contain later
rules set by the kernel in a way which is HW/driver dependent.
Agreed a black hole miss rule needs to be applied but rather than apply
it automatically with some toggle I would prefer to just add a 'tc' rule
for this. Or alternatively it can be added by configuring flooding
ports so that only a single port is in the flooding mode. This could
all be done via 'bridge fdb ...' and 'bridge link ...' today I believe.
Then the user defines the state and not the driver writer. It really is
cleaner in my opinion.
One oddball case I have is if I have two PF functions behind a single
network facing port. Yes its a bit strange but in this case its nice to
pick which host facing PF to flood on vs the driver picking one.
And send-to-vport rules I'm not entirely clear on what these actually
are used for. Is this a rule to match packets sent from a VF representer
netdev to the actual VF pcie device? If this is the case its seems to
me that any packet sent on a VF representer should be sent to the VF
directly and these rules can be created when the VF is created. Or did
you mean some other rule by this?
The legacy mode creates the tables differently and populates them later
with rule set by
the driver and not the kernel.
Even if we put the different table setup issue a side, I don't think it
would be correct for bridge/tc to remove rules they didn't add, which is
needed under your proposal when moving from legacy type rules to
offloads mode. Querying is problematic too, since legacy could (and
does) involve some default rules set by the FW, e.g that deals with
outer world (== not belonging to VM on this host) MACs which are
invisible to the driver.
But even legacy mode should report the correct fdb table and setup.
I don't think querying should be a problem if the driver reports the
configuration correctly. This allows us visibility into the driver
default case so we don't have to guess what driver X writer implemented.
That legacy was here and we can't avoid handling it properly for which
this knob is needed. Note that a vendor can choose to put their default
to be offloads, hopefully over time, we will all go there :)
But you can come up in legacy mode and report it via the existing
mechanisms 'tc', 'bridge', etc. and then users can transition to any
mode they like using the tools.
I really don't think the switch here is necessary if you implement the
bridge hooks and tc hooks. cls_u32 can handle this for example and I
would expect flower can as well if you want to do mgmt via flow based
tc commands. And the bridge tool has the attributes for per port
flooding but not sure off-hand if its packed into the msg sent to the
driver. But we could fix that fairly easily in another patch series if
needed.
quoted
quoted
Otherwise I didn't review the mlx code but read the commit msgs and
it looks good. I'll take a closer look in the morning.
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-28 17:19:43
On 16-06-28 09:19 AM, John Fastabend wrote:
On 16-06-28 03:25 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 8:57 AM, John Fastabend wrote:
quoted
On 16-06-27 09:07 AM, Saeed Mahameed wrote:
quoted
Add the commands to set and show the mode of SRIOV E-Switch, two
modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows
based) set by the host OS
Nice work overall also I really appreciated that the core networking
interfaces appear to able to support this without any change.
thanks..
quoted
quoted
On this patch though do we really need modes like this? My concern with
modes is two fold. One its another knob that some controller will have
to get right which I would prefer to avoid. And two I suspect switching
between the two modes flushes the tables or leaves them in some
unexpected state? At least I can't figure out what the expected should
be off-hand.
Re the 1st concern (another knob), I think we do want that, see below
Re the 2nd concern, I will re-read the cover letter and change logs and
if needed clarify/improve: the transition is clean! When you are moving
from legacy to offloads or the other way around, nothing is left in
unexpected state, all HW forwarding tables as filled by the current
mode are flushed and next they are set as needed for the new mode.
OK if I had read the entire patch series maybe I would have caught this
:)
quoted
quoted
quoted
Could we instead continue to use the "legacy" mode by default by just
populating the fdb table correctly and then if users want to enable
the "offloads" mode they can modify the fdb tables by deleting entries
or adding them or just extending the dmac/vf mapping via 'tc'. This
would seem natural to me. The flooding rules in fdb might need to be
exposed a bit more cleanly to get the right default flooding behavior
etc. But to me at least this would be much cleaner. Everything will be
nicely defined and we wont have issues with drivers doing slightly
and subtle different defaults between legacy/offload and the transitions
between the states or on resets or etc. If users need to discover the
current configuration then they just query fdb, query tc, and the state
is known no need for any magic toggle switch as best I can see.
Few comments here:
Each mode has it's own way of the driver doing setup for the HW tables
and how population of the HW tables is done.
hmm so in the hardware I have there is actually a l2 table and various
other tables so I don't have any issue with doing table setup. I would
like to see a table_create/table_delete/table_show devlink commands at
some point though but I'm not there yet. This would allow users to
optimize the table slices if they cared to. But that is future work
IMO. Certainly not needed in this series at least. If you want I can
show you a patch I had for this against rocker but it was before devlink
so it would need some porting.
quoted
The offloads mode needs to create a black hole miss rule and
send-to-vport rules and create the tables so they can contain later
rules set by the kernel in a way which is HW/driver dependent.
Agreed a black hole miss rule needs to be applied but rather than apply
it automatically with some toggle I would prefer to just add a 'tc' rule
for this. Or alternatively it can be added by configuring flooding
ports so that only a single port is in the flooding mode. This could
all be done via 'bridge fdb ...' and 'bridge link ...' today I believe.
Then the user defines the state and not the driver writer. It really is
cleaner in my opinion.
One oddball case I have is if I have two PF functions behind a single
network facing port. Yes its a bit strange but in this case its nice to
pick which host facing PF to flood on vs the driver picking one.
And send-to-vport rules I'm not entirely clear on what these actually
are used for. Is this a rule to match packets sent from a VF representer
netdev to the actual VF pcie device? If this is the case its seems to
me that any packet sent on a VF representer should be sent to the VF
directly and these rules can be created when the VF is created. Or did
you mean some other rule by this?
quoted
The legacy mode creates the tables differently and populates them later
with rule set by
the driver and not the kernel.
Even if we put the different table setup issue a side, I don't think it
would be correct for bridge/tc to remove rules they didn't add, which is
needed under your proposal when moving from legacy type rules to
offloads mode. Querying is problematic too, since legacy could (and
does) involve some default rules set by the FW, e.g that deals with
outer world (== not belonging to VM on this host) MACs which are
invisible to the driver.
But even legacy mode should report the correct fdb table and setup.
I don't think querying should be a problem if the driver reports the
configuration correctly. This allows us visibility into the driver
default case so we don't have to guess what driver X writer implemented.
quoted
That legacy was here and we can't avoid handling it properly for which
this knob is needed. Note that a vendor can choose to put their default
to be offloads, hopefully over time, we will all go there :)
But you can come up in legacy mode and report it via the existing
mechanisms 'tc', 'bridge', etc. and then users can transition to any
mode they like using the tools.
I really don't think the switch here is necessary if you implement the
bridge hooks and tc hooks. cls_u32 can handle this for example and I
would expect flower can as well if you want to do mgmt via flow based
tc commands. And the bridge tool has the attributes for per port
flooding but not sure off-hand if its packed into the msg sent to the
driver. But we could fix that fairly easily in another patch series if
needed.
Actually with a bit more thought it might be nice to have a
flag to enable/disable creation of vf netdev representer in case it
somehow causes issues with existing software. We typically
enable/disable features with ethtool feature flags though not via
devlink so I think it would fit better as an ethtool flag same as
all the other hardware features.
The above points in the last mail are more about how it influences the
forwarding rules in the switch and my preference would be that it
doesn't change how the forwarding works in the switch and instead the
forwarding state is managed via standard tools 'tc', 'bridge', etc. So
I think my comments are still relevant. However as long as when we
query the nic/switch we get the correct information back in any mode
I'm not too concerned I suspect any software that actually uses this
will have to query and reconfigure either way counting on driver
writers to get policy correct is not a stable way to write usermode
software.
All that said I don't plan to change the forwarding state this way
with the intel drivers when implementing the vf representer.
Yet another reason not to change the state of the forwarding rules is
even on older hardware that only supports l2 mac/vlan based forwarding
having a VF representer is useful to configure the device and send/recv
some basic control packets (e.g. lldp). On these devices l2 mac/vlan
mode is the only one supported.
quoted
quoted
quoted
Otherwise I didn't review the mlx code but read the commit msgs and
it looks good. I'll take a closer look in the morning.
Tue, Jun 28, 2016 at 07:19:06PM CEST, john.fastabend@gmail.com wrote:
On 16-06-28 09:19 AM, John Fastabend wrote:
quoted
On 16-06-28 03:25 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 8:57 AM, John Fastabend wrote:
quoted
On 16-06-27 09:07 AM, Saeed Mahameed wrote:
quoted
Add the commands to set and show the mode of SRIOV E-Switch, two
modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows
based) set by the host OS
Nice work overall also I really appreciated that the core networking
interfaces appear to able to support this without any change.
thanks..
quoted
quoted
On this patch though do we really need modes like this? My concern with
modes is two fold. One its another knob that some controller will have
to get right which I would prefer to avoid. And two I suspect switching
between the two modes flushes the tables or leaves them in some
unexpected state? At least I can't figure out what the expected should
be off-hand.
Re the 1st concern (another knob), I think we do want that, see below
Re the 2nd concern, I will re-read the cover letter and change logs and
if needed clarify/improve: the transition is clean! When you are moving
from legacy to offloads or the other way around, nothing is left in
unexpected state, all HW forwarding tables as filled by the current
mode are flushed and next they are set as needed for the new mode.
OK if I had read the entire patch series maybe I would have caught this
:)
quoted
quoted
quoted
Could we instead continue to use the "legacy" mode by default by just
populating the fdb table correctly and then if users want to enable
the "offloads" mode they can modify the fdb tables by deleting entries
or adding them or just extending the dmac/vf mapping via 'tc'. This
would seem natural to me. The flooding rules in fdb might need to be
exposed a bit more cleanly to get the right default flooding behavior
etc. But to me at least this would be much cleaner. Everything will be
nicely defined and we wont have issues with drivers doing slightly
and subtle different defaults between legacy/offload and the transitions
between the states or on resets or etc. If users need to discover the
current configuration then they just query fdb, query tc, and the state
is known no need for any magic toggle switch as best I can see.
Few comments here:
Each mode has it's own way of the driver doing setup for the HW tables
and how population of the HW tables is done.
hmm so in the hardware I have there is actually a l2 table and various
other tables so I don't have any issue with doing table setup. I would
like to see a table_create/table_delete/table_show devlink commands at
some point though but I'm not there yet. This would allow users to
optimize the table slices if they cared to. But that is future work
IMO. Certainly not needed in this series at least. If you want I can
show you a patch I had for this against rocker but it was before devlink
so it would need some porting.
quoted
The offloads mode needs to create a black hole miss rule and
send-to-vport rules and create the tables so they can contain later
rules set by the kernel in a way which is HW/driver dependent.
Agreed a black hole miss rule needs to be applied but rather than apply
it automatically with some toggle I would prefer to just add a 'tc' rule
for this. Or alternatively it can be added by configuring flooding
ports so that only a single port is in the flooding mode. This could
all be done via 'bridge fdb ...' and 'bridge link ...' today I believe.
Then the user defines the state and not the driver writer. It really is
cleaner in my opinion.
One oddball case I have is if I have two PF functions behind a single
network facing port. Yes its a bit strange but in this case its nice to
pick which host facing PF to flood on vs the driver picking one.
And send-to-vport rules I'm not entirely clear on what these actually
are used for. Is this a rule to match packets sent from a VF representer
netdev to the actual VF pcie device? If this is the case its seems to
me that any packet sent on a VF representer should be sent to the VF
directly and these rules can be created when the VF is created. Or did
you mean some other rule by this?
quoted
The legacy mode creates the tables differently and populates them later
with rule set by
the driver and not the kernel.
Even if we put the different table setup issue a side, I don't think it
would be correct for bridge/tc to remove rules they didn't add, which is
needed under your proposal when moving from legacy type rules to
offloads mode. Querying is problematic too, since legacy could (and
does) involve some default rules set by the FW, e.g that deals with
outer world (== not belonging to VM on this host) MACs which are
invisible to the driver.
But even legacy mode should report the correct fdb table and setup.
I don't think querying should be a problem if the driver reports the
configuration correctly. This allows us visibility into the driver
default case so we don't have to guess what driver X writer implemented.
quoted
That legacy was here and we can't avoid handling it properly for which
this knob is needed. Note that a vendor can choose to put their default
to be offloads, hopefully over time, we will all go there :)
But you can come up in legacy mode and report it via the existing
mechanisms 'tc', 'bridge', etc. and then users can transition to any
mode they like using the tools.
I really don't think the switch here is necessary if you implement the
bridge hooks and tc hooks. cls_u32 can handle this for example and I
would expect flower can as well if you want to do mgmt via flow based
tc commands. And the bridge tool has the attributes for per port
flooding but not sure off-hand if its packed into the msg sent to the
driver. But we could fix that fairly easily in another patch series if
needed.
Actually with a bit more thought it might be nice to have a
flag to enable/disable creation of vf netdev representer in case it
somehow causes issues with existing software. We typically
enable/disable features with ethtool feature flags though not via
devlink so I think it would fit better as an ethtool flag same as
all the other hardware features.
This is not a property of a netdevice, but a devlink device. That should
be a handle of creating/not creating representors. And I think that what
this patch is doing serves that purpose as well. For legacy mode, the
representors are not created, for offload/switchdev mode they are
created.
Does not make sense to have this in ethtool one bit to me.
The above points in the last mail are more about how it influences the
forwarding rules in the switch and my preference would be that it
doesn't change how the forwarding works in the switch and instead the
forwarding state is managed via standard tools 'tc', 'bridge', etc. So
I think my comments are still relevant. However as long as when we
query the nic/switch we get the correct information back in any mode
I'm not too concerned I suspect any software that actually uses this
will have to query and reconfigure either way counting on driver
writers to get policy correct is not a stable way to write usermode
software.
All that said I don't plan to change the forwarding state this way
with the intel drivers when implementing the vf representer.
Yet another reason not to change the state of the forwarding rules is
even on older hardware that only supports l2 mac/vlan based forwarding
having a VF representer is useful to configure the device and send/recv
some basic control packets (e.g. lldp). On these devices l2 mac/vlan
mode is the only one supported.
quoted
quoted
quoted
quoted
Otherwise I didn't review the mlx code but read the commit msgs and
it looks good. I'll take a closer look in the morning.
Tue, Jun 28, 2016 at 07:19:06PM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-28 09:19 AM, John Fastabend wrote:
quoted
On 16-06-28 03:25 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 8:57 AM, John Fastabend wrote:
quoted
On 16-06-27 09:07 AM, Saeed Mahameed wrote:
quoted
Add the commands to set and show the mode of SRIOV E-Switch, two
modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows
based) set by the host OS
Nice work overall also I really appreciated that the core networking
interfaces appear to able to support this without any change.
thanks..
quoted
quoted
On this patch though do we really need modes like this? My concern with
modes is two fold. One its another knob that some controller will have
to get right which I would prefer to avoid. And two I suspect switching
between the two modes flushes the tables or leaves them in some
unexpected state? At least I can't figure out what the expected should
be off-hand.
Re the 1st concern (another knob), I think we do want that, see below
Re the 2nd concern, I will re-read the cover letter and change logs and
if needed clarify/improve: the transition is clean! When you are moving
from legacy to offloads or the other way around, nothing is left in
unexpected state, all HW forwarding tables as filled by the current
mode are flushed and next they are set as needed for the new mode.
OK if I had read the entire patch series maybe I would have caught this
:)
quoted
quoted
quoted
Could we instead continue to use the "legacy" mode by default by just
populating the fdb table correctly and then if users want to enable
the "offloads" mode they can modify the fdb tables by deleting entries
or adding them or just extending the dmac/vf mapping via 'tc'. This
would seem natural to me. The flooding rules in fdb might need to be
exposed a bit more cleanly to get the right default flooding behavior
etc. But to me at least this would be much cleaner. Everything will be
nicely defined and we wont have issues with drivers doing slightly
and subtle different defaults between legacy/offload and the transitions
between the states or on resets or etc. If users need to discover the
current configuration then they just query fdb, query tc, and the state
is known no need for any magic toggle switch as best I can see.
Few comments here:
Each mode has it's own way of the driver doing setup for the HW tables
and how population of the HW tables is done.
hmm so in the hardware I have there is actually a l2 table and various
other tables so I don't have any issue with doing table setup. I would
like to see a table_create/table_delete/table_show devlink commands at
some point though but I'm not there yet. This would allow users to
optimize the table slices if they cared to. But that is future work
IMO. Certainly not needed in this series at least. If you want I can
show you a patch I had for this against rocker but it was before devlink
so it would need some porting.
quoted
The offloads mode needs to create a black hole miss rule and
send-to-vport rules and create the tables so they can contain later
rules set by the kernel in a way which is HW/driver dependent.
Agreed a black hole miss rule needs to be applied but rather than apply
it automatically with some toggle I would prefer to just add a 'tc' rule
for this. Or alternatively it can be added by configuring flooding
ports so that only a single port is in the flooding mode. This could
all be done via 'bridge fdb ...' and 'bridge link ...' today I believe.
Then the user defines the state and not the driver writer. It really is
cleaner in my opinion.
One oddball case I have is if I have two PF functions behind a single
network facing port. Yes its a bit strange but in this case its nice to
pick which host facing PF to flood on vs the driver picking one.
And send-to-vport rules I'm not entirely clear on what these actually
are used for. Is this a rule to match packets sent from a VF representer
netdev to the actual VF pcie device? If this is the case its seems to
me that any packet sent on a VF representer should be sent to the VF
directly and these rules can be created when the VF is created. Or did
you mean some other rule by this?
quoted
The legacy mode creates the tables differently and populates them later
with rule set by
the driver and not the kernel.
Even if we put the different table setup issue a side, I don't think it
would be correct for bridge/tc to remove rules they didn't add, which is
needed under your proposal when moving from legacy type rules to
offloads mode. Querying is problematic too, since legacy could (and
does) involve some default rules set by the FW, e.g that deals with
outer world (== not belonging to VM on this host) MACs which are
invisible to the driver.
But even legacy mode should report the correct fdb table and setup.
I don't think querying should be a problem if the driver reports the
configuration correctly. This allows us visibility into the driver
default case so we don't have to guess what driver X writer implemented.
quoted
That legacy was here and we can't avoid handling it properly for which
this knob is needed. Note that a vendor can choose to put their default
to be offloads, hopefully over time, we will all go there :)
But you can come up in legacy mode and report it via the existing
mechanisms 'tc', 'bridge', etc. and then users can transition to any
mode they like using the tools.
I really don't think the switch here is necessary if you implement the
bridge hooks and tc hooks. cls_u32 can handle this for example and I
would expect flower can as well if you want to do mgmt via flow based
tc commands. And the bridge tool has the attributes for per port
flooding but not sure off-hand if its packed into the msg sent to the
driver. But we could fix that fairly easily in another patch series if
needed.
Actually with a bit more thought it might be nice to have a
flag to enable/disable creation of vf netdev representer in case it
somehow causes issues with existing software. We typically
enable/disable features with ethtool feature flags though not via
devlink so I think it would fit better as an ethtool flag same as
all the other hardware features.
This is not a property of a netdevice, but a devlink device. That should
be a handle of creating/not creating representors. And I think that what
this patch is doing serves that purpose as well. For legacy mode, the
representors are not created, for offload/switchdev mode they are
created.
Even in legacy mode, i think there is a value in creating VP representor
netdevs.
We are planning to expose VF statistics, ntuple filters, additional
fdb, vlan entries via this
netdev for VFs in the default mode.
Isn't it possible to switch to offloads mode by deleting the 'legacy'
flow rules and
adding 'offloads' flow rules from userspace?
Does not make sense to have this in ethtool one bit to me.
quoted
The above points in the last mail are more about how it influences the
forwarding rules in the switch and my preference would be that it
doesn't change how the forwarding works in the switch and instead the
forwarding state is managed via standard tools 'tc', 'bridge', etc. So
I think my comments are still relevant. However as long as when we
query the nic/switch we get the correct information back in any mode
I'm not too concerned I suspect any software that actually uses this
will have to query and reconfigure either way counting on driver
writers to get policy correct is not a stable way to write usermode
software.
All that said I don't plan to change the forwarding state this way
with the intel drivers when implementing the vf representer.
Yet another reason not to change the state of the forwarding rules is
even on older hardware that only supports l2 mac/vlan based forwarding
having a VF representer is useful to configure the device and send/recv
some basic control packets (e.g. lldp). On these devices l2 mac/vlan
mode is the only one supported.
quoted
quoted
quoted
quoted
Otherwise I didn't review the mlx code but read the commit msgs and
it looks good. I'll take a closer look in the morning.
Tue, Jun 28, 2016 at 09:04:00PM CEST, sridhar.samudrala@intel.com wrote:
On 6/28/2016 11:46 AM, Jiri Pirko wrote:
quoted
Tue, Jun 28, 2016 at 07:19:06PM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-28 09:19 AM, John Fastabend wrote:
quoted
On 16-06-28 03:25 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 8:57 AM, John Fastabend wrote:
quoted
On 16-06-27 09:07 AM, Saeed Mahameed wrote:
quoted
Add the commands to set and show the mode of SRIOV E-Switch, two
modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows
based) set by the host OS
Nice work overall also I really appreciated that the core networking
interfaces appear to able to support this without any change.
thanks..
quoted
quoted
On this patch though do we really need modes like this? My concern with
modes is two fold. One its another knob that some controller will have
to get right which I would prefer to avoid. And two I suspect switching
between the two modes flushes the tables or leaves them in some
unexpected state? At least I can't figure out what the expected should
be off-hand.
Re the 1st concern (another knob), I think we do want that, see below
Re the 2nd concern, I will re-read the cover letter and change logs and
if needed clarify/improve: the transition is clean! When you are moving
from legacy to offloads or the other way around, nothing is left in
unexpected state, all HW forwarding tables as filled by the current
mode are flushed and next they are set as needed for the new mode.
OK if I had read the entire patch series maybe I would have caught this
:)
quoted
quoted
quoted
Could we instead continue to use the "legacy" mode by default by just
populating the fdb table correctly and then if users want to enable
the "offloads" mode they can modify the fdb tables by deleting entries
or adding them or just extending the dmac/vf mapping via 'tc'. This
would seem natural to me. The flooding rules in fdb might need to be
exposed a bit more cleanly to get the right default flooding behavior
etc. But to me at least this would be much cleaner. Everything will be
nicely defined and we wont have issues with drivers doing slightly
and subtle different defaults between legacy/offload and the transitions
between the states or on resets or etc. If users need to discover the
current configuration then they just query fdb, query tc, and the state
is known no need for any magic toggle switch as best I can see.
Few comments here:
Each mode has it's own way of the driver doing setup for the HW tables
and how population of the HW tables is done.
hmm so in the hardware I have there is actually a l2 table and various
other tables so I don't have any issue with doing table setup. I would
like to see a table_create/table_delete/table_show devlink commands at
some point though but I'm not there yet. This would allow users to
optimize the table slices if they cared to. But that is future work
IMO. Certainly not needed in this series at least. If you want I can
show you a patch I had for this against rocker but it was before devlink
so it would need some porting.
quoted
The offloads mode needs to create a black hole miss rule and
send-to-vport rules and create the tables so they can contain later
rules set by the kernel in a way which is HW/driver dependent.
Agreed a black hole miss rule needs to be applied but rather than apply
it automatically with some toggle I would prefer to just add a 'tc' rule
for this. Or alternatively it can be added by configuring flooding
ports so that only a single port is in the flooding mode. This could
all be done via 'bridge fdb ...' and 'bridge link ...' today I believe.
Then the user defines the state and not the driver writer. It really is
cleaner in my opinion.
One oddball case I have is if I have two PF functions behind a single
network facing port. Yes its a bit strange but in this case its nice to
pick which host facing PF to flood on vs the driver picking one.
And send-to-vport rules I'm not entirely clear on what these actually
are used for. Is this a rule to match packets sent from a VF representer
netdev to the actual VF pcie device? If this is the case its seems to
me that any packet sent on a VF representer should be sent to the VF
directly and these rules can be created when the VF is created. Or did
you mean some other rule by this?
quoted
The legacy mode creates the tables differently and populates them later
with rule set by
the driver and not the kernel.
Even if we put the different table setup issue a side, I don't think it
would be correct for bridge/tc to remove rules they didn't add, which is
needed under your proposal when moving from legacy type rules to
offloads mode. Querying is problematic too, since legacy could (and
does) involve some default rules set by the FW, e.g that deals with
outer world (== not belonging to VM on this host) MACs which are
invisible to the driver.
But even legacy mode should report the correct fdb table and setup.
I don't think querying should be a problem if the driver reports the
configuration correctly. This allows us visibility into the driver
default case so we don't have to guess what driver X writer implemented.
quoted
That legacy was here and we can't avoid handling it properly for which
this knob is needed. Note that a vendor can choose to put their default
to be offloads, hopefully over time, we will all go there :)
But you can come up in legacy mode and report it via the existing
mechanisms 'tc', 'bridge', etc. and then users can transition to any
mode they like using the tools.
I really don't think the switch here is necessary if you implement the
bridge hooks and tc hooks. cls_u32 can handle this for example and I
would expect flower can as well if you want to do mgmt via flow based
tc commands. And the bridge tool has the attributes for per port
flooding but not sure off-hand if its packed into the msg sent to the
driver. But we could fix that fairly easily in another patch series if
needed.
Actually with a bit more thought it might be nice to have a
flag to enable/disable creation of vf netdev representer in case it
somehow causes issues with existing software. We typically
enable/disable features with ethtool feature flags though not via
devlink so I think it would fit better as an ethtool flag same as
all the other hardware features.
This is not a property of a netdevice, but a devlink device. That should
be a handle of creating/not creating representors. And I think that what
this patch is doing serves that purpose as well. For legacy mode, the
representors are not created, for offload/switchdev mode they are
created.
Even in legacy mode, i think there is a value in creating VP representor
netdevs.
Why?! Please, leave legacy be legacy. Use the new mode for implementing new
features. Don't make things any more complicated :(
We are planning to expose VF statistics, ntuple filters, additional fdb,
vlan entries via this
netdev for VFs in the default mode.
Isn't it possible to switch to offloads mode by deleting the 'legacy' flow
rules and
adding 'offloads' flow rules from userspace?
quoted
Does not make sense to have this in ethtool one bit to me.
quoted
The above points in the last mail are more about how it influences the
forwarding rules in the switch and my preference would be that it
doesn't change how the forwarding works in the switch and instead the
forwarding state is managed via standard tools 'tc', 'bridge', etc. So
I think my comments are still relevant. However as long as when we
query the nic/switch we get the correct information back in any mode
I'm not too concerned I suspect any software that actually uses this
will have to query and reconfigure either way counting on driver
writers to get policy correct is not a stable way to write usermode
software.
All that said I don't plan to change the forwarding state this way
with the intel drivers when implementing the vf representer.
Yet another reason not to change the state of the forwarding rules is
even on older hardware that only supports l2 mac/vlan based forwarding
having a VF representer is useful to configure the device and send/recv
some basic control packets (e.g. lldp). On these devices l2 mac/vlan
mode is the only one supported.
quoted
quoted
quoted
quoted
Otherwise I didn't review the mlx code but read the commit msgs and
it looks good. I'll take a closer look in the morning.
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-28 19:31:52
On 16-06-28 12:12 PM, Jiri Pirko wrote:
Tue, Jun 28, 2016 at 09:04:00PM CEST, sridhar.samudrala@intel.com wrote:
quoted
On 6/28/2016 11:46 AM, Jiri Pirko wrote:
quoted
Tue, Jun 28, 2016 at 07:19:06PM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-28 09:19 AM, John Fastabend wrote:
quoted
On 16-06-28 03:25 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 8:57 AM, John Fastabend wrote:
quoted
On 16-06-27 09:07 AM, Saeed Mahameed wrote:
quoted
Add the commands to set and show the mode of SRIOV E-Switch, two
modes are supported:
* legacy : operating in the "old" L2 based mode (DMAC --> VF vport)
* offloads : offloading SW rules/policy (e.g Bridge/FDB or TC/Flows
based) set by the host OS
Nice work overall also I really appreciated that the core networking
interfaces appear to able to support this without any change.
thanks..
quoted
quoted
On this patch though do we really need modes like this? My concern with
modes is two fold. One its another knob that some controller will have
to get right which I would prefer to avoid. And two I suspect switching
between the two modes flushes the tables or leaves them in some
unexpected state? At least I can't figure out what the expected should
be off-hand.
Re the 1st concern (another knob), I think we do want that, see below
Re the 2nd concern, I will re-read the cover letter and change logs and
if needed clarify/improve: the transition is clean! When you are moving
from legacy to offloads or the other way around, nothing is left in
unexpected state, all HW forwarding tables as filled by the current
mode are flushed and next they are set as needed for the new mode.
OK if I had read the entire patch series maybe I would have caught this
:)
quoted
quoted
quoted
Could we instead continue to use the "legacy" mode by default by just
populating the fdb table correctly and then if users want to enable
the "offloads" mode they can modify the fdb tables by deleting entries
or adding them or just extending the dmac/vf mapping via 'tc'. This
would seem natural to me. The flooding rules in fdb might need to be
exposed a bit more cleanly to get the right default flooding behavior
etc. But to me at least this would be much cleaner. Everything will be
nicely defined and we wont have issues with drivers doing slightly
and subtle different defaults between legacy/offload and the transitions
between the states or on resets or etc. If users need to discover the
current configuration then they just query fdb, query tc, and the state
is known no need for any magic toggle switch as best I can see.
Few comments here:
Each mode has it's own way of the driver doing setup for the HW tables
and how population of the HW tables is done.
hmm so in the hardware I have there is actually a l2 table and various
other tables so I don't have any issue with doing table setup. I would
like to see a table_create/table_delete/table_show devlink commands at
some point though but I'm not there yet. This would allow users to
optimize the table slices if they cared to. But that is future work
IMO. Certainly not needed in this series at least. If you want I can
show you a patch I had for this against rocker but it was before devlink
so it would need some porting.
quoted
The offloads mode needs to create a black hole miss rule and
send-to-vport rules and create the tables so they can contain later
rules set by the kernel in a way which is HW/driver dependent.
Agreed a black hole miss rule needs to be applied but rather than apply
it automatically with some toggle I would prefer to just add a 'tc' rule
for this. Or alternatively it can be added by configuring flooding
ports so that only a single port is in the flooding mode. This could
all be done via 'bridge fdb ...' and 'bridge link ...' today I believe.
Then the user defines the state and not the driver writer. It really is
cleaner in my opinion.
One oddball case I have is if I have two PF functions behind a single
network facing port. Yes its a bit strange but in this case its nice to
pick which host facing PF to flood on vs the driver picking one.
And send-to-vport rules I'm not entirely clear on what these actually
are used for. Is this a rule to match packets sent from a VF representer
netdev to the actual VF pcie device? If this is the case its seems to
me that any packet sent on a VF representer should be sent to the VF
directly and these rules can be created when the VF is created. Or did
you mean some other rule by this?
quoted
The legacy mode creates the tables differently and populates them later
with rule set by
the driver and not the kernel.
Even if we put the different table setup issue a side, I don't think it
would be correct for bridge/tc to remove rules they didn't add, which is
needed under your proposal when moving from legacy type rules to
offloads mode. Querying is problematic too, since legacy could (and
does) involve some default rules set by the FW, e.g that deals with
outer world (== not belonging to VM on this host) MACs which are
invisible to the driver.
But even legacy mode should report the correct fdb table and setup.
I don't think querying should be a problem if the driver reports the
configuration correctly. This allows us visibility into the driver
default case so we don't have to guess what driver X writer implemented.
quoted
That legacy was here and we can't avoid handling it properly for which
this knob is needed. Note that a vendor can choose to put their default
to be offloads, hopefully over time, we will all go there :)
But you can come up in legacy mode and report it via the existing
mechanisms 'tc', 'bridge', etc. and then users can transition to any
mode they like using the tools.
I really don't think the switch here is necessary if you implement the
bridge hooks and tc hooks. cls_u32 can handle this for example and I
would expect flower can as well if you want to do mgmt via flow based
tc commands. And the bridge tool has the attributes for per port
flooding but not sure off-hand if its packed into the msg sent to the
driver. But we could fix that fairly easily in another patch series if
needed.
Actually with a bit more thought it might be nice to have a
flag to enable/disable creation of vf netdev representer in case it
somehow causes issues with existing software. We typically
enable/disable features with ethtool feature flags though not via
devlink so I think it would fit better as an ethtool flag same as
all the other hardware features.
This is not a property of a netdevice, but a devlink device. That should
be a handle of creating/not creating representors. And I think that what
this patch is doing serves that purpose as well. For legacy mode, the
representors are not created, for offload/switchdev mode they are
created.
Even in legacy mode, i think there is a value in creating VP representor
netdevs.
Why?! Please, leave legacy be legacy. Use the new mode for implementing new
features. Don't make things any more complicated :(
OK so how I read this is there are two things going on that are being
conflated together. Creating VF netdev's is linked to the PCIe
subsystems and brings VFs into the netdev model. This is a good thing
but doesn't need to be a global nic policy it can be per port hence
the ethtool flag vs devlink discussion. I don't actually have a use case
to have one port with VF netdevs and another without it so I'm not too
particular on this. Logically it looks like a per port setting because
the hardware has no issues with making one physical function create
a netdev for each of its VFs and the other one run without these
netdevs. This is why I called it out.
How this relates to bridge, tc, etc. is now you have a identifier to
configure instead of using strange 'ip link set ... vf#' commands. This
is great. But I see no reason the hardware has to make changes to
the existing tables or any of this. Before we used 'bridge fdb' and 'ip
link' now we can use bridge tools more effectively and can deprecate
the overloaded use of ip. But again I see no reason to thrash the
forwarding state of the switch because we happen to be adding VFs.
Having a set of fdb rules to forward MAC/Vlan pairs (as we do now)
seems like a perfectly reasonable default. Add with this patch now
when I run 'fdb show' I can see the defaults.
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
quoted
We are planning to expose VF statistics, ntuple filters, additional fdb,
vlan entries via this
netdev for VFs in the default mode.
Isn't it possible to switch to offloads mode by deleting the 'legacy' flow
rules and
adding 'offloads' flow rules from userspace?
quoted
Does not make sense to have this in ethtool one bit to me.
quoted
The above points in the last mail are more about how it influences the
forwarding rules in the switch and my preference would be that it
doesn't change how the forwarding works in the switch and instead the
forwarding state is managed via standard tools 'tc', 'bridge', etc. So
I think my comments are still relevant. However as long as when we
query the nic/switch we get the correct information back in any mode
I'm not too concerned I suspect any software that actually uses this
will have to query and reconfigure either way counting on driver
writers to get policy correct is not a stable way to write usermode
software.
All that said I don't plan to change the forwarding state this way
with the intel drivers when implementing the vf representer.
Yet another reason not to change the state of the forwarding rules is
even on older hardware that only supports l2 mac/vlan based forwarding
having a VF representer is useful to configure the device and send/recv
some basic control packets (e.g. lldp). On these devices l2 mac/vlan
mode is the only one supported.
quoted
quoted
quoted
quoted
Otherwise I didn't review the mlx code but read the commit msgs and
it looks good. I'll take a closer look in the morning.
From: Or Gerlitz <hidden> Date: 2016-06-29 09:44:59
On 6/28/2016 7:19 PM, John Fastabend wrote:
On 16-06-28 03:25 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 8:57 AM, John Fastabend wrote:
hmm so in the hardware I have there is actually a l2 table and various
other tables so I don't have any issue with doing table setup. I would
like to see a table_create/table_delete/table_show devlink commands at
some point though but I'm not there yet. This would allow users to
optimize the table slices if they cared to. But that is future work
IMO. Certainly not needed in this series at least.
Agree that we could do that and agree that we need not do that now, as
was agreed (...) in Seville,we are not yet to the geography (== HW
tables and table graph) advertisement and setup class.
quoted
The offloads mode needs to create a black hole miss rule and
send-to-vport rules and create the tables so they can contain later
rules set by the kernel in a way which is HW/driver dependent.
Agreed a black hole miss rule needs to be applied but rather than apply
it automatically with some toggle I would prefer to just add a 'tc' rule
for this. Or alternatively it can be added by configuring flooding
ports so that only a single port is in the flooding mode. This could
all be done via 'bridge fdb ...' and 'bridge link ...' today I believe.
Then the user defines the state and not the driver writer. It really is
cleaner in my opinion.
The black hole serves for throwing packets arriving from **anywhere**
and not matched to any other HW rule towards the CPU where the e-switch
manager runs. Hence, it would be correct in my opinion to have it set
by the e-switch manager and it means when some API/knob is applied on
PCI device and not network device, so tc and Co will not really serve
nicely for that.
And send-to-vport rules I'm not entirely clear on what these actually
are used for. Is this a rule to match packets sent from a VF representer
netdev to the actual VF pcie device? If this is the case its seems to
me that any packet sent on a VF representer should be sent to the VF
directly and these rules can be created when the VF is created. Or did
you mean some other rule by this?
YES, send-to-vports rule serve for having the functionality which is
described in the cover letter and on the relevant commit/s: doing xmit
on VF rep netdevice always ends up with the packet to arrive the VF PCI
device. We create these HW rules when in the offloads mode per each
VF/rep indeed.
So when a driver SRIOV logic wakes up in offloads mode (hopefully will
happen a lot soon...), they would (1) create VF reps (2) set these
rules, sure. Currently a transition from legacy to offloads is defined
and these two acts are than on the transition, e.g for mlx5 whose
current default is legacy.
Or.
From: Or Gerlitz <hidden> Date: 2016-06-29 14:49:13
On 6/28/2016 10:31 PM, John Fastabend wrote:
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for implementing new features. Don't make things any more complicated :(
OK so how I read this is there are two things going on that are being
conflated together. Creating VF netdev's is linked to the PCIe
subsystems and brings VFs into the netdev model. This is a good thing
but doesn't need to be a global nic policy it can be per port hence
the ethtool flag vs devlink discussion. I don't actually have a use case
to have one port with VF netdevs and another without it so I'm not too
particular on this. Logically it looks like a per port setting because
the hardware has no issues with making one physical function create
a netdev for each of its VFs and the other one run without these
netdevs. This is why I called it out.
How this relates to bridge, tc, etc. is now you have a identifier to
configure instead of using strange 'ip link set ... vf#' commands. This
is great. But I see no reason the hardware has to make changes to
the existing tables or any of this. Before we used 'bridge fdb' and 'ip
link' now we can use bridge tools more effectively and can deprecate
the overloaded use of ip. But again I see no reason to thrash the
forwarding state of the switch because we happen to be adding VFs.
Having a set of fdb rules to forward MAC/Vlan pairs (as we do now)
seems like a perfectly reasonable default. Add with this patch now
when I run 'fdb show' I can see the defaults.
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
John,
I'll try to address here the core questions and arguments you brought.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The new model has few building blocks, and by all means, have the VF
representors is not the full story, which is not magic but rather the
following:
1. VF (vport) representors netdevices + the needed mechanics
(send-to-vport rules that makes xmit on VF rep --> recv on VF)
2. handling HW data-patch misses --> send to CPU or drop
3. ability to offload SW rules (tc/bridge/etc) using VF representors and
ingress qdiscs / bridge fdb rules / switchdev fdb rule, etc
The knob we suggested says that the system is put into a state where
1,2,3 are needed to make it full performance and functional one. This
submission includes parts 1 and 2, so the offloading of SW rules will
done in successive submission which uses TC offloads which are already
upstream (u32 or flower).
So... we're almost in agreement, do you have another name for the knob
that goes beyond creation/deletion of VF reps? maybe that would be it
for making a progress...
Or.
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-29 16:42:35
On 16-06-29 07:48 AM, Or Gerlitz wrote:
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
OK so how I read this is there are two things going on that are being
conflated together. Creating VF netdev's is linked to the PCIe
subsystems and brings VFs into the netdev model. This is a good thing
but doesn't need to be a global nic policy it can be per port hence
the ethtool flag vs devlink discussion. I don't actually have a use case
to have one port with VF netdevs and another without it so I'm not too
particular on this. Logically it looks like a per port setting because
the hardware has no issues with making one physical function create
a netdev for each of its VFs and the other one run without these
netdevs. This is why I called it out.
How this relates to bridge, tc, etc. is now you have a identifier to
configure instead of using strange 'ip link set ... vf#' commands. This
is great. But I see no reason the hardware has to make changes to
the existing tables or any of this. Before we used 'bridge fdb' and 'ip
link' now we can use bridge tools more effectively and can deprecate
the overloaded use of ip. But again I see no reason to thrash the
forwarding state of the switch because we happen to be adding VFs.
Having a set of fdb rules to forward MAC/Vlan pairs (as we do now)
seems like a perfectly reasonable default. Add with this patch now
when I run 'fdb show' I can see the defaults.
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
John,
I'll try to address here the core questions and arguments you brought.
thanks. Also just to reiterate I really like the series just a few
details here.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
The new model has few building blocks, and by all means, have the VF
representors is not the full story, which is not magic but rather the
following:
1. VF (vport) representors netdevices + the needed mechanics
(send-to-vport rules that makes xmit on VF rep --> recv on VF)
We all agree on this. For me this should be its own knob VF netdevs or
no VF netdevs.
There is also my point that this is really a port attribute of the PCIe
configuration not a switch attribute.
2. handling HW data-patch misses --> send to CPU or drop
Yep need this also but we have a standard way to configure this already
with bridge and 'tc' so why have a toggle for it? Also you don't know
in the driver where I want to send missed packets. In some use cases I
have the VM is managing the system and in these cases I want to send
missed packets to a VF.
In ixgbe we get this for free (with the vf identifier netdevs) because
we have 'tc' and 'bridge' already hooked up. With 'tc' you can define
a wild card match with low priority and with 'bridge' model you can
setup the flood ports to do this.
3. ability to offload SW rules (tc/bridge/etc) using VF representors and
ingress qdiscs / bridge fdb rules / switchdev fdb rule, etc
The knob we suggested says that the system is put into a state where
1,2,3 are needed to make it full performance and functional one. This
submission includes parts 1 and 2, so the offloading of SW rules will
done in successive submission which uses TC offloads which are already
upstream (u32 or flower).
So... we're almost in agreement, do you have another name for the knob
that goes beyond creation/deletion of VF reps? maybe that would be it
for making a progress...
The sticking point for me is (2) is not needed if you do (3)
correctly. So once you have implemented bridge and one of the 'tc'
classifiers that can be used to specify the policy in (2) and you don't
have a chunk policy being defined by the driver writer.
Just to put out an alternative if you add an ethtool feature flag 'VF
representer' so that I can specify enable/disable of VFs per port that
would resolve my concerns.
If you have this additional switch in devlink to hammer the datapath
between two switch modes that seems OK but I'm not sure who else other
than mlx drivers would use it. Additionally if you just used this
devlink hook to set the feature flag on each port and made it 'fixed'
from an ethtool perspective that would work for me as well. Then on
my devices that support VF representers per port I can configure it
and on the devices that can only do it globally it is configured with
this devlink thing.
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
Thanks,
.John
From: Or Gerlitz <hidden> Date: 2016-06-29 21:33:51
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
quoted
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
quoted
quoted
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
quoted
quoted
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
quoted
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Just to clarify, to what exact bridge command support did you refer for ixgbe?
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
[...]
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-29 22:09:22
On 16-06-29 02:33 PM, Or Gerlitz wrote:
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
quoted
quoted
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
quoted
quoted
quoted
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
quoted
quoted
quoted
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
quoted
quoted
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
quoted
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-30 03:36:39
On 16-06-29 03:09 PM, John Fastabend wrote:
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
quoted
quoted
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
quoted
quoted
quoted
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
quoted
quoted
quoted
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
quoted
quoted
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
quoted
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-30 04:05:01
On 16-06-29 08:35 PM, John Fastabend wrote:
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
quoted
quoted
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
quoted
quoted
quoted
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
quoted
quoted
quoted
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
quoted
quoted
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
quoted
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
+};
I'll Ack it and implement it on the drivers I tend to work on.
.John
Thu, Jun 30, 2016 at 06:04:39AM CEST, john.fastabend@gmail.com wrote:
On 16-06-29 08:35 PM, John Fastabend wrote:
quoted
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
quoted
quoted
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
quoted
quoted
quoted
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
quoted
quoted
quoted
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
quoted
quoted
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
quoted
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
That is certainly totally misleading name. The mode is not about
creating "VF netdevs".
The VF representors are created but just as a side effect. The "offload"
mode or maybe better "switchdev" mode is creating representor netdevs for
VFs because they are needed in order to be able to configure ESwitch in
the same way we configure physical switches - putting netdevs into
bridge/bond/ovs/whatever. You see stats on the representors. Basicaly
they are the same as physical port representors on physical switch ASIC.
+};
I'll Ack it and implement it on the drivers I tend to work on.
.John
Thu, Jun 30, 2016 at 06:04:39AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-29 08:35 PM, John Fastabend wrote:
quoted
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
That is certainly totally misleading name. The mode is not about
creating "VF netdevs".
The VF representors are created but just as a side effect. The "offload"
mode or maybe better "switchdev" mode is creating representor netdevs for
VFs because they are needed in order to be able to configure ESwitch in
the same way we configure physical switches - putting netdevs into
bridge/bond/ovs/whatever. You see stats on the representors. Basicaly
they are the same as physical port representors on physical switch ASIC.
May be we need 2 new modes
- legacy+ mode which only creates VF netdevs and let the user configure and manage the switch via the standard bridge/tc/ip/ethtool interfaces
- 'offload' or 'switchdev' mode that does more than just creating VF netdevs if it is not possible to configure the switch into this mode via standard interfaces.
quoted
+};
I'll Ack it and implement it on the drivers I tend to work on.
.John
Thu, Jun 30, 2016 at 09:13:55AM CEST, sridhar.samudrala@intel.com wrote:
On 6/29/2016 11:25 PM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 06:04:39AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-29 08:35 PM, John Fastabend wrote:
quoted
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
That is certainly totally misleading name. The mode is not about
creating "VF netdevs".
The VF representors are created but just as a side effect. The "offload"
mode or maybe better "switchdev" mode is creating representor netdevs for
VFs because they are needed in order to be able to configure ESwitch in
the same way we configure physical switches - putting netdevs into
bridge/bond/ovs/whatever. You see stats on the representors. Basicaly
they are the same as physical port representors on physical switch ASIC.
May be we need 2 new modes
- legacy+ mode which only creates VF netdevs and let the user configure and manage the switch via the standard bridge/tc/ip/ethtool interfaces
- 'offload' or 'switchdev' mode that does more than just creating VF netdevs if it is not possible to configure the switch into this mode via standard interfaces.
What?
That what you described as "legacy+" as "let the user configure and
manage the switch via the standard bridge/tc/ip/ethtool interfaces" is
exactly the "offload/switchdev" mode.
The second mode you described is something that I don't get what you are
talking about...
Please forget about legacy. It's a mistake. Similar to SDKs :(
Let's work on getting the proper offload solution in.
quoted
quoted
+};
I'll Ack it and implement it on the drivers I tend to work on.
.John
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-30 07:57:45
On 16-06-30 12:41 AM, Jiri Pirko wrote:
Thu, Jun 30, 2016 at 09:13:55AM CEST, sridhar.samudrala@intel.com wrote:
quoted
On 6/29/2016 11:25 PM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 06:04:39AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-29 08:35 PM, John Fastabend wrote:
quoted
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
That is certainly totally misleading name. The mode is not about
creating "VF netdevs".
The VF representors are created but just as a side effect. The "offload"
mode or maybe better "switchdev" mode is creating representor netdevs for
VFs because they are needed in order to be able to configure ESwitch in
the same way we configure physical switches - putting netdevs into
bridge/bond/ovs/whatever. You see stats on the representors. Basicaly
they are the same as physical port representors on physical switch ASIC.
May be we need 2 new modes
- legacy+ mode which only creates VF netdevs and let the user configure and manage the switch via the standard bridge/tc/ip/ethtool interfaces
- 'offload' or 'switchdev' mode that does more than just creating VF netdevs if it is not possible to configure the switch into this mode via standard interfaces.
What?
That what you described as "legacy+" as "let the user configure and
manage the switch via the standard bridge/tc/ip/ethtool interfaces" is
exactly the "offload/switchdev" mode.
The second mode you described is something that I don't get what you are
talking about...
Please forget about legacy. It's a mistake. Similar to SDKs :(
Let's work on getting the proper offload solution in.
I think the point here is switchdev is not needed to use bridge, tc,
ip, and ethtool tools. By adding the VF representors we can continue
using 'tc', 'bridge', etc. and it is much more interesting because
we bring the VFs into the netdev world even without switchdev support
this is nice. Adding switchdev of course gets you some extra goodies
like l3 and l2 learning if your nic supports it but its not strictly
required to see goodness from this patch. Without switchdev support
you get stats (big win), basic port configuration with ip link cmds,
tc and bridge fdb to name a few.
Also we can't completely forget about legacy though because we have
infrastructure built around it and its unlikely we can switch entirely
over in one shot. For example the firewall application may switch over
to the new VF rep model while the libvirt VM manager continues to use
the 'ip link set ... vf #' model. No reason to stop this from being
supported its actually more work in the code to block it. We get it for
free.
I've come to the conclusion that we are just arguing over a name and
a bit of perspective calling it "offload" mode is OK with me even
though legacy mode did offloading as well just not as interesting of
offloads. If the VF representors are the cause or effect is not all
that important to me.
If drivers populate the fdb table with known MACs is a side issue
IMO (the thread Or and I got lost in) and doesn't need to hold up this
patch.
.John
Thu, Jun 30, 2016 at 09:57:21AM CEST, john.fastabend@gmail.com wrote:
On 16-06-30 12:41 AM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 09:13:55AM CEST, sridhar.samudrala@intel.com wrote:
quoted
On 6/29/2016 11:25 PM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 06:04:39AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-29 08:35 PM, John Fastabend wrote:
quoted
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
That is certainly totally misleading name. The mode is not about
creating "VF netdevs".
The VF representors are created but just as a side effect. The "offload"
mode or maybe better "switchdev" mode is creating representor netdevs for
VFs because they are needed in order to be able to configure ESwitch in
the same way we configure physical switches - putting netdevs into
bridge/bond/ovs/whatever. You see stats on the representors. Basicaly
they are the same as physical port representors on physical switch ASIC.
May be we need 2 new modes
- legacy+ mode which only creates VF netdevs and let the user configure and manage the switch via the standard bridge/tc/ip/ethtool interfaces
- 'offload' or 'switchdev' mode that does more than just creating VF netdevs if it is not possible to configure the switch into this mode via standard interfaces.
What?
That what you described as "legacy+" as "let the user configure and
manage the switch via the standard bridge/tc/ip/ethtool interfaces" is
exactly the "offload/switchdev" mode.
The second mode you described is something that I don't get what you are
talking about...
Please forget about legacy. It's a mistake. Similar to SDKs :(
Let's work on getting the proper offload solution in.
I think the point here is switchdev is not needed to use bridge, tc,
ip, and ethtool tools. By adding the VF representors we can continue
using 'tc', 'bridge', etc. and it is much more interesting because
we bring the VFs into the netdev world even without switchdev support
this is nice. Adding switchdev of course gets you some extra goodies
like l3 and l2 learning if your nic supports it but its not strictly
required to see goodness from this patch. Without switchdev support
you get stats (big win), basic port configuration with ip link cmds,
tc and bridge fdb to name a few.
Why not to have 2 modes:
1) lagacy - the current solution, blackbox eswitch, undefined behaviour
2) switchdev - with representors, all features possible as on physical
switches, whitebox eswitch configured using standard tools?
I don't see *ANY* reason for a hybrid. That would only make things
already complicated much more complicated.
Also we can't completely forget about legacy though because we have
infrastructure built around it and its unlikely we can switch entirely
over in one shot. For example the firewall application may switch over
to the new VF rep model while the libvirt VM manager continues to use
the 'ip link set ... vf #' model. No reason to stop this from being
supported its actually more work in the code to block it. We get it for
free.
Let legacy be legacy, I have no problem with that. New drivers would be
encouraged to implement only new switchdev mode.
I've come to the conclusion that we are just arguing over a name and
a bit of perspective calling it "offload" mode is OK with me even
though legacy mode did offloading as well just not as interesting of
offloads. If the VF representors are the cause or effect is not all
that important to me.
Why not call it just MODE_SWITCHDEV? I believe it describes it the best.
Everyone knows what that is about.
If drivers populate the fdb table with known MACs is a side issue
IMO (the thread Or and I got lost in) and doesn't need to hold up this
patch.
.John
From: Or Gerlitz <hidden> Date: 2016-06-30 14:26:45
On Thu, Jun 30, 2016 at 1:52 PM, Jiri Pirko [off-list ref] wrote:
Why not to have 2 modes:
1) lagacy - the current solution, blackbox eswitch, undefined behaviour
2) switchdev - with representors, all features possible as on physical
switches, whitebox eswitch configured using standard tools?
yep, this makes sense to me. I will rename the offloads mode to be
called switchdev
and we'll respin V2 with few more small fixes and clarification on the
change log of this patch
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-30 15:41:20
On 16-06-30 03:52 AM, Jiri Pirko wrote:
Thu, Jun 30, 2016 at 09:57:21AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-30 12:41 AM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 09:13:55AM CEST, sridhar.samudrala@intel.com wrote:
quoted
On 6/29/2016 11:25 PM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 06:04:39AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-29 08:35 PM, John Fastabend wrote:
quoted
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
That is certainly totally misleading name. The mode is not about
creating "VF netdevs".
The VF representors are created but just as a side effect. The "offload"
mode or maybe better "switchdev" mode is creating representor netdevs for
VFs because they are needed in order to be able to configure ESwitch in
the same way we configure physical switches - putting netdevs into
bridge/bond/ovs/whatever. You see stats on the representors. Basicaly
they are the same as physical port representors on physical switch ASIC.
May be we need 2 new modes
- legacy+ mode which only creates VF netdevs and let the user configure and manage the switch via the standard bridge/tc/ip/ethtool interfaces
- 'offload' or 'switchdev' mode that does more than just creating VF netdevs if it is not possible to configure the switch into this mode via standard interfaces.
What?
That what you described as "legacy+" as "let the user configure and
manage the switch via the standard bridge/tc/ip/ethtool interfaces" is
exactly the "offload/switchdev" mode.
The second mode you described is something that I don't get what you are
talking about...
Please forget about legacy. It's a mistake. Similar to SDKs :(
Let's work on getting the proper offload solution in.
I think the point here is switchdev is not needed to use bridge, tc,
ip, and ethtool tools. By adding the VF representors we can continue
using 'tc', 'bridge', etc. and it is much more interesting because
we bring the VFs into the netdev world even without switchdev support
this is nice. Adding switchdev of course gets you some extra goodies
like l3 and l2 learning if your nic supports it but its not strictly
required to see goodness from this patch. Without switchdev support
you get stats (big win), basic port configuration with ip link cmds,
tc and bridge fdb to name a few.
Why not to have 2 modes:
1) lagacy - the current solution, blackbox eswitch, undefined behaviour
2) switchdev - with representors, all features possible as on physical
switches, whitebox eswitch configured using standard tools?
I don't see *ANY* reason for a hybrid. That would only make things
already complicated much more complicated.
quoted
Also we can't completely forget about legacy though because we have
infrastructure built around it and its unlikely we can switch entirely
over in one shot. For example the firewall application may switch over
to the new VF rep model while the libvirt VM manager continues to use
the 'ip link set ... vf #' model. No reason to stop this from being
supported its actually more work in the code to block it. We get it for
free.
Let legacy be legacy, I have no problem with that. New drivers would be
encouraged to implement only new switchdev mode.
Nope I disagree there is no reason to break existing userspace here just
continue to support the handful of ip commands and bridge commands
already supported. The code is already in the driver and supported.
In general the kernel shouldn't break UAPI already in place.
quoted
I've come to the conclusion that we are just arguing over a name and
a bit of perspective calling it "offload" mode is OK with me even
though legacy mode did offloading as well just not as interesting of
offloads. If the VF representors are the cause or effect is not all
that important to me.
Why not call it just MODE_SWITCHDEV? I believe it describes it the best.
Everyone knows what that is about.
This is fine but it doesn't require drivers actually register with
switchdev here to get the goodness.
quoted
If drivers populate the fdb table with known MACs is a side issue
IMO (the thread Or and I got lost in) and doesn't need to hold up this
patch.
.John
Thu, Jun 30, 2016 at 05:40:57PM CEST, john.fastabend@gmail.com wrote:
On 16-06-30 03:52 AM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 09:57:21AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-30 12:41 AM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 09:13:55AM CEST, sridhar.samudrala@intel.com wrote:
quoted
On 6/29/2016 11:25 PM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 06:04:39AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-29 08:35 PM, John Fastabend wrote:
quoted
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
That is certainly totally misleading name. The mode is not about
creating "VF netdevs".
The VF representors are created but just as a side effect. The "offload"
mode or maybe better "switchdev" mode is creating representor netdevs for
VFs because they are needed in order to be able to configure ESwitch in
the same way we configure physical switches - putting netdevs into
bridge/bond/ovs/whatever. You see stats on the representors. Basicaly
they are the same as physical port representors on physical switch ASIC.
May be we need 2 new modes
- legacy+ mode which only creates VF netdevs and let the user configure and manage the switch via the standard bridge/tc/ip/ethtool interfaces
- 'offload' or 'switchdev' mode that does more than just creating VF netdevs if it is not possible to configure the switch into this mode via standard interfaces.
What?
That what you described as "legacy+" as "let the user configure and
manage the switch via the standard bridge/tc/ip/ethtool interfaces" is
exactly the "offload/switchdev" mode.
The second mode you described is something that I don't get what you are
talking about...
Please forget about legacy. It's a mistake. Similar to SDKs :(
Let's work on getting the proper offload solution in.
I think the point here is switchdev is not needed to use bridge, tc,
ip, and ethtool tools. By adding the VF representors we can continue
using 'tc', 'bridge', etc. and it is much more interesting because
we bring the VFs into the netdev world even without switchdev support
this is nice. Adding switchdev of course gets you some extra goodies
like l3 and l2 learning if your nic supports it but its not strictly
required to see goodness from this patch. Without switchdev support
you get stats (big win), basic port configuration with ip link cmds,
tc and bridge fdb to name a few.
Why not to have 2 modes:
1) lagacy - the current solution, blackbox eswitch, undefined behaviour
2) switchdev - with representors, all features possible as on physical
switches, whitebox eswitch configured using standard tools?
I don't see *ANY* reason for a hybrid. That would only make things
already complicated much more complicated.
quoted
Also we can't completely forget about legacy though because we have
infrastructure built around it and its unlikely we can switch entirely
over in one shot. For example the firewall application may switch over
to the new VF rep model while the libvirt VM manager continues to use
the 'ip link set ... vf #' model. No reason to stop this from being
supported its actually more work in the code to block it. We get it for
free.
Let legacy be legacy, I have no problem with that. New drivers would be
encouraged to implement only new switchdev mode.
Nope I disagree there is no reason to break existing userspace here just
continue to support the handful of ip commands and bridge commands
already supported. The code is already in the driver and supported.
In general the kernel shouldn't break UAPI already in place.
Who is breaking existing userspace? I don't understand what breakage are
you are referring to :(
quoted
quoted
I've come to the conclusion that we are just arguing over a name and
a bit of perspective calling it "offload" mode is OK with me even
though legacy mode did offloading as well just not as interesting of
offloads. If the VF representors are the cause or effect is not all
that important to me.
Why not call it just MODE_SWITCHDEV? I believe it describes it the best.
Everyone knows what that is about.
This is fine but it doesn't require drivers actually register with
switchdev here to get the goodness.
Switch id for ports, that is the only thing needed.
quoted
quoted
If drivers populate the fdb table with known MACs is a side issue
IMO (the thread Or and I got lost in) and doesn't need to hold up this
patch.
.John
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-06-30 16:29:26
On 16-06-30 08:53 AM, Jiri Pirko wrote:
Thu, Jun 30, 2016 at 05:40:57PM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-30 03:52 AM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 09:57:21AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-30 12:41 AM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 09:13:55AM CEST, sridhar.samudrala@intel.com wrote:
quoted
On 6/29/2016 11:25 PM, Jiri Pirko wrote:
quoted
Thu, Jun 30, 2016 at 06:04:39AM CEST, john.fastabend@gmail.com wrote:
quoted
On 16-06-29 08:35 PM, John Fastabend wrote:
quoted
On 16-06-29 03:09 PM, John Fastabend wrote:
quoted
On 16-06-29 02:33 PM, Or Gerlitz wrote:
quoted
On Wed, Jun 29, 2016 at 7:35 PM, John Fastabend
[off-list ref] wrote:
quoted
On 16-06-29 07:48 AM, Or Gerlitz wrote:
quoted
On 6/28/2016 10:31 PM, John Fastabend wrote:
quoted
On 16-06-28 12:12 PM, Jiri Pirko wrote:
quoted
Why?! Please, leave legacy be legacy. Use the new mode for
implementing new features. Don't make things any more complicated :(
[...]
quoted
quoted
quoted
Maybe I'm reading to much into the devlink flag names and if instead
you use a switch like the following,
VF representer : enable/disable the creation VF netdev's to represent
the virtual functions on the PF
Much less complicated then magic switching between forwarding logic IMO
and you don't whack a default configuration that an entire stack (e.g.
libvirt) has been built to use.
Re letting the user to observe/modify the rules added by the
driver/firmware while legacy mode. Even if possible with bridge/fdb, it
will be really pragmatical and doesn't make sense to get that donefor
the TC subsystem. So this isn't a well defined solution and anyway, as
you said, legacy mode enhancements is a different exercise. Personally,
I agree with Jiri, that we should legacy be legacyand focus on adding
the new model.
The ixgbe driver already supports bridge and tc commands without the VF
representer. Adding the VF representer to these drivers just extends
the existing support so we have an identifier for VFs and now the
redirect action works and the fdb commands can specify the VF netdevs.
I don't see this as a problem because we already do it today with
'ip' and bridge tools.
To be precise, for both ixgbe and mlx5, the existing tc support
(u32/ixgbe, flower/mlx5) is not for switching functionality but rather
for NIC-ish one, e.g drop, mark, etc. Indeed in ixgbe you added
redirect to VF, but this is only for south --> north (wire --> VF)
traffic, w.o the VF rep you can't do the other way around.
Correct which is why we need the VF rep. So we are completely in
sync there.
quoted
Just to clarify, to what exact bridge command support did you refer for ixgbe?
'bridge fdb' commands are supported today on the PF. But its the
same story as above we need the VF rep to also use it on the
VF representer
Also 'bridge link' command for veb/vepa modes is supported and the
other link attributes could be supported with additional driver
support. No need for core changes here. But again yes only on the
PF so again we need the VF reps.
quoted
The forwarding done in the legacy mode is not well defined, and
different across vendors, adding there the VF reps will not make it
any better b/c some steering rules will be set by tc/bridge offloads
while other rules will be put by the driver.
I don't see how this takes us to better place.
In legacy mode or any other mode you are defining some default policy
and rules.
In the legacy mode we use mac/vlan assigned l2 forwarding entries in the
hardware fdb which are seen when you query 'ip link' and 'bridge fdb'
today. And similarly can be modified today using 'ip link' and 'bridge
fdb' at least on the intel devices. Its not undefined in any way with
a quick query of the tools we can learn exactly what the configuration
is and even change it. This works fairly well with existing controllers
and stacks.
The limitations are 'ip' only supports a single MAC address per VF and
'tc' doesn't work on VF ports because when the VF is assigned to a VM
or namespace we lose visibility of it. Providing a VF rep for this
solves both of those problems.
In this new mode the default policy is to create a default miss rule
and implement no l2 forwarding rules. Unfortunately not all hardware
in use supports this default miss rule case but would still benefit
from having a VF rep. So we shouldn't make this a stipulation for
enabling VF reps. It also changes a default policy that has been in
place for years without IMO at least any compelling reason. It will
be easy enough to change the default l2 policy to a flow based model
with a few bridge/tc commands.
quoted
quoted
We are also slightly in disagreement about what the default should be
with VF netdevs. I think the default should be the same L2 mac/vlan
switch behavior and see no reason to change it by default just because
we added VF netdevs. The infrastructure libvirt/openstack/etc are built
around this default today. But I guess nothing in this series specifies
what the defaults of any given driver will be. VF netdevs are still
useful even on older hardware that only supports mac/vlan forwarding to
expose statistics and send/receive control frames such as lldp.
Again, this is not about default engineering... and using the VF reps
(not VF netdevs) in legacy mode only make it more cryptic to my
opinion. I agree some changes would be needed in openstack to support
the new model, but this is how progress is made... you can't always
make all layer above you unchanged. Note that the VF reps behave the
same as tap devices (v-switch doing xmit on tap --> recv in VM, VM
sends --> recv on tap into the v-switch), so the change in open-stack
would not be that big.
But in this case we have no reason to break the stack above us. The
currently deployed usage is L2 mac/vlan. As soon as you bind a vSwitch
or whatever mgmt agent to the device it can go ahead and manage the
switch putting it in the correct mode using the tooling in 'bridge' and
'tc'.
quoted
[...]
quoted
Why I think the VF representer is a per port ethtool flag and not a
devlink option is my use case might be to assign a PF into a VM or
namespace where I don't want VF netdevs.
again, we think the correct place to set how the eswitch is managed is
through eswitch manager PCI devices and not net devices and hence
ethtool is not the way to go.
Also, how do you want your e-switch to be managed in this case?
In the case where I don't create vf netdevs on one of the PFs I'll
manage the forwarding tables via the existing mechanisms 'ip' and
'bridge'. However its likely not a big deal because 'ip' and 'bridge'
will continue to work even if VF reps are around. The ethtool/devlink
comment was more about pointing out that creating VFs does not
require you to manage your switch any differently. Its useful even on
devices that can't support flow based forwarding for statistics and
setting port attributes like mtu, etc.
.John
Probably bad form to respond to my own email but just to highlight how
subtle the distinction is (hopefully not to much repeat),
Today in "legacy" mode each VF mac address is automatically added to
the fdb along with the PF mac address. If there is a miss in the table
(an unknown mac) the packet is sent to the PF but unless the PF is in
promisc mode the packet is dropped by the rx filter. I presume even
with the proposed model you would want to continue to enforce the
rx filter otherwise the instance you flip the mode you are open to
receive unwanted traffic. The promisc mode semantics have been in place
for a long time so certainly don't want to break that. Can we agree on
the promisc point? Also bridges/vswitch/etc already set promisc mode
once they attach to the netdevs.
(assuming we agree on the promisc point?)
In your proposed model the only difference I can see is when the mode is
changed you don't want to add the VF mac address to the fdb table. How
about rather than make this part of the mode selection pick one way to
do this in all cases. Either add the VF mac addresses to the fdb or
do not do this. I have a preference for adding the VF mac addresses
because this is the current behavior. Then rename the devlink option
"VF reps" or something because that is what it is controlling.
The last thing to argue about is if its a port attribute ala ethtool
or a device attribute ala devlink. But maybe we can agree on everything
up to this point?
Thanks,
John
FWIW reviewing devlink and items I want to put there in the future I've
decided it makes sense to keep it in devlink (sorry took me a day of
emails to get here). If you can agree to the above and rename it
something like,
+enum devlink_eswitch_mode {
+ DEVLINK_ESWITCH_MODE_NONE,
+ DEVLINK_ESWITCH_MODE_LEGACY,
+ DEVLINK_ESWITCH_MODE_CREATE_VF_NETDEVS,
That is certainly totally misleading name. The mode is not about
creating "VF netdevs".
The VF representors are created but just as a side effect. The "offload"
mode or maybe better "switchdev" mode is creating representor netdevs for
VFs because they are needed in order to be able to configure ESwitch in
the same way we configure physical switches - putting netdevs into
bridge/bond/ovs/whatever. You see stats on the representors. Basicaly
they are the same as physical port representors on physical switch ASIC.
May be we need 2 new modes
- legacy+ mode which only creates VF netdevs and let the user configure and manage the switch via the standard bridge/tc/ip/ethtool interfaces
- 'offload' or 'switchdev' mode that does more than just creating VF netdevs if it is not possible to configure the switch into this mode via standard interfaces.
What?
That what you described as "legacy+" as "let the user configure and
manage the switch via the standard bridge/tc/ip/ethtool interfaces" is
exactly the "offload/switchdev" mode.
The second mode you described is something that I don't get what you are
talking about...
Please forget about legacy. It's a mistake. Similar to SDKs :(
Let's work on getting the proper offload solution in.
I think the point here is switchdev is not needed to use bridge, tc,
ip, and ethtool tools. By adding the VF representors we can continue
using 'tc', 'bridge', etc. and it is much more interesting because
we bring the VFs into the netdev world even without switchdev support
this is nice. Adding switchdev of course gets you some extra goodies
like l3 and l2 learning if your nic supports it but its not strictly
required to see goodness from this patch. Without switchdev support
you get stats (big win), basic port configuration with ip link cmds,
tc and bridge fdb to name a few.
Why not to have 2 modes:
1) lagacy - the current solution, blackbox eswitch, undefined behaviour
2) switchdev - with representors, all features possible as on physical
switches, whitebox eswitch configured using standard tools?
I don't see *ANY* reason for a hybrid. That would only make things
already complicated much more complicated.
quoted
Also we can't completely forget about legacy though because we have
infrastructure built around it and its unlikely we can switch entirely
over in one shot. For example the firewall application may switch over
to the new VF rep model while the libvirt VM manager continues to use
the 'ip link set ... vf #' model. No reason to stop this from being
supported its actually more work in the code to block it. We get it for
free.
Let legacy be legacy, I have no problem with that. New drivers would be
encouraged to implement only new switchdev mode.
Nope I disagree there is no reason to break existing userspace here just
continue to support the handful of ip commands and bridge commands
already supported. The code is already in the driver and supported.
In general the kernel shouldn't break UAPI already in place.
Who is breaking existing userspace? I don't understand what breakage are
you are referring to :(
When we switch to 'offload'/'switchdev' mode please continue to support
the 'ip link set ... vf #' command and 'bridge fdb' commands. These use
the following ops,
ndo_set_vf_*
ndo_get_vf_config
ndo_get_stats64
ndo_fdb_add
ndo_bridge_*
Then userspace continues to work. Note your patchset doesn't block these
ops so it should all continue to work.
quoted
quoted
quoted
I've come to the conclusion that we are just arguing over a name and
a bit of perspective calling it "offload" mode is OK with me even
though legacy mode did offloading as well just not as interesting of
offloads. If the VF representors are the cause or effect is not all
that important to me.
Why not call it just MODE_SWITCHDEV? I believe it describes it the best.
Everyone knows what that is about.
This is fine but it doesn't require drivers actually register with
switchdev here to get the goodness.
Switch id for ports, that is the only thing needed.
Fair enough. For many NICs where ports are isolated (e.g. port can
not forward to other ports and rules on one port do not effect other
ports) the switch id is the phys_port_id. But agree implementing the
switch id lets userspace figure out the hierarchy between physical
ports and switch domains vs the old way of assuming a nic model that
binds independent switches to ports.
A generic wrapper to return the phys_port_id as the switchid would be
useful on a handful of nics but maybe will see if its worth the effort.
.John