[net-next,RFC,v2,08/11] mlxsw: spectrum_dpipe: Connect dpipe tables to resources

Message ID 20171114161852.6633-9-jiri@resnulli.us
State RFC
Delegated to: David Miller
Headers show
Series
  • Add support for resource abstraction
Related show

Commit Message

Jiri Pirko Nov. 14, 2017, 4:18 p.m.
From: Arkadi Sharshevsky <arkadis@mellanox.com>

Connect current dpipe tables to resources. The tables are connected
in the following fashion:
1. IPv4 host - KVD hash single
2. IPv6 host - KVD hash double
3. Adjacency - KVD linear

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum_dpipe.c   | 72 ++++++++++++++++++----
 1 file changed, 60 insertions(+), 12 deletions(-)

Comments

David Ahern Nov. 18, 2017, 7:19 p.m. | #1
On 11/14/17 9:18 AM, Jiri Pirko wrote:
> From: Arkadi Sharshevsky <arkadis@mellanox.com>
> 
> Connect current dpipe tables to resources. The tables are connected
> in the following fashion:
> 1. IPv4 host - KVD hash single
> 2. IPv6 host - KVD hash double
> 3. Adjacency - KVD linear

Those descriptions would be helpful to the user. A description attribute
for the resources?
Arkadi Sharshevsky Nov. 19, 2017, 9:16 a.m. | #2
On 11/18/2017 09:19 PM, David Ahern wrote:
> On 11/14/17 9:18 AM, Jiri Pirko wrote:
>> From: Arkadi Sharshevsky <arkadis@mellanox.com>
>>
>> Connect current dpipe tables to resources. The tables are connected
>> in the following fashion:
>> 1. IPv4 host - KVD hash single
>> 2. IPv6 host - KVD hash double
>> 3. Adjacency - KVD linear
> 
> Those descriptions would be helpful to the user. A description attribute
> for the resources?
> 

As described in the cover letter this resources are used by the
majority of the ASICs lookup processes. So currently there is one
to one mapping but is should increase as more tables are exposed,
so I don't think its a good idea to maintain such an attribute.
David Ahern Nov. 19, 2017, 3:58 p.m. | #3
On 11/19/17 2:16 AM, Arkadi Sharshevsky wrote:
> 
> 
> On 11/18/2017 09:19 PM, David Ahern wrote:
>> On 11/14/17 9:18 AM, Jiri Pirko wrote:
>>> From: Arkadi Sharshevsky <arkadis@mellanox.com>
>>>
>>> Connect current dpipe tables to resources. The tables are connected
>>> in the following fashion:
>>> 1. IPv4 host - KVD hash single
>>> 2. IPv6 host - KVD hash double
>>> 3. Adjacency - KVD linear
>>
>> Those descriptions would be helpful to the user. A description attribute
>> for the resources?
>>
> 
> As described in the cover letter this resources are used by the
> majority of the ASICs lookup processes. So currently there is one
> to one mapping but is should increase as more tables are exposed,
> so I don't think its a good idea to maintain such an attribute.
> 

'IPv4 host' yes, but I mean the term 'KVD hash single'? Is it the same
across all h/w vendors? I have only seen that in the context of MLX. If
it is a MLX term then a description to the user that KVD hash single ==
IPv4 host is warranted.
Arkadi Sharshevsky Nov. 23, 2017, 1:40 p.m. | #4
On 11/19/2017 05:58 PM, David Ahern wrote:
> On 11/19/17 2:16 AM, Arkadi Sharshevsky wrote:
>>
>>
>> On 11/18/2017 09:19 PM, David Ahern wrote:
>>> On 11/14/17 9:18 AM, Jiri Pirko wrote:
>>>> From: Arkadi Sharshevsky <arkadis@mellanox.com>
>>>>
>>>> Connect current dpipe tables to resources. The tables are connected
>>>> in the following fashion:
>>>> 1. IPv4 host - KVD hash single
>>>> 2. IPv6 host - KVD hash double
>>>> 3. Adjacency - KVD linear
>>>
>>> Those descriptions would be helpful to the user. A description attribute
>>> for the resources?
>>>
>>
>> As described in the cover letter this resources are used by the
>> majority of the ASICs lookup processes. So currently there is one
>> to one mapping but is should increase as more tables are exposed,
>> so I don't think its a good idea to maintain such an attribute.
>>
> 
> 'IPv4 host' yes, but I mean the term 'KVD hash single'? Is it the same
> across all h/w vendors? I have only seen that in the context of MLX. If
> it is a MLX term then a description to the user that KVD hash single ==
> IPv4 host is warranted.
> 

