| Message ID | 20190617192247.25107-12-saeedm@mellanox.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | Mellanox, mlx5 vport metadata matching | expand |
On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: > From: Jianbo Liu <jianbol@mellanox.com> > > If vport metadata matching is enabled in eswitch, the rule created > must be changed to match on the metadata, instead of source port. > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> > Reviewed-by: Mark Bloch <markb@mellanox.com> > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> > --- > drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ > drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ > drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ > 3 files changed, 63 insertions(+), 9 deletions(-) > > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c > index 22e651cb5534..d4ed611de35d 100644 > --- a/drivers/infiniband/hw/mlx5/ib_rep.c > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) > return mlx5_eswitch_vport_rep(esw, vport); > } > > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) > +{ > + return mlx5_eswitch_vport_match_metadata_enabled(esw); > +} > + > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, > + u16 vport) > +{ > + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); > +} 1. There is no need to introduce one line functions, call to that code directly. 2. It should be bool and not u32. Thanks
The 06/18/2019 18:19, Leon Romanovsky wrote: > On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: > > From: Jianbo Liu <jianbol@mellanox.com> > > > > If vport metadata matching is enabled in eswitch, the rule created > > must be changed to match on the metadata, instead of source port. > > > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > > Reviewed-by: Roi Dayan <roid@mellanox.com> > > Reviewed-by: Mark Bloch <markb@mellanox.com> > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> > > --- > > drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ > > drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ > > drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ > > 3 files changed, 63 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c > > index 22e651cb5534..d4ed611de35d 100644 > > --- a/drivers/infiniband/hw/mlx5/ib_rep.c > > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c > > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) > > return mlx5_eswitch_vport_rep(esw, vport); > > } > > > > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) > > +{ > > + return mlx5_eswitch_vport_match_metadata_enabled(esw); > > +} > > + > > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, > > + u16 vport) > > +{ > > + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); > > +} > > 1. There is no need to introduce one line functions, call to that code directly. No. They are in IB, and we don't want them be mixed up by the original functions in eswitch. Please ask Mark more about it. > 2. It should be bool and not u32. > > Thanks --
On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote: > The 06/18/2019 18:19, Leon Romanovsky wrote: > > On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: > > > From: Jianbo Liu <jianbol@mellanox.com> > > > > > > If vport metadata matching is enabled in eswitch, the rule created > > > must be changed to match on the metadata, instead of source port. > > > > > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > > > Reviewed-by: Roi Dayan <roid@mellanox.com> > > > Reviewed-by: Mark Bloch <markb@mellanox.com> > > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> > > > --- > > > drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ > > > drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ > > > drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ > > > 3 files changed, 63 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c > > > index 22e651cb5534..d4ed611de35d 100644 > > > --- a/drivers/infiniband/hw/mlx5/ib_rep.c > > > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c > > > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) > > > return mlx5_eswitch_vport_rep(esw, vport); > > > } > > > > > > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) > > > +{ > > > + return mlx5_eswitch_vport_match_metadata_enabled(esw); > > > +} > > > + > > > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, > > > + u16 vport) > > > +{ > > > + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); > > > +} > > > > 1. There is no need to introduce one line functions, call to that code directly. > > No. They are in IB, and we don't want them be mixed up by the original > functions in eswitch. Please ask Mark more about it. Please enlighten me. > > > 2. It should be bool and not u32. > > > > Thanks > > --
The 06/19/2019 13:04, Leon Romanovsky wrote: > On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote: > > The 06/18/2019 18:19, Leon Romanovsky wrote: > > > On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: > > > > From: Jianbo Liu <jianbol@mellanox.com> > > > > > > > > If vport metadata matching is enabled in eswitch, the rule created > > > > must be changed to match on the metadata, instead of source port. > > > > > > > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > > > > Reviewed-by: Roi Dayan <roid@mellanox.com> > > > > Reviewed-by: Mark Bloch <markb@mellanox.com> > > > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> > > > > --- > > > > drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ > > > > drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ > > > > drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ > > > > 3 files changed, 63 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c > > > > index 22e651cb5534..d4ed611de35d 100644 > > > > --- a/drivers/infiniband/hw/mlx5/ib_rep.c > > > > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c > > > > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) > > > > return mlx5_eswitch_vport_rep(esw, vport); > > > > } > > > > > > > > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) > > > > +{ > > > > + return mlx5_eswitch_vport_match_metadata_enabled(esw); > > > > +} > > > > + > > > > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, > > > > + u16 vport) > > > > +{ > > > > + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); > > > > +} > > > > > > 1. There is no need to introduce one line functions, call to that code directly. > > > > No. They are in IB, and we don't want them be mixed up by the original > > functions in eswitch. Please ask Mark more about it. > > Please enlighten me. It was suggested by Mark in prevouis review. I think it's because there are in different modules, and better to with different names, so introduce there extra one line functions. Please correct me if I'm wrong, Mark... > > > > > > 2. It should be bool and not u32. > > > > > > Thanks > > > > -- --
On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote: > The 06/19/2019 13:04, Leon Romanovsky wrote: > > On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote: > > > The 06/18/2019 18:19, Leon Romanovsky wrote: > > > > On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: > > > > > From: Jianbo Liu <jianbol@mellanox.com> > > > > > > > > > > If vport metadata matching is enabled in eswitch, the rule created > > > > > must be changed to match on the metadata, instead of source port. > > > > > > > > > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > > > > > Reviewed-by: Roi Dayan <roid@mellanox.com> > > > > > Reviewed-by: Mark Bloch <markb@mellanox.com> > > > > > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> > > > > > --- > > > > > drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ > > > > > drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ > > > > > drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ > > > > > 3 files changed, 63 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c > > > > > index 22e651cb5534..d4ed611de35d 100644 > > > > > --- a/drivers/infiniband/hw/mlx5/ib_rep.c > > > > > +++ b/drivers/infiniband/hw/mlx5/ib_rep.c > > > > > @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) > > > > > return mlx5_eswitch_vport_rep(esw, vport); > > > > > } > > > > > > > > > > +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) > > > > > +{ > > > > > + return mlx5_eswitch_vport_match_metadata_enabled(esw); > > > > > +} > > > > > + > > > > > +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, > > > > > + u16 vport) > > > > > +{ > > > > > + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); > > > > > +} > > > > > > > > 1. There is no need to introduce one line functions, call to that code directly. > > > > > > No. They are in IB, and we don't want them be mixed up by the original > > > functions in eswitch. Please ask Mark more about it. > > > > Please enlighten me. > > It was suggested by Mark in prevouis review. > I think it's because there are in different modules, and better to with > different names, so introduce there extra one line functions. > Please correct me if I'm wrong, Mark... mlx5_ib is full of direct function calls to mlx5_core and it is done on purpose for at least two reasons. First is to control in one place all compilation options and expose proper API interface with and without specific kernel config is on. Second is to emphasize that this is core function and save us time in refactoring and reviewing. > > > > > > > > > > 2. It should be bool and not u32. > > > > > > > > Thanks > > > > > > -- > > --
On 6/18/2019 23:51, Leon Romanovsky wrote: > On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote: >> The 06/19/2019 13:04, Leon Romanovsky wrote: >>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote: >>>> The 06/18/2019 18:19, Leon Romanovsky wrote: >>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: >>>>>> From: Jianbo Liu <jianbol@mellanox.com> >>>>>> >>>>>> If vport metadata matching is enabled in eswitch, the rule created >>>>>> must be changed to match on the metadata, instead of source port. >>>>>> >>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com> >>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com> >>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> >>>>>> --- >>>>>> drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ >>>>>> drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ >>>>>> drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ >>>>>> 3 files changed, 63 insertions(+), 9 deletions(-) >>>>>> >>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>> index 22e651cb5534..d4ed611de35d 100644 >>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) >>>>>> return mlx5_eswitch_vport_rep(esw, vport); >>>>>> } >>>>>> >>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) >>>>>> +{ >>>>>> + return mlx5_eswitch_vport_match_metadata_enabled(esw); >>>>>> +} >>>>>> + >>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, >>>>>> + u16 vport) >>>>>> +{ >>>>>> + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); >>>>>> +} >>>>> >>>>> 1. There is no need to introduce one line functions, call to that code directly. >>>> >>>> No. They are in IB, and we don't want them be mixed up by the original >>>> functions in eswitch. Please ask Mark more about it. >>> >>> Please enlighten me. >> >> It was suggested by Mark in prevouis review. >> I think it's because there are in different modules, and better to with >> different names, so introduce there extra one line functions. >> Please correct me if I'm wrong, Mark... > > mlx5_ib is full of direct function calls to mlx5_core and it is done on > purpose for at least two reasons. First is to control in one place > all compilation options and expose proper API interface with and without > specific kernel config is on. Second is to emphasize that this is core > function and save us time in refactoring and reviewing. This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH, I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined. (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode() should have probably done the same. As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC) I want to group together stuff in separate files. If you prefer direct calls that's okay as well. Mark > >> >>> >>>> >>>>> 2. It should be bool and not u32. >>>>> >>>>> Thanks >>>> >>>> -- >> >> --
On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote: > > > On 6/18/2019 23:51, Leon Romanovsky wrote: > > On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote: > >> The 06/19/2019 13:04, Leon Romanovsky wrote: > >>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote: > >>>> The 06/18/2019 18:19, Leon Romanovsky wrote: > >>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: > >>>>>> From: Jianbo Liu <jianbol@mellanox.com> > >>>>>> > >>>>>> If vport metadata matching is enabled in eswitch, the rule created > >>>>>> must be changed to match on the metadata, instead of source port. > >>>>>> > >>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > >>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com> > >>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com> > >>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> > >>>>>> --- > >>>>>> drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ > >>>>>> drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ > >>>>>> drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ > >>>>>> 3 files changed, 63 insertions(+), 9 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c > >>>>>> index 22e651cb5534..d4ed611de35d 100644 > >>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c > >>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c > >>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) > >>>>>> return mlx5_eswitch_vport_rep(esw, vport); > >>>>>> } > >>>>>> > >>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) > >>>>>> +{ > >>>>>> + return mlx5_eswitch_vport_match_metadata_enabled(esw); > >>>>>> +} > >>>>>> + > >>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, > >>>>>> + u16 vport) > >>>>>> +{ > >>>>>> + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); > >>>>>> +} > >>>>> > >>>>> 1. There is no need to introduce one line functions, call to that code directly. > >>>> > >>>> No. They are in IB, and we don't want them be mixed up by the original > >>>> functions in eswitch. Please ask Mark more about it. > >>> > >>> Please enlighten me. > >> > >> It was suggested by Mark in prevouis review. > >> I think it's because there are in different modules, and better to with > >> different names, so introduce there extra one line functions. > >> Please correct me if I'm wrong, Mark... > > > > mlx5_ib is full of direct function calls to mlx5_core and it is done on > > purpose for at least two reasons. First is to control in one place > > all compilation options and expose proper API interface with and without > > specific kernel config is on. Second is to emphasize that this is core > > function and save us time in refactoring and reviewing. > > This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH, > I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h > so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined. > (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode() > should have probably done the same. This is exactly the problem, eswitch.h should provide stubs for all exported functions, so other clients of eswitch won't need to deal with various unrelated config options. > > As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC) > I want to group together stuff in separate files. Yes, it is right thing to do. > > If you prefer direct calls that's okay as well. Yes, please. > > Mark > > > > >> > >>> > >>>> > >>>>> 2. It should be bool and not u32. > >>>>> > >>>>> Thanks > >>>> > >>>> -- > >> > >> --
On 6/19/2019 00:43, Leon Romanovsky wrote: > On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote: >> >> >> On 6/18/2019 23:51, Leon Romanovsky wrote: >>> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote: >>>> The 06/19/2019 13:04, Leon Romanovsky wrote: >>>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote: >>>>>> The 06/18/2019 18:19, Leon Romanovsky wrote: >>>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: >>>>>>>> From: Jianbo Liu <jianbol@mellanox.com> >>>>>>>> >>>>>>>> If vport metadata matching is enabled in eswitch, the rule created >>>>>>>> must be changed to match on the metadata, instead of source port. >>>>>>>> >>>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com> >>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com> >>>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> >>>>>>>> --- >>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ >>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ >>>>>>>> drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ >>>>>>>> 3 files changed, 63 insertions(+), 9 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>>>> index 22e651cb5534..d4ed611de35d 100644 >>>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) >>>>>>>> return mlx5_eswitch_vport_rep(esw, vport); >>>>>>>> } >>>>>>>> >>>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) >>>>>>>> +{ >>>>>>>> + return mlx5_eswitch_vport_match_metadata_enabled(esw); >>>>>>>> +} >>>>>>>> + >>>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, >>>>>>>> + u16 vport) >>>>>>>> +{ >>>>>>>> + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); >>>>>>>> +} >>>>>>> >>>>>>> 1. There is no need to introduce one line functions, call to that code directly. >>>>>> >>>>>> No. They are in IB, and we don't want them be mixed up by the original >>>>>> functions in eswitch. Please ask Mark more about it. >>>>> >>>>> Please enlighten me. >>>> >>>> It was suggested by Mark in prevouis review. >>>> I think it's because there are in different modules, and better to with >>>> different names, so introduce there extra one line functions. >>>> Please correct me if I'm wrong, Mark... >>> >>> mlx5_ib is full of direct function calls to mlx5_core and it is done on >>> purpose for at least two reasons. First is to control in one place >>> all compilation options and expose proper API interface with and without >>> specific kernel config is on. Second is to emphasize that this is core >>> function and save us time in refactoring and reviewing. >> >> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH, >> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h >> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined. >> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode() >> should have probably done the same. > > This is exactly the problem, eswitch.h should provide stubs for all > exported functions, so other clients of eswitch won't need to deal with > various unrelated config options. The way it works today, code in drivers/infiniband/hw/mlx5/main.c doesn't call eswitch layer directly but the functions in ib_rep.{c,h} as most often there is additional logic we must do before calling the eswitch layer. If you look at drivers/infiniband/hw/mlx5/Makefile you will see ib_rep is complied only when CONFIG_MLX5_ESWITCH id defined. so instead of having to deal with two places that contain stubs, we need to deal with only one (ib_rep.h). For me it makes it easier to follow, but I can adept if you don't like it. Mark > >> >> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC) >> I want to group together stuff in separate files. > > Yes, it is right thing to do. > >> >> If you prefer direct calls that's okay as well. > > Yes, please. > >> >> Mark >> >>> >>>> >>>>> >>>>>> >>>>>>> 2. It should be bool and not u32. >>>>>>> >>>>>>> Thanks >>>>>> >>>>>> -- >>>> >>>> --
On Wed, Jun 19, 2019 at 07:58:51AM +0000, Mark Bloch wrote: > > > On 6/19/2019 00:43, Leon Romanovsky wrote: > > On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote: > >> > >> > >> On 6/18/2019 23:51, Leon Romanovsky wrote: > >>> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote: > >>>> The 06/19/2019 13:04, Leon Romanovsky wrote: > >>>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote: > >>>>>> The 06/18/2019 18:19, Leon Romanovsky wrote: > >>>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: > >>>>>>>> From: Jianbo Liu <jianbol@mellanox.com> > >>>>>>>> > >>>>>>>> If vport metadata matching is enabled in eswitch, the rule created > >>>>>>>> must be changed to match on the metadata, instead of source port. > >>>>>>>> > >>>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > >>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com> > >>>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com> > >>>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> > >>>>>>>> --- > >>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ > >>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ > >>>>>>>> drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ > >>>>>>>> 3 files changed, 63 insertions(+), 9 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c > >>>>>>>> index 22e651cb5534..d4ed611de35d 100644 > >>>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c > >>>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c > >>>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) > >>>>>>>> return mlx5_eswitch_vport_rep(esw, vport); > >>>>>>>> } > >>>>>>>> > >>>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) > >>>>>>>> +{ > >>>>>>>> + return mlx5_eswitch_vport_match_metadata_enabled(esw); > >>>>>>>> +} > >>>>>>>> + > >>>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, > >>>>>>>> + u16 vport) > >>>>>>>> +{ > >>>>>>>> + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); > >>>>>>>> +} > >>>>>>> > >>>>>>> 1. There is no need to introduce one line functions, call to that code directly. > >>>>>> > >>>>>> No. They are in IB, and we don't want them be mixed up by the original > >>>>>> functions in eswitch. Please ask Mark more about it. > >>>>> > >>>>> Please enlighten me. > >>>> > >>>> It was suggested by Mark in prevouis review. > >>>> I think it's because there are in different modules, and better to with > >>>> different names, so introduce there extra one line functions. > >>>> Please correct me if I'm wrong, Mark... > >>> > >>> mlx5_ib is full of direct function calls to mlx5_core and it is done on > >>> purpose for at least two reasons. First is to control in one place > >>> all compilation options and expose proper API interface with and without > >>> specific kernel config is on. Second is to emphasize that this is core > >>> function and save us time in refactoring and reviewing. > >> > >> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH, > >> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h > >> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined. > >> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode() > >> should have probably done the same. > > > > This is exactly the problem, eswitch.h should provide stubs for all > > exported functions, so other clients of eswitch won't need to deal with > > various unrelated config options. > > The way it works today, code in drivers/infiniband/hw/mlx5/main.c doesn't call eswitch layer directly > but the functions in ib_rep.{c,h} as most often there is additional logic we must do before calling > the eswitch layer. > > If you look at drivers/infiniband/hw/mlx5/Makefile you will see ib_rep is complied only when > CONFIG_MLX5_ESWITCH id defined. This simple patch + cleanup of ib_rep.h will do the trick. diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index 67b9e7ac569a..b917ba28659e 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -59,7 +59,9 @@ #include <linux/in.h> #include <linux/etherdevice.h> #include "mlx5_ib.h" +#if defined(CONFIG_MLX5_ESWITCH) #include "ib_rep.h" +#endif #include "cmd.h" #include "srq.h" #include <linux/mlx5/fs_helpers.h> @@ -6765,6 +6767,7 @@ static const struct mlx5_ib_profile pf_profile = { mlx5_ib_stage_delay_drop_cleanup), }; +#if defined(CONFIG_MLX5_ESWITCH) const struct mlx5_ib_profile uplink_rep_profile = { STAGE_CREATE(MLX5_IB_STAGE_INIT, mlx5_ib_stage_init_init, @@ -6812,6 +6815,7 @@ const struct mlx5_ib_profile uplink_rep_profile = { mlx5_ib_stage_post_ib_reg_umr_init, NULL), }; > > so instead of having to deal with two places that contain stubs, we need to deal with only one (ib_rep.h). > For me it makes it easier to follow, but I can adept if you don't like it. > > Mark > > > > >> > >> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC) > >> I want to group together stuff in separate files. > > > > Yes, it is right thing to do. > > > >> > >> If you prefer direct calls that's okay as well. > > > > Yes, please. > > > >> > >> Mark > >> > >>> > >>>> > >>>>> > >>>>>> > >>>>>>> 2. It should be bool and not u32. > >>>>>>> > >>>>>>> Thanks > >>>>>> > >>>>>> -- > >>>> > >>>> --
On 6/19/19 1:12 AM, Leon Romanovsky wrote: > On Wed, Jun 19, 2019 at 07:58:51AM +0000, Mark Bloch wrote: >> >> >> On 6/19/2019 00:43, Leon Romanovsky wrote: >>> On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote: >>>> >>>> >>>> On 6/18/2019 23:51, Leon Romanovsky wrote: >>>>> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote: >>>>>> The 06/19/2019 13:04, Leon Romanovsky wrote: >>>>>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote: >>>>>>>> The 06/18/2019 18:19, Leon Romanovsky wrote: >>>>>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote: >>>>>>>>>> From: Jianbo Liu <jianbol@mellanox.com> >>>>>>>>>> >>>>>>>>>> If vport metadata matching is enabled in eswitch, the rule created >>>>>>>>>> must be changed to match on the metadata, instead of source port. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com> >>>>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com> >>>>>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com> >>>>>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> >>>>>>>>>> --- >>>>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++ >>>>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++ >>>>>>>>>> drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------ >>>>>>>>>> 3 files changed, 63 insertions(+), 9 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>>>>>> index 22e651cb5534..d4ed611de35d 100644 >>>>>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c >>>>>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) >>>>>>>>>> return mlx5_eswitch_vport_rep(esw, vport); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) >>>>>>>>>> +{ >>>>>>>>>> + return mlx5_eswitch_vport_match_metadata_enabled(esw); >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, >>>>>>>>>> + u16 vport) >>>>>>>>>> +{ >>>>>>>>>> + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); >>>>>>>>>> +} >>>>>>>>> >>>>>>>>> 1. There is no need to introduce one line functions, call to that code directly. >>>>>>>> >>>>>>>> No. They are in IB, and we don't want them be mixed up by the original >>>>>>>> functions in eswitch. Please ask Mark more about it. >>>>>>> >>>>>>> Please enlighten me. >>>>>> >>>>>> It was suggested by Mark in prevouis review. >>>>>> I think it's because there are in different modules, and better to with >>>>>> different names, so introduce there extra one line functions. >>>>>> Please correct me if I'm wrong, Mark... >>>>> >>>>> mlx5_ib is full of direct function calls to mlx5_core and it is done on >>>>> purpose for at least two reasons. First is to control in one place >>>>> all compilation options and expose proper API interface with and without >>>>> specific kernel config is on. Second is to emphasize that this is core >>>>> function and save us time in refactoring and reviewing. >>>> >>>> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH, >>>> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h >>>> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined. >>>> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode() >>>> should have probably done the same. >>> >>> This is exactly the problem, eswitch.h should provide stubs for all >>> exported functions, so other clients of eswitch won't need to deal with >>> various unrelated config options. >> >> The way it works today, code in drivers/infiniband/hw/mlx5/main.c doesn't call eswitch layer directly >> but the functions in ib_rep.{c,h} as most often there is additional logic we must do before calling >> the eswitch layer. >> >> If you look at drivers/infiniband/hw/mlx5/Makefile you will see ib_rep is complied only when >> CONFIG_MLX5_ESWITCH id defined. > > This simple patch + cleanup of ib_rep.h will do the trick. > > diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c > index 67b9e7ac569a..b917ba28659e 100644 > --- a/drivers/infiniband/hw/mlx5/main.c > +++ b/drivers/infiniband/hw/mlx5/main.c > @@ -59,7 +59,9 @@ > #include <linux/in.h> > #include <linux/etherdevice.h> > #include "mlx5_ib.h" > +#if defined(CONFIG_MLX5_ESWITCH) > #include "ib_rep.h" > +#endif > #include "cmd.h" > #include "srq.h" > #include <linux/mlx5/fs_helpers.h> > @@ -6765,6 +6767,7 @@ static const struct mlx5_ib_profile pf_profile = { > mlx5_ib_stage_delay_drop_cleanup), > }; > > +#if defined(CONFIG_MLX5_ESWITCH) > const struct mlx5_ib_profile uplink_rep_profile = { > STAGE_CREATE(MLX5_IB_STAGE_INIT, > mlx5_ib_stage_init_init, > @@ -6812,6 +6815,7 @@ > const struct mlx5_ib_profile uplink_rep_profile = { > mlx5_ib_stage_post_ib_reg_umr_init, > NULL), > }; > I really dislike seeing #if defined(CONFIG_MLX5_ESWITCH) inside .c files and here it's easily avoided so I don't see a reason do it it. Can this cleanup wait for after this series? Mark >> >> so instead of having to deal with two places that contain stubs, we need to deal with only one (ib_rep.h). >> For me it makes it easier to follow, but I can adept if you don't like it. >> >> Mark >> >>> >>>> >>>> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC) >>>> I want to group together stuff in separate files. >>> >>> Yes, it is right thing to do. >>> >>>> >>>> If you prefer direct calls that's okay as well. >>> >>> Yes, please. >>> >>>> >>>> Mark >>>> >>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>>>>> 2. It should be bool and not u32. >>>>>>>>> >>>>>>>>> Thanks >>>>>>>> >>>>>>>> -- >>>>>> >>>>>> --
diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c index 22e651cb5534..d4ed611de35d 100644 --- a/drivers/infiniband/hw/mlx5/ib_rep.c +++ b/drivers/infiniband/hw/mlx5/ib_rep.c @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport) return mlx5_eswitch_vport_rep(esw, vport); } +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) +{ + return mlx5_eswitch_vport_match_metadata_enabled(esw); +} + +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, + u16 vport) +{ + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport); +} + struct mlx5_flow_handle *create_flow_rule_vport_sq(struct mlx5_ib_dev *dev, struct mlx5_ib_sq *sq, u16 port) diff --git a/drivers/infiniband/hw/mlx5/ib_rep.h b/drivers/infiniband/hw/mlx5/ib_rep.h index 22adce2d6795..65a04b6b0df3 100644 --- a/drivers/infiniband/hw/mlx5/ib_rep.h +++ b/drivers/infiniband/hw/mlx5/ib_rep.h @@ -25,6 +25,9 @@ struct mlx5_flow_handle *create_flow_rule_vport_sq(struct mlx5_ib_dev *dev, u16 port); struct net_device *mlx5_ib_get_rep_netdev(struct mlx5_eswitch *esw, int vport_index); +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw); +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, + u16 vport); #else /* CONFIG_MLX5_ESWITCH */ static inline u8 mlx5_ib_eswitch_mode(struct mlx5_eswitch *esw) { @@ -67,6 +70,19 @@ struct net_device *mlx5_ib_get_rep_netdev(struct mlx5_eswitch *esw, { return NULL; } + +static inline +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw) +{ + return 0; +}; + +static inline +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw, + u16 vport) +{ + return 0; +}; #endif static inline diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index be4c9a687df7..f97519c0c4df 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -3469,6 +3469,37 @@ static int flow_counters_set_data(struct ib_counters *ibcounters, return ret; } +static void mlx5_ib_set_rule_source_port(struct mlx5_ib_dev *dev, + struct mlx5_flow_spec *spec, + struct mlx5_eswitch_rep *rep) +{ + struct mlx5_eswitch *esw = dev->mdev->priv.eswitch; + void *misc; + + if (mlx5_ib_eswitch_vport_match_metadata_enabled(esw)) { + misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, + misc_parameters_2); + + MLX5_SET(fte_match_set_misc2, misc, metadata_reg_c_0, + mlx5_ib_eswitch_get_vport_metadata_for_match(esw, + rep->vport)); + misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, + misc_parameters_2); + + MLX5_SET_TO_ONES(fte_match_set_misc2, misc, metadata_reg_c_0); + } else { + misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, + misc_parameters); + + MLX5_SET(fte_match_set_misc, misc, source_port, rep->vport); + + misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, + misc_parameters); + + MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port); + } +} + static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev, struct mlx5_ib_flow_prio *ft_prio, const struct ib_flow_attr *flow_attr, @@ -3523,19 +3554,15 @@ static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev, set_underlay_qp(dev, spec, underlay_qpn); if (dev->is_rep) { - void *misc; + struct mlx5_eswitch_rep *rep; - if (!dev->port[flow_attr->port - 1].rep) { + rep = dev->port[flow_attr->port - 1].rep; + if (!rep) { err = -EINVAL; goto free; } - misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, - misc_parameters); - MLX5_SET(fte_match_set_misc, misc, source_port, - dev->port[flow_attr->port - 1].rep->vport); - misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, - misc_parameters); - MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port); + + mlx5_ib_set_rule_source_port(dev, spec, rep); } spec->match_criteria_enable = get_match_criteria_enable(spec->match_criteria);