| Message ID | 20250407-airoha-flowtable-l2b-v1-1-18777778e568@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add L2 hw acceleration for airoha_eth driver | expand |
On Mon, Apr 07, 2025 at 04:18:30PM +0200, Lorenzo Bianconi wrote: > Introduce l2_flows rhashtable in airoha_ppe struct in order to > store L2 flows committed by upper layers of the kernel. This is a > preliminary patch in order to offload L2 traffic rules. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> The patch logic and coding style looks OK to me. Just one question inline. Thanks, Michal > --- > drivers/net/ethernet/airoha/airoha_eth.h | 15 ++++++- > drivers/net/ethernet/airoha/airoha_ppe.c | 67 +++++++++++++++++++++++++++----- > 2 files changed, 72 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h > index ec8908f904c61988c3dc973e187596c49af139fb..57925648155b104021c10821096ba267c9c7cef6 100644 > --- a/drivers/net/ethernet/airoha/airoha_eth.h > +++ b/drivers/net/ethernet/airoha/airoha_eth.h > @@ -422,12 +422,23 @@ struct airoha_flow_data { > } pppoe; > }; > > +enum airoha_flow_entry_type { > + FLOW_TYPE_L4, I didn't find any usage of L4 flow type in the series. Is that reserved for future series? Shouldn't it be added together with its usage then? > + FLOW_TYPE_L2, > + FLOW_TYPE_L2_SUBFLOW, > +}; > +
> On Mon, Apr 07, 2025 at 04:18:30PM +0200, Lorenzo Bianconi wrote: > > Introduce l2_flows rhashtable in airoha_ppe struct in order to > > store L2 flows committed by upper layers of the kernel. This is a > > preliminary patch in order to offload L2 traffic rules. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > The patch logic and coding style looks OK to me. > Just one question inline. > > Thanks, > Michal > > > --- > > drivers/net/ethernet/airoha/airoha_eth.h | 15 ++++++- > > drivers/net/ethernet/airoha/airoha_ppe.c | 67 +++++++++++++++++++++++++++----- > > 2 files changed, 72 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h > > index ec8908f904c61988c3dc973e187596c49af139fb..57925648155b104021c10821096ba267c9c7cef6 100644 > > --- a/drivers/net/ethernet/airoha/airoha_eth.h > > +++ b/drivers/net/ethernet/airoha/airoha_eth.h > > @@ -422,12 +422,23 @@ struct airoha_flow_data { > > } pppoe; > > }; > > > > +enum airoha_flow_entry_type { > > + FLOW_TYPE_L4, > > I didn't find any usage of L4 flow type in the series. > Is that reserved for future series? Shouldn't it be added together with > its usage then? Hi Michal, FLOW_TYPE_L4 is equal to 0 so it is the default value for airoha_flow_table_entry type when not set explicitly. It is done this way to reduce code changes. Regards, Lorenzo > > > + FLOW_TYPE_L2, > > + FLOW_TYPE_L2_SUBFLOW, > > +}; > > +
On Tue, Apr 08, 2025 at 06:49:40PM +0200, Lorenzo Bianconi wrote: > > On Mon, Apr 07, 2025 at 04:18:30PM +0200, Lorenzo Bianconi wrote: > > > Introduce l2_flows rhashtable in airoha_ppe struct in order to > > > store L2 flows committed by upper layers of the kernel. This is a > > > preliminary patch in order to offload L2 traffic rules. > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > > The patch logic and coding style looks OK to me. > > Just one question inline. > > > > Thanks, > > Michal > > > > > --- > > > drivers/net/ethernet/airoha/airoha_eth.h | 15 ++++++- > > > drivers/net/ethernet/airoha/airoha_ppe.c | 67 +++++++++++++++++++++++++++----- > > > 2 files changed, 72 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h > > > index ec8908f904c61988c3dc973e187596c49af139fb..57925648155b104021c10821096ba267c9c7cef6 100644 > > > --- a/drivers/net/ethernet/airoha/airoha_eth.h > > > +++ b/drivers/net/ethernet/airoha/airoha_eth.h > > > @@ -422,12 +422,23 @@ struct airoha_flow_data { > > > } pppoe; > > > }; > > > > > > +enum airoha_flow_entry_type { > > > + FLOW_TYPE_L4, > > > > I didn't find any usage of L4 flow type in the series. > > Is that reserved for future series? Shouldn't it be added together with > > its usage then? > > Hi Michal, > > FLOW_TYPE_L4 is equal to 0 so it is the default value for > airoha_flow_table_entry type when not set explicitly. > It is done this way to reduce code changes. > > Regards, > Lorenzo > Thanks, Lorenzo! I'm OK with that. Regards, Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
On Tue, 8 Apr 2025 18:49:40 +0200 Lorenzo Bianconi wrote: > > I didn't find any usage of L4 flow type in the series. > > Is that reserved for future series? Shouldn't it be added together with > > its usage then? > > FLOW_TYPE_L4 is equal to 0 so it is the default value for > airoha_flow_table_entry type when not set explicitly. > It is done this way to reduce code changes. That seems quite unintuitive. Could you init explicitly for the benefit of people reading this code?
> On Tue, 8 Apr 2025 18:49:40 +0200 Lorenzo Bianconi wrote: > > > I didn't find any usage of L4 flow type in the series. > > > Is that reserved for future series? Shouldn't it be added together with > > > its usage then? > > > > FLOW_TYPE_L4 is equal to 0 so it is the default value for > > airoha_flow_table_entry type when not set explicitly. > > It is done this way to reduce code changes. > > That seems quite unintuitive. Could you init explicitly for the > benefit of people reading this code? ack, I will do in v2. Regards, Lorenzo
diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h index ec8908f904c61988c3dc973e187596c49af139fb..57925648155b104021c10821096ba267c9c7cef6 100644 --- a/drivers/net/ethernet/airoha/airoha_eth.h +++ b/drivers/net/ethernet/airoha/airoha_eth.h @@ -422,12 +422,23 @@ struct airoha_flow_data { } pppoe; }; +enum airoha_flow_entry_type { + FLOW_TYPE_L4, + FLOW_TYPE_L2, + FLOW_TYPE_L2_SUBFLOW, +}; + struct airoha_flow_table_entry { - struct hlist_node list; + union { + struct hlist_node list; + struct rhash_head l2_node; + }; struct airoha_foe_entry data; u32 hash; + enum airoha_flow_entry_type type; + struct rhash_head node; unsigned long cookie; }; @@ -480,6 +491,8 @@ struct airoha_ppe { void *foe; dma_addr_t foe_dma; + struct rhashtable l2_flows; + struct hlist_head *foe_flow; u16 foe_check_time[PPE_NUM_ENTRIES]; diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c index f10dab935cab6fad747fdfaa70b67903904c1703..aed4a22f3a8b8737f18509b48fc47eae594b9d5f 100644 --- a/drivers/net/ethernet/airoha/airoha_ppe.c +++ b/drivers/net/ethernet/airoha/airoha_ppe.c @@ -24,6 +24,13 @@ static const struct rhashtable_params airoha_flow_table_params = { .automatic_shrinking = true, }; +static const struct rhashtable_params airoha_l2_flow_table_params = { + .head_offset = offsetof(struct airoha_flow_table_entry, l2_node), + .key_offset = offsetof(struct airoha_flow_table_entry, data.bridge), + .key_len = 2 * ETH_ALEN, + .automatic_shrinking = true, +}; + static bool airoha_ppe2_is_enabled(struct airoha_eth *eth) { return airoha_fe_rr(eth, REG_PPE_GLO_CFG(1)) & PPE_GLO_CFG_EN_MASK; @@ -505,11 +512,36 @@ static void airoha_ppe_foe_insert_entry(struct airoha_ppe *ppe, u32 hash) spin_unlock_bh(&ppe_lock); } +static int +airoha_ppe_foe_l2_flow_commit_entry(struct airoha_ppe *ppe, + struct airoha_flow_table_entry *e) +{ + struct airoha_flow_table_entry *prev; + + e->type = FLOW_TYPE_L2; + prev = rhashtable_lookup_get_insert_fast(&ppe->l2_flows, &e->l2_node, + airoha_l2_flow_table_params); + if (!prev) + return 0; + + if (IS_ERR(prev)) + return PTR_ERR(prev); + + return rhashtable_replace_fast(&ppe->l2_flows, &prev->l2_node, + &e->l2_node, + airoha_l2_flow_table_params); +} + static int airoha_ppe_foe_flow_commit_entry(struct airoha_ppe *ppe, struct airoha_flow_table_entry *e) { - u32 hash = airoha_ppe_foe_get_entry_hash(&e->data); + int type = FIELD_GET(AIROHA_FOE_IB1_BIND_PACKET_TYPE, e->data.ib1); + u32 hash; + if (type == PPE_PKT_TYPE_BRIDGE) + return airoha_ppe_foe_l2_flow_commit_entry(ppe, e); + + hash = airoha_ppe_foe_get_entry_hash(&e->data); e->hash = 0xffff; spin_lock_bh(&ppe_lock); @@ -524,13 +556,18 @@ static void airoha_ppe_foe_flow_remove_entry(struct airoha_ppe *ppe, { spin_lock_bh(&ppe_lock); - hlist_del_init(&e->list); - if (e->hash != 0xffff) { - e->data.ib1 &= ~AIROHA_FOE_IB1_BIND_STATE; - e->data.ib1 |= FIELD_PREP(AIROHA_FOE_IB1_BIND_STATE, - AIROHA_FOE_STATE_INVALID); - airoha_ppe_foe_commit_entry(ppe, &e->data, e->hash); - e->hash = 0xffff; + if (e->type == FLOW_TYPE_L2) { + rhashtable_remove_fast(&ppe->l2_flows, &e->l2_node, + airoha_l2_flow_table_params); + } else { + hlist_del_init(&e->list); + if (e->hash != 0xffff) { + e->data.ib1 &= ~AIROHA_FOE_IB1_BIND_STATE; + e->data.ib1 |= FIELD_PREP(AIROHA_FOE_IB1_BIND_STATE, + AIROHA_FOE_STATE_INVALID); + airoha_ppe_foe_commit_entry(ppe, &e->data, e->hash); + e->hash = 0xffff; + } } spin_unlock_bh(&ppe_lock); @@ -890,9 +927,20 @@ int airoha_ppe_init(struct airoha_eth *eth) if (err) return err; + err = rhashtable_init(&ppe->l2_flows, &airoha_l2_flow_table_params); + if (err) + goto error_flow_table_destroy; + err = airoha_ppe_debugfs_init(ppe); if (err) - rhashtable_destroy(ð->flow_table); + goto error_l2_flow_table_destroy; + + return 0; + +error_l2_flow_table_destroy: + rhashtable_destroy(&ppe->l2_flows); +error_flow_table_destroy: + rhashtable_destroy(ð->flow_table); return err; } @@ -909,6 +957,7 @@ void airoha_ppe_deinit(struct airoha_eth *eth) } rcu_read_unlock(); + rhashtable_destroy(ð->ppe->l2_flows); rhashtable_destroy(ð->flow_table); debugfs_remove(eth->ppe->debugfs_dir); }
Introduce l2_flows rhashtable in airoha_ppe struct in order to store L2 flows committed by upper layers of the kernel. This is a preliminary patch in order to offload L2 traffic rules. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/net/ethernet/airoha/airoha_eth.h | 15 ++++++- drivers/net/ethernet/airoha/airoha_ppe.c | 67 +++++++++++++++++++++++++++----- 2 files changed, 72 insertions(+), 10 deletions(-)