But this relation is wrong, there is no equality here. The LPM, FDB and
VID to FID mapping are all can be modeled as lookup tables (via dpipe)
that use KVD hash single resource.

This description string will grow very long. I dont think this is the
right place to document such thing, eitherway, the user can dump the
dpipe tables and see which is mapped to what resource.
David Ahern Nov. 27, 2017, 4:12 p.m. | #5
On 11/23/17 6:40 AM, Arkadi Sharshevsky wrote:
> 
> 
> On 11/19/2017 05:58 PM, David Ahern wrote:
>> On 11/19/17 2:16 AM, Arkadi Sharshevsky wrote:
>>>
>>>
>>> On 11/18/2017 09:19 PM, David Ahern wrote:
>>>> On 11/14/17 9:18 AM, Jiri Pirko wrote:
>>>>> From: Arkadi Sharshevsky <arkadis@mellanox.com>
>>>>>
>>>>> Connect current dpipe tables to resources. The tables are connected
>>>>> in the following fashion:
>>>>> 1. IPv4 host - KVD hash single
>>>>> 2. IPv6 host - KVD hash double
>>>>> 3. Adjacency - KVD linear
>>>>
>>>> Those descriptions would be helpful to the user. A description attribute
>>>> for the resources?
>>>>
>>>
>>> As described in the cover letter this resources are used by the
>>> majority of the ASICs lookup processes. So currently there is one
>>> to one mapping but is should increase as more tables are exposed,
>>> so I don't think its a good idea to maintain such an attribute.
>>>
>>
>> 'IPv4 host' yes, but I mean the term 'KVD hash single'? Is it the same
>> across all h/w vendors? I have only seen that in the context of MLX. If
>> it is a MLX term then a description to the user that KVD hash single ==
>> IPv4 host is warranted.
>>
> 
> But this relation is wrong, there is no equality here. The LPM, FDB and
> VID to FID mapping are all can be modeled as lookup tables (via dpipe)
> that use KVD hash single resource.
> 
> This description string will grow very long. I dont think this is the
> right place to document such thing, eitherway, the user can dump the
> dpipe tables and see which is mapped to what resource.

Users should not have to find a PRM or user guide for *each version of
their hardware* to program something so fundamental. This is software.
We can make it user friendly. Use of vendor specific terms is fine --
allows correlation to vendor docs. But there should also be text to help
the user correlate vendor terms to generic industry terms.
Arkadi Sharshevsky Nov. 28, 2017, 10:04 a.m. | #6
On 11/27/2017 06:12 PM, David Ahern wrote:
> On 11/23/17 6:40 AM, Arkadi Sharshevsky wrote:
>>
>>
>> On 11/19/2017 05:58 PM, David Ahern wrote:
>>> On 11/19/17 2:16 AM, Arkadi Sharshevsky wrote:
>>>>
>>>>
>>>> On 11/18/2017 09:19 PM, David Ahern wrote:
>>>>> On 11/14/17 9:18 AM, Jiri Pirko wrote:
>>>>>> From: Arkadi Sharshevsky <arkadis@mellanox.com>
>>>>>>
>>>>>> Connect current dpipe tables to resources. The tables are connected
>>>>>> in the following fashion:
>>>>>> 1. IPv4 host - KVD hash single
>>>>>> 2. IPv6 host - KVD hash double
>>>>>> 3. Adjacency - KVD linear
>>>>>
>>>>> Those descriptions would be helpful to the user. A description attribute
>>>>> for the resources?
>>>>>
>>>>
>>>> As described in the cover letter this resources are used by the
>>>> majority of the ASICs lookup processes. So currently there is one
>>>> to one mapping but is should increase as more tables are exposed,
>>>> so I don't think its a good idea to maintain such an attribute.
>>>>
>>>
>>> 'IPv4 host' yes, but I mean the term 'KVD hash single'? Is it the same
>>> across all h/w vendors? I have only seen that in the context of MLX. If
>>> it is a MLX term then a description to the user that KVD hash single ==
>>> IPv4 host is warranted.
>>>
>>
>> But this relation is wrong, there is no equality here. The LPM, FDB and
>> VID to FID mapping are all can be modeled as lookup tables (via dpipe)
>> that use KVD hash single resource.
>>
>> This description string will grow very long. I dont think this is the
>> right place to document such thing, eitherway, the user can dump the
>> dpipe tables and see which is mapped to what resource.
> 
> Users should not have to find a PRM or user guide for *each version of
> their hardware* to program something so fundamental. This is software.

