henrik.bjoernlund at microchip.com
2020-Sep-15 08:23 UTC
[Bridge] [PATCH RFC 1/7] net: bridge: extend the process of special frames
Thanks for your review. I will update in the next version as suggested. Regards Henrik The 09/08/2020 12:12, Nikolay Aleksandrov wrote:> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, 2020-09-04 at 09:15 +0000, Henrik Bjoernlund wrote: > > This patch extends the processing of frames in the bridge. Currently MRP > > frames needs special processing and the current implementation doesn't > > allow a nice way to process different frame types. Therefore try to > > improve this by adding a list that contains frame types that need > > special processing. This list is iterated for each input frame and if > > there is a match based on frame type then these functions will be called > > and decide what to do with the frame. It can process the frame then the > > bridge doesn't need to do anything or don't process so then the bridge > > will do normal forwarding. > > > > Signed-off-by: Henrik Bjoernlund <henrik.bjoernlund at microchip.com> > > --- > > net/bridge/br_device.c | 1 + > > net/bridge/br_input.c | 31 ++++++++++++++++++++++++++++++- > > net/bridge/br_mrp.c | 19 +++++++++++++++---- > > net/bridge/br_private.h | 18 ++++++++++++------ > > 4 files changed, 58 insertions(+), 11 deletions(-) > > > > Hi, > First I must say I do like this approach, thanks for generalizing it. > You can switch to a hlist so that there's just 1 pointer in the head. > I don't think you need list when you're walking only in one direction. > A few more minor comments below. > > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > > index 9a2fb4aa1a10..a9232db03108 100644 > > --- a/net/bridge/br_device.c > > +++ b/net/bridge/br_device.c > > @@ -473,6 +473,7 @@ void br_dev_setup(struct net_device *dev) > > spin_lock_init(&br->lock); > > INIT_LIST_HEAD(&br->port_list); > > INIT_HLIST_HEAD(&br->fdb_list); > > + INIT_LIST_HEAD(&br->ftype_list); > > #if IS_ENABLED(CONFIG_BRIDGE_MRP) > > INIT_LIST_HEAD(&br->mrp_list); > > #endif > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > > index 59a318b9f646..0f475b21094c 100644 > > --- a/net/bridge/br_input.c > > +++ b/net/bridge/br_input.c > > @@ -254,6 +254,21 @@ static int nf_hook_bridge_pre(struct sk_buff *skb, struct sk_buff **pskb) > > return RX_HANDLER_CONSUMED; > > } > > > > +/* Return 0 if the frame was not processed otherwise 1 > > + * note: already called with rcu_read_lock > > + */ > > +static int br_process_frame_type(struct net_bridge_port *p, > > + struct sk_buff *skb) > > +{ > > + struct br_frame_type *tmp; > > + > > + list_for_each_entry_rcu(tmp, &p->br->ftype_list, list) { > > + if (unlikely(tmp->type == skb->protocol)) > > + return tmp->func(p, skb); > > + } > > Nit: you can drop the {}. > > > + return 0; > > +} > > + > > /* > > * Return NULL if skb is handled > > * note: already called with rcu_read_lock > > @@ -343,7 +358,7 @@ static rx_handler_result_t br_handle_frame(struct sk_buff **pskb) > > } > > } > > > > - if (unlikely(br_mrp_process(p, skb))) > > + if (unlikely(br_process_frame_type(p, skb))) > > return RX_HANDLER_PASS; > > > > forward: > > @@ -380,3 +395,17 @@ rx_handler_func_t *br_get_rx_handler(const struct net_device *dev) > > > > return br_handle_frame; > > } > > + > > +void br_add_frame(struct net_bridge *br, struct br_frame_type *ft) > > +{ > > + list_add_rcu(&ft->list, &br->ftype_list); > > +} > > + > > +void br_del_frame(struct net_bridge *br, struct br_frame_type *ft) > > +{ > > + struct br_frame_type *tmp; > > + > > + list_for_each_entry(tmp, &br->ftype_list, list) > > + if (ft == tmp) > > + list_del_rcu(&ft->list); > > +} > > diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c > > index b36689e6e7cb..0428e1785041 100644 > > --- a/net/bridge/br_mrp.c > > +++ b/net/bridge/br_mrp.c > > @@ -6,6 +6,13 @@ > > static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 }; > > static const u8 mrp_in_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 }; > > > > +static int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb); > > + > > +static struct br_frame_type mrp_frame_type __read_mostly = { > > + .type = cpu_to_be16(ETH_P_MRP), > > + .func = br_mrp_process, > > +}; > > + > > static bool br_mrp_is_ring_port(struct net_bridge_port *p_port, > > struct net_bridge_port *s_port, > > struct net_bridge_port *port) > > @@ -445,6 +452,9 @@ static void br_mrp_del_impl(struct net_bridge *br, struct br_mrp *mrp) > > > > list_del_rcu(&mrp->list); > > kfree_rcu(mrp, rcu); > > + > > + if (list_empty(&br->mrp_list)) > > + br_del_frame(br, &mrp_frame_type); > > } > > > > /* Adds a new MRP instance. > > @@ -493,6 +503,9 @@ int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance) > > spin_unlock_bh(&br->lock); > > rcu_assign_pointer(mrp->s_port, p); > > > > + if (list_empty(&br->mrp_list)) > > + br_add_frame(br, &mrp_frame_type); > > + > > INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired); > > INIT_DELAYED_WORK(&mrp->in_test_work, br_mrp_in_test_work_expired); > > list_add_tail_rcu(&mrp->list, &br->mrp_list); > > @@ -1172,15 +1185,13 @@ static int br_mrp_rcv(struct net_bridge_port *p, > > * normal forwarding. > > * note: already called with rcu_read_lock > > */ > > -int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb) > > +static int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb) > > { > > /* If there is no MRP instance do normal forwarding */ > > if (likely(!(p->flags & BR_MRP_AWARE))) > > goto out; > > > > - if (unlikely(skb->protocol == htons(ETH_P_MRP))) > > - return br_mrp_rcv(p, skb, p->dev); > > - > > + return br_mrp_rcv(p, skb, p->dev); > > out: > > return 0; > > } > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > > index baa1500f384f..e67c6d9e8bea 100644 > > --- a/net/bridge/br_private.h > > +++ b/net/bridge/br_private.h > > @@ -89,6 +89,13 @@ struct bridge_mcast_stats { > > }; > > #endif > > > > +struct br_frame_type { > > + __be16 type; > > + int (*func)(struct net_bridge_port *port, > > + struct sk_buff *skb); > > Perhaps frame_handler or something similar would be a better name for the > callback ? > > > + struct list_head list; > > +}; > > + > > struct br_vlan_stats { > > u64 rx_bytes; > > u64 rx_packets; > > @@ -433,6 +440,8 @@ struct net_bridge { > > #endif > > struct hlist_head fdb_list; > > > > + struct list_head ftype_list; > > Could you please expand this to frame_type_list ? > > > + > > #if IS_ENABLED(CONFIG_BRIDGE_MRP) > > struct list_head mrp_list; > > #endif > > @@ -708,6 +717,9 @@ int nbp_backup_change(struct net_bridge_port *p, struct net_device *backup_dev); > > int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb); > > rx_handler_func_t *br_get_rx_handler(const struct net_device *dev); > > > > +void br_add_frame(struct net_bridge *br, struct br_frame_type *ft); > > +void br_del_frame(struct net_bridge *br, struct br_frame_type *ft); > > + > > static inline bool br_rx_handler_check_rcu(const struct net_device *dev) > > { > > return rcu_dereference(dev->rx_handler) == br_get_rx_handler(dev); > > @@ -1320,7 +1332,6 @@ extern int (*br_fdb_test_addr_hook)(struct net_device *dev, unsigned char *addr) > > #if IS_ENABLED(CONFIG_BRIDGE_MRP) > > int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p, > > struct nlattr *attr, int cmd, struct netlink_ext_ack *extack); > > -int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb); > > bool br_mrp_enabled(struct net_bridge *br); > > void br_mrp_port_del(struct net_bridge *br, struct net_bridge_port *p); > > int br_mrp_fill_info(struct sk_buff *skb, struct net_bridge *br); > > @@ -1332,11 +1343,6 @@ static inline int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p, > > return -EOPNOTSUPP; > > } > > > > -static inline int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb) > > -{ > > - return 0; > > -} > > - > > static inline bool br_mrp_enabled(struct net_bridge *br) > > { > > return false; >-- /Henrik