I still don't understand, you can dump the dpipe table to see which
tables use which resource. Why I need this redundant documentation string
in the kernel?

> We can make it user friendly. Use of vendor specific terms is fine --
> allows correlation to vendor docs. But there should also be text to help

IMHO such documentation strings should not be in the kernel.

> the user correlate vendor terms to generic industry terms.
> 

Really you want to add DEVLINK_RESOURCE_DESCRIPTION string attributed?
"
Used by the following hardware tables
- VID-to-FID
- LPM
- FDB
- HOST
...
"

Again I think it is redundant, just dump those tables. No need to use
user-guides nor PRM.
Jiri Pirko Nov. 28, 2017, 1:27 p.m. | #7
Mon, Nov 27, 2017 at 05:12:47PM CET, dsa@cumulusnetworks.com wrote:
>On 11/23/17 6:40 AM, Arkadi Sharshevsky wrote:
>> 
>> 
>> On 11/19/2017 05:58 PM, David Ahern wrote:
>>> On 11/19/17 2:16 AM, Arkadi Sharshevsky wrote:
>>>>
>>>>
>>>> On 11/18/2017 09:19 PM, David Ahern wrote:
>>>>> On 11/14/17 9:18 AM, Jiri Pirko wrote:
>>>>>> From: Arkadi Sharshevsky <arkadis@mellanox.com>
>>>>>>
>>>>>> Connect current dpipe tables to resources. The tables are connected
>>>>>> in the following fashion:
>>>>>> 1. IPv4 host - KVD hash single
>>>>>> 2. IPv6 host - KVD hash double
>>>>>> 3. Adjacency - KVD linear
>>>>>
>>>>> Those descriptions would be helpful to the user. A description attribute
>>>>> for the resources?
>>>>>
>>>>
>>>> As described in the cover letter this resources are used by the
>>>> majority of the ASICs lookup processes. So currently there is one
>>>> to one mapping but is should increase as more tables are exposed,
>>>> so I don't think its a good idea to maintain such an attribute.
>>>>
>>>
>>> 'IPv4 host' yes, but I mean the term 'KVD hash single'? Is it the same
>>> across all h/w vendors? I have only seen that in the context of MLX. If
>>> it is a MLX term then a description to the user that KVD hash single ==
>>> IPv4 host is warranted.
>>>
>> 
>> But this relation is wrong, there is no equality here. The LPM, FDB and
>> VID to FID mapping are all can be modeled as lookup tables (via dpipe)
>> that use KVD hash single resource.
>> 
>> This description string will grow very long. I dont think this is the
>> right place to document such thing, eitherway, the user can dump the
>> dpipe tables and see which is mapped to what resource.
>
>Users should not have to find a PRM or user guide for *each version of
>their hardware* to program something so fundamental. This is software.
>We can make it user friendly. Use of vendor specific terms is fine --
>allows correlation to vendor docs. But there should also be text to help
>the user correlate vendor terms to generic industry terms.

I have to be missing something. You can easily see the relation between
each dpipe table and resources already as a part of this patchset. The
string you suggest shows the same thing, therefore it is completely
redundant. What am I missing?
David Ahern Nov. 28, 2017, 4:05 p.m. | #8
On 11/28/17 6:27 AM, Jiri Pirko wrote:
> 
> I have to be missing something. You can easily see the relation between
> each dpipe table and resources already as a part of this patchset. The
> string you suggest shows the same thing, therefore it is completely
> redundant. What am I missing?
> 

At this point let's see a working RFC v3 and we can discuss from there.

Patch

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c
index 96fdba7..282fe82 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_dpipe.c
@@ -774,11 +774,27 @@  static struct devlink_dpipe_table_ops mlxsw_sp_host4_ops = {
 static int mlxsw_sp_dpipe_host4_table_init(struct mlxsw_sp *mlxsw_sp)
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
+	int err;
 
-	return devlink_dpipe_table_register(devlink,
-					    MLXSW_SP_DPIPE_TABLE_NAME_HOST4,
-					    &mlxsw_sp_host4_ops,
-					    mlxsw_sp, false);
+	err = devlink_dpipe_table_register(devlink,
+					   MLXSW_SP_DPIPE_TABLE_NAME_HOST4,
+					   &mlxsw_sp_host4_ops,
+					   mlxsw_sp, false);
+	if (err)
+		return err;
+
+	err = devlink_dpipe_table_resource_set(devlink,
+					       MLXSW_SP_DPIPE_TABLE_NAME_HOST4,
+					       MLXSW_SP_RESOURCE_KVD_HASH_SINGLE);
+	if (err)
+		goto err_resource_set;
+
+	return 0;
+
+err_resource_set:
+	devlink_dpipe_table_unregister(devlink,
+				       MLXSW_SP_DPIPE_TABLE_NAME_HOST4);
+	return err;
 }
 
 static void mlxsw_sp_dpipe_host4_table_fini(struct mlxsw_sp *mlxsw_sp)
@@ -832,11 +848,27 @@  static struct devlink_dpipe_table_ops mlxsw_sp_host6_ops = {
 static int mlxsw_sp_dpipe_host6_table_init(struct mlxsw_sp *mlxsw_sp)
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
+	int err;
 
-	return devlink_dpipe_table_register(devlink,
-					    MLXSW_SP_DPIPE_TABLE_NAME_HOST6,
-					    &mlxsw_sp_host6_ops,
-					    mlxsw_sp, false);
+	err = devlink_dpipe_table_register(devlink,
+					   MLXSW_SP_DPIPE_TABLE_NAME_HOST6,
+					   &mlxsw_sp_host6_ops,
+					   mlxsw_sp, false);
+	if (err)
+		return err;
+
+	err = devlink_dpipe_table_resource_set(devlink,
+					       MLXSW_SP_DPIPE_TABLE_NAME_HOST6,
+					       MLXSW_SP_RESOURCE_KVD_HASH_DOUBLE);
+	if (err)
+		goto err_resource_set;
+
+	return 0;
+
+err_resource_set:
+	devlink_dpipe_table_unregister(devlink,
+				       MLXSW_SP_DPIPE_TABLE_NAME_HOST6);
+	return err;
 }
 
 static void mlxsw_sp_dpipe_host6_table_fini(struct mlxsw_sp *mlxsw_sp)
@@ -1216,11 +1248,27 @@  static struct devlink_dpipe_table_ops mlxsw_sp_dpipe_table_adj_ops = {
 static int mlxsw_sp_dpipe_adj_table_init(struct mlxsw_sp *mlxsw_sp)
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
+	int err;
 
-	return devlink_dpipe_table_register(devlink,
-					    MLXSW_SP_DPIPE_TABLE_NAME_ADJ,
-					    &mlxsw_sp_dpipe_table_adj_ops,
-					    mlxsw_sp, false);
+	err = devlink_dpipe_table_register(devlink,
+					   MLXSW_SP_DPIPE_TABLE_NAME_ADJ,
+					   &mlxsw_sp_dpipe_table_adj_ops,
+					   mlxsw_sp, false);
+	if (err)
+		return err;
+
+	err = devlink_dpipe_table_resource_set(devlink,
+					       MLXSW_SP_DPIPE_TABLE_NAME_ADJ,
+					       MLXSW_SP_RESOURCE_KVD_LINEAR);
+	if (err)
+		goto err_resource_set;
+
+	return 0;
+
+err_resource_set:
+	devlink_dpipe_table_unregister(devlink,
+				       MLXSW_SP_DPIPE_TABLE_NAME_ADJ);
+	return err;
 }
 
 static void mlxsw_sp_dpipe_adj_table_fini(struct mlxsw_sp *mlxsw_sp)