net: replace hooks in __netif_receive_skb V2
Subsystems (3)
PATCH 1/1 net: replace hooks in __netif_receive_skb V2
changelog:v1->v2
- writers are locked by rtnl_lock (removed unnecessary spinlock)
- using call_rcu in unregister
- nicer macvlan_port_create cleanup
- struct rx_hanler is made const in funtion parameters
What this patch does is it removes two receive frame hooks (for bridge and for macvlan) from __netif_receive_skb. These are replaced them with a general list of rx_handlers which is iterated thru instead.
Then a network driver (of virtual netdev like macvlan or bridge) can register an rx_handler for needed net device.
--- drivers/net/macvlan.c | 27 ++++++-- include/linux/if_bridge.h | 2 - include/linux/if_macvlan.h | 4 - include/linux/netdevice.h | 18 +++++ net/bridge/br.c | 2 - net/bridge/br_if.c | 14 ++++ net/bridge/br_input.c | 12 +++- net/bridge/br_private.h | 3 +- net/core/dev.c | 160 ++++++++++++++++++++++++++------------------ 9 files changed, 160 insertions(+), 82 deletions(-)
drivers/net/macvlan.c +22 -5
@@ -145,15 +145,16 @@ static void macvlan_broadcast(struct sk_buff *skb, }/* called under rcu_read_lock() from netif_receive_skb */ -static struct sk_buff *macvlan_handle_frame (struct macvlan_port *port, -struct sk_buff *skb) +static struct sk_buff *macvlan_handle_frame (struct sk_buff *skb) { +struct macvlan_port *port;const struct ethhdr *eth =eth_hdr (skb);const struct macvlan_dev *vlan;const struct macvlan_dev *src;struct net_device *dev;unsigned int len; + port =rcu_dereference (skb->dev->macvlan_port);if (is_multicast_ether_addr (eth->h_dest)) { src =macvlan_hash_lookup (port, eth->h_source);if (!src)
@@ -511,10 +512,16 @@ static void macvlan_setup(struct net_device *dev) dev->tx_queue_len =0 ; } +static const struct netdev_rx_handler macvlan_rx_handler = { + .order =NETDEV_RX_HANDLER_ORDER_MACVLAN , + .callback = macvlan_handle_frame, +}; +static int macvlan_port_create (struct net_device *dev) {struct macvlan_port *port;unsigned int i; +int err;if (dev->type !=ARPHRD_ETHER || dev->flags &IFF_LOOPBACK )return -EINVAL ;
@@ -528,13 +535,26 @@ static int macvlan_port_create(struct net_device *dev)for (i =0 ; i <MACVLAN_HASH_SIZE ; i++)INIT_HLIST_HEAD (&port->vlan_hash[i]);rcu_assign_pointer (dev->macvlan_port, port); + + err =netdev_rx_handler_register (dev, &macvlan_rx_handler); +if (err) + goto cleanup; +return 0 ; + +cleanup: +rcu_assign_pointer (dev->macvlan_port,NULL ); +synchronize_rcu (); +kfree (port); + +return err; }static void macvlan_port_destroy (struct net_device *dev) {struct macvlan_port *port = dev->macvlan_port; +netdev_rx_handler_unregister (dev, &macvlan_rx_handler);rcu_assign_pointer (dev->macvlan_port,NULL );synchronize_rcu ();kfree (port);
@@ -767,14 +787,12 @@ static int __init macvlan_init_module(void)int err;register_netdevice_notifier (&macvlan_notifier_block); - macvlan_handle_frame_hook = macvlan_handle_frame; err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ; err1: - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block);return err; }
@@ -782,7 +800,6 @@ err1: static void __exit macvlan_cleanup_module(void) {rtnl_link_unregister (&macvlan_link_ops); - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block); }
include/linux/if_bridge.h +0 -2
@@ -102,8 +102,6 @@ struct __fdb_entry {#include <linux/netdevice.h> extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *)); -extern struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb);extern int (*br_should_route_hook)(struct sk_buff *skb);#endif
include/linux/if_macvlan.h +0 -4
@@ -84,8 +84,4 @@ extern int macvlan_link_register(struct rtnl_link_ops *ops);extern netdev_tx_t macvlan_start_xmit (struct sk_buff *skb,struct net_device *dev); - -extern struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *, -struct sk_buff *); -#endif /* _LINUX_IF_MACVLAN_H */
include/linux/netdevice.h +18 -0
@@ -254,6 +254,16 @@ struct netdev_hw_addr_list {#define netdev_for_each_mc_addr (ha, dev) \ netdev_hw_addr_list_for_each(ha, &(dev)->mc) + +struct netdev_rx_handler { +struct list_head list; +unsigned int order; +#define NETDEV_RX_HANDLER_ORDER_BRIDGE 1 +#define NETDEV_RX_HANDLER_ORDER_MACVLAN 2 +struct sk_buff *(*callback)(struct sk_buff *skb); +struct rcu_head rcu; +}; +struct hh_cache {struct hh_cache *hh_next;/* Next entry */ atomic_t hh_refcnt;/* number of users */
@@ -1031,6 +1041,9 @@ struct net_device {/* GARP */ struct garp_port *garp_port; +/* receive handlers (hooks) list */ +struct list_head rx_handlers; +/* class/net/name entry */ struct device dev;/* space for optional device, statistics, and wireless sysfs groups */
@@ -1685,6 +1698,11 @@ static inline void napi_free_frags(struct napi_struct *napi) napi->skb =NULL ; } +extern int netdev_rx_handler_register (struct net_device *dev, +const struct netdev_rx_handler *rh); +extern void netdev_rx_handler_unregister (struct net_device *dev, +const struct netdev_rx_handler *rh); +extern void netif_nit_deliver (struct sk_buff *skb);extern int dev_valid_name (const char *name);extern int dev_ioctl (struct net *net,unsigned int cmd,void __user *);
net/bridge/br.c +0 -2
@@ -63,7 +63,6 @@ static int __init br_init(void) goto err_out4;brioctl_set (br_ioctl_deviceless_stub); - br_handle_frame_hook = br_handle_frame;#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) br_fdb_test_addr_hook = br_fdb_test_addr;
@@ -100,7 +99,6 @@ static void __exit br_deinit(void) br_fdb_test_addr_hook =NULL ;#endif - br_handle_frame_hook =NULL ;br_fdb_fini (); }
net/bridge/br_if.c +14 -0
@@ -119,6 +119,11 @@ static void destroy_nbp_rcu(struct rcu_head *head)destroy_nbp (p); } +static const struct netdev_rx_handler br_rx_handler = { + .order =NETDEV_RX_HANDLER_ORDER_BRIDGE , + .callback = br_handle_frame, +}; +/* Delete port(interface) from bridge is done in two steps. * via RCU. First step, marks device as down. That deletes * all the timers and stops new packets from flowing through.
@@ -147,6 +152,7 @@ static void del_nbp(struct net_bridge_port *p)list_del_rcu (&p->list); +netdev_rx_handler_unregister (dev, &br_rx_handler);rcu_assign_pointer (dev->br_port,NULL );br_multicast_del_port (p);
@@ -429,6 +435,11 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) goto err2;rcu_assign_pointer (dev->br_port, p); + + err =netdev_rx_handler_register (dev, &br_rx_handler); +if (err) + goto err3; +dev_disable_lro (dev);list_add_rcu (&p->list, &br->port_list);
@@ -451,6 +462,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)br_netpoll_enable (br, dev);return 0 ; +err3: +rcu_assign_pointer (dev->br_port,NULL ); +synchronize_rcu (); err2:br_fdb_delete_by_port (br, p,1 ); err1:
net/bridge/br_input.c +9 -3
@@ -131,15 +131,19 @@ static inline int is_link_local(const unsigned char *dest) }/* -* Called via br_handle_frame_hook. * Return NULL if skb is handled -* note: already called with rcu_read_lock (preempt_disabled) +* note: already called with rcu_read_lock (preempt_disabled) from +* netif_receive_skb */ -struct sk_buff *br_handle_frame (struct net_bridge_port *p,struct sk_buff *skb) +struct sk_buff *br_handle_frame (struct sk_buff *skb) { +struct net_bridge_port *p;const unsigned char *dest =eth_hdr (skb)->h_dest;int (*rhook)(struct sk_buff *skb); +if (skb->pkt_type ==PACKET_LOOPBACK ) +return skb; +if (!is_valid_ether_addr (eth_hdr (skb)->h_source)) goto drop;
@@ -147,6 +151,8 @@ struct sk_buff *br_handle_frame(struct net_bridge_port *p, struct sk_buff *skb)if (!skb)return NULL ; + p =rcu_dereference (skb->dev->br_port); +if (unlikely (is_link_local (dest))) {/* Pause frames shouldn't be passed up by driver anyway */ if (skb->protocol ==htons (ETH_P_PAUSE ))
net/bridge/br_private.h +1 -2
@@ -331,8 +331,7 @@ extern void br_features_recompute(struct net_bridge *br);/* br_input.c */ extern int br_handle_frame_finish (struct sk_buff *skb); -extern struct sk_buff *br_handle_frame (struct net_bridge_port *p, -struct sk_buff *skb); +extern struct sk_buff *br_handle_frame (struct sk_buff *skb);/* br_ioctl.c */ extern int br_dev_ioctl (struct net_device *dev,struct ifreq *rq,int cmd);
net/core/dev.c +96 -64
196 lines not on this page · raw message
Signature · 1 line
1.6.6.1
Quoted 7 lines · On Thu, 27 May 2010 20:08:24 +0200 Jiri Pirko <[email protected]> wrote:
changelog:
v1->v2
- writers are locked byrtnl_lock(removed unnecessary spinlock)
- usingcall_rcuin unregister
- nicermacvlan_port_createcleanup
- structrx_hanleris made const in funtion parameters
Quoted 8 lines
What this patch does is it removes two receive frame hooks (for bridge and for macvlan) from
__netif_receive_skb. These are replaced them with a general list ofrx_handlerswhich is iterated thru instead.Then a network driver (of virtual netdev like macvlan or bridge) can register an
rx_handlerfor needed net device.Signed-off-by Jiri Pirko <[email protected]>
I wonder if we really need the chaining. What about allowing only one hook per device (and return -EBUSY)? That also gets rid of the allocation, and the extra overhead.
The handler hook, has to be export GPL, since we really don't want more network stack abuse by 3rd parties.
Quoted 14 lines · Thu, May 27, 2010 at 10:08:22PM CEST, [email protected] wrote:
Quoted 11 lines · On Thu, 27 May 2010 20:08:24 +0200 Jiri Pirko <[email protected]> wrote:
changelog:
v1->v2
- writers are locked byrtnl_lock(removed unnecessary spinlock)
- usingcall_rcuin unregister
- nicermacvlan_port_createcleanup
- structrx_hanleris made const in funtion parametersWhat this patch does is it removes two receive frame hooks (for bridge and for macvlan) from
__netif_receive_skb. These are replaced them with a general list ofrx_handlerswhich is iterated thru instead.
Quoted 8 lines
Quoted 4 lines
Then a network driver (of virtual netdev like macvlan or bridge) can register an
rx_handlerfor needed net device.Signed-off-by Jiri Pirko <[email protected]>I wonder if we really need the chaining. What about allowing only one hook per device (and return
-EBUSY)? That also gets rid of the allocation, and the extra overhead.
Hmm, that's a good question. I thought about it already. But I'm also wondering if there is a possible scenario, when the chaining can be necessary. Also I do not see any -significant- downside of having multiple handlers in a list, so I would probably go this way now. Opinions?
Quoted 3 lines
The handler hook, has to be export GPL, since we really don't want more network stack abuse by 3rd parties.
Noted, will be in the next patch version.
Jirka
PATCH 1/1 net: replace hooks in __netif_receive_skb V3
changelog:v2->v3
- used GPL-ed exportsv1->v2
- writers are locked by rtnl_lock (removed unnecessary spinlock)
- using call_rcu in unregister
- nicer macvlan_port_create cleanup
- struct rx_hanler is made const in funtion parameters
What this patch does is it removes two receive frame hooks (for bridge and for macvlan) from __netif_receive_skb. These are replaced them with a general list of rx_handlers which is iterated thru instead.
Then a network driver (of virtual netdev like macvlan or bridge) can register an rx_handler for needed net device.
--- drivers/net/macvlan.c | 27 ++++++-- include/linux/if_bridge.h | 2 - include/linux/if_macvlan.h | 4 - include/linux/netdevice.h | 18 +++++ net/bridge/br.c | 2 - net/bridge/br_if.c | 14 ++++ net/bridge/br_input.c | 12 +++- net/bridge/br_private.h | 3 +- net/core/dev.c | 160 ++++++++++++++++++++++++++------------------ 9 files changed, 160 insertions(+), 82 deletions(-)
drivers/net/macvlan.c +22 -5
@@ -145,15 +145,16 @@ static void macvlan_broadcast(struct sk_buff *skb, }/* called under rcu_read_lock() from netif_receive_skb */ -static struct sk_buff *macvlan_handle_frame (struct macvlan_port *port, -struct sk_buff *skb) +static struct sk_buff *macvlan_handle_frame (struct sk_buff *skb) { +struct macvlan_port *port;const struct ethhdr *eth =eth_hdr (skb);const struct macvlan_dev *vlan;const struct macvlan_dev *src;struct net_device *dev;unsigned int len; + port =rcu_dereference (skb->dev->macvlan_port);if (is_multicast_ether_addr (eth->h_dest)) { src =macvlan_hash_lookup (port, eth->h_source);if (!src)
@@ -511,10 +512,16 @@ static void macvlan_setup(struct net_device *dev) dev->tx_queue_len =0 ; } +static const struct netdev_rx_handler macvlan_rx_handler = { + .order =NETDEV_RX_HANDLER_ORDER_MACVLAN , + .callback = macvlan_handle_frame, +}; +static int macvlan_port_create (struct net_device *dev) {struct macvlan_port *port;unsigned int i; +int err;if (dev->type !=ARPHRD_ETHER || dev->flags &IFF_LOOPBACK )return -EINVAL ;
@@ -528,13 +535,26 @@ static int macvlan_port_create(struct net_device *dev)for (i =0 ; i <MACVLAN_HASH_SIZE ; i++)INIT_HLIST_HEAD (&port->vlan_hash[i]);rcu_assign_pointer (dev->macvlan_port, port); + + err =netdev_rx_handler_register (dev, &macvlan_rx_handler); +if (err) + goto cleanup; +return 0 ; + +cleanup: +rcu_assign_pointer (dev->macvlan_port,NULL ); +synchronize_rcu (); +kfree (port); + +return err; }static void macvlan_port_destroy (struct net_device *dev) {struct macvlan_port *port = dev->macvlan_port; +netdev_rx_handler_unregister (dev, &macvlan_rx_handler);rcu_assign_pointer (dev->macvlan_port,NULL );synchronize_rcu ();kfree (port);
@@ -767,14 +787,12 @@ static int __init macvlan_init_module(void)int err;register_netdevice_notifier (&macvlan_notifier_block); - macvlan_handle_frame_hook = macvlan_handle_frame; err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ; err1: - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block);return err; }
@@ -782,7 +800,6 @@ err1: static void __exit macvlan_cleanup_module(void) {rtnl_link_unregister (&macvlan_link_ops); - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block); }
include/linux/if_bridge.h +0 -2
@@ -102,8 +102,6 @@ struct __fdb_entry {#include <linux/netdevice.h> extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *)); -extern struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb);extern int (*br_should_route_hook)(struct sk_buff *skb);#endif
include/linux/if_macvlan.h +0 -4
@@ -84,8 +84,4 @@ extern int macvlan_link_register(struct rtnl_link_ops *ops);extern netdev_tx_t macvlan_start_xmit (struct sk_buff *skb,struct net_device *dev); - -extern struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *, -struct sk_buff *); -#endif /* _LINUX_IF_MACVLAN_H */
include/linux/netdevice.h +18 -0
@@ -254,6 +254,16 @@ struct netdev_hw_addr_list {#define netdev_for_each_mc_addr (ha, dev) \ netdev_hw_addr_list_for_each(ha, &(dev)->mc) + +struct netdev_rx_handler { +struct list_head list; +unsigned int order; +#define NETDEV_RX_HANDLER_ORDER_BRIDGE 1 +#define NETDEV_RX_HANDLER_ORDER_MACVLAN 2 +struct sk_buff *(*callback)(struct sk_buff *skb); +struct rcu_head rcu; +}; +struct hh_cache {struct hh_cache *hh_next;/* Next entry */ atomic_t hh_refcnt;/* number of users */
@@ -1031,6 +1041,9 @@ struct net_device {/* GARP */ struct garp_port *garp_port; +/* receive handlers (hooks) list */ +struct list_head rx_handlers; +/* class/net/name entry */ struct device dev;/* space for optional device, statistics, and wireless sysfs groups */
@@ -1685,6 +1698,11 @@ static inline void napi_free_frags(struct napi_struct *napi) napi->skb =NULL ; } +extern int netdev_rx_handler_register (struct net_device *dev, +const struct netdev_rx_handler *rh); +extern void netdev_rx_handler_unregister (struct net_device *dev, +const struct netdev_rx_handler *rh); +extern void netif_nit_deliver (struct sk_buff *skb);extern int dev_valid_name (const char *name);extern int dev_ioctl (struct net *net,unsigned int cmd,void __user *);
net/bridge/br.c +0 -2
@@ -63,7 +63,6 @@ static int __init br_init(void) goto err_out4;brioctl_set (br_ioctl_deviceless_stub); - br_handle_frame_hook = br_handle_frame;#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) br_fdb_test_addr_hook = br_fdb_test_addr;
@@ -100,7 +99,6 @@ static void __exit br_deinit(void) br_fdb_test_addr_hook =NULL ;#endif - br_handle_frame_hook =NULL ;br_fdb_fini (); }
net/bridge/br_if.c +14 -0
@@ -119,6 +119,11 @@ static void destroy_nbp_rcu(struct rcu_head *head)destroy_nbp (p); } +static const struct netdev_rx_handler br_rx_handler = { + .order =NETDEV_RX_HANDLER_ORDER_BRIDGE , + .callback = br_handle_frame, +}; +/* Delete port(interface) from bridge is done in two steps. * via RCU. First step, marks device as down. That deletes * all the timers and stops new packets from flowing through.
@@ -147,6 +152,7 @@ static void del_nbp(struct net_bridge_port *p)list_del_rcu (&p->list); +netdev_rx_handler_unregister (dev, &br_rx_handler);rcu_assign_pointer (dev->br_port,NULL );br_multicast_del_port (p);
@@ -429,6 +435,11 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) goto err2;rcu_assign_pointer (dev->br_port, p); + + err =netdev_rx_handler_register (dev, &br_rx_handler); +if (err) + goto err3; +dev_disable_lro (dev);list_add_rcu (&p->list, &br->port_list);
@@ -451,6 +462,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)br_netpoll_enable (br, dev);return 0 ; +err3: +rcu_assign_pointer (dev->br_port,NULL ); +synchronize_rcu (); err2:br_fdb_delete_by_port (br, p,1 ); err1:
net/bridge/br_input.c +9 -3
@@ -131,15 +131,19 @@ static inline int is_link_local(const unsigned char *dest) }/* -* Called via br_handle_frame_hook. * Return NULL if skb is handled -* note: already called with rcu_read_lock (preempt_disabled) +* note: already called with rcu_read_lock (preempt_disabled) from +* netif_receive_skb */ -struct sk_buff *br_handle_frame (struct net_bridge_port *p,struct sk_buff *skb) +struct sk_buff *br_handle_frame (struct sk_buff *skb) { +struct net_bridge_port *p;const unsigned char *dest =eth_hdr (skb)->h_dest;int (*rhook)(struct sk_buff *skb); +if (skb->pkt_type ==PACKET_LOOPBACK ) +return skb; +if (!is_valid_ether_addr (eth_hdr (skb)->h_source)) goto drop;
@@ -147,6 +151,8 @@ struct sk_buff *br_handle_frame(struct net_bridge_port *p, struct sk_buff *skb)if (!skb)return NULL ; + p =rcu_dereference (skb->dev->br_port); +if (unlikely (is_link_local (dest))) {/* Pause frames shouldn't be passed up by driver anyway */ if (skb->protocol ==htons (ETH_P_PAUSE ))
net/bridge/br_private.h +1 -2
@@ -331,8 +331,7 @@ extern void br_features_recompute(struct net_bridge *br);/* br_input.c */ extern int br_handle_frame_finish (struct sk_buff *skb); -extern struct sk_buff *br_handle_frame (struct net_bridge_port *p, -struct sk_buff *skb); +extern struct sk_buff *br_handle_frame (struct sk_buff *skb);/* br_ioctl.c */ extern int br_dev_ioctl (struct net_device *dev,struct ifreq *rq,int cmd);
net/core/dev.c +96 -64
196 lines not on this page · raw message
Signature · 1 line
1.6.6.1
Re: [PATCH net-next-2.6] net: replace hooks in __netif_receive_skb V3
Quoted 32 lines · Jiri Pirko
include/linux/netdevice.h +13
-0@@ -254,6 +254,16 @@ struct netdev_hw_addr_list {#define netdev_for_each_mc_addr (ha, dev) \ netdev_hw_addr_list_for_each(ha, &(dev)->mc) + +struct netdev_rx_handler { +struct list_head list; +unsigned int order; +#define NETDEV_RX_HANDLER_ORDER_BRIDGE 1 +#define NETDEV_RX_HANDLER_ORDER_MACVLAN 2 +struct sk_buff *(*callback)(struct sk_buff *skb); +struct rcu_head rcu; +}; +struct hh_cache {struct hh_cache *hh_next;/* Next entry */ atomic_t hh_refcnt;/* number of users */ @@ -1031,6 +1041,9 @@ struct net_device {/* GARP */ struct garp_port *garp_port; +/* receive handlers (hooks) list */ +struct list_head rx_handlers; +/* class/net/name entry */ struct device dev;/* space for optional device, statistics, and wireless sysfs groups */
include/linux/netdevice.h static inline void napi_free_frags(struct napi_struct *napi) · line 1700
Quoted 3 lines
include/linux/netdevice.h +0
-0@@ -1685,6 +1698,11 @@ static inline void napi_free_frags(struct napi_struct *napi) napi->skb =NULL ; }
Please chose another place to hold rx_handlers, to keep rx path memory needs to minimum cache lines. Somewhere in the following section :
/*
* Cache line mostly used on receive path (including eth_type_trans()) */
unsigned long last_rx; /* Time of last Rx */
/* Interface address info used in eth_type_trans() */
unsigned char *dev_addr; /* hw address, (before bcast
because most packets are unicast) */
struct netdev_hw_addr_list dev_addrs; /* list of device
hw addresses */
unsigned char broadcast[MAX_ADDR_LEN]; /* hw bcast add */
#ifdef CONFIG_RPS
struct kset *queues_kset;
struct netdev_rx_queue *_rx;
/* Number of RX queues allocated at alloc_netdev_mq() time */
unsigned int num_rx_queues;#endif
struct netdev_queue rx_queue;
and before the :
struct netdev_queue *_tx ____cacheline_aligned_in_smp;
PATCH 1/1 net: replace hooks in __netif_receive_skb V4
changelog:v3->v4
- moved rx_handlers in net_device structure to be near to other data used in rx pathv2->v3
- used GPL-ed exportsv1->v2
- writers are locked by rtnl_lock (removed unnecessary spinlock)
- using call_rcu in unregister
- nicer macvlan_port_create cleanup
- struct rx_hanler is made const in funtion parameters
What this patch does is it removes two receive frame hooks (for bridge and for macvlan) from __netif_receive_skb. These are replaced them with a general list of rx_handlers which is iterated thru instead.
Then a network driver (of virtual netdev like macvlan or bridge) can register an rx_handler for needed net device.
--- drivers/net/macvlan.c | 27 ++++++-- include/linux/if_bridge.h | 2 - include/linux/if_macvlan.h | 4 - include/linux/netdevice.h | 18 +++++ net/bridge/br.c | 2 - net/bridge/br_if.c | 14 ++++ net/bridge/br_input.c | 12 +++- net/bridge/br_private.h | 3 +- net/core/dev.c | 160 ++++++++++++++++++++++++++------------------ 9 files changed, 160 insertions(+), 82 deletions(-)
drivers/net/macvlan.c +22 -5
@@ -145,15 +145,16 @@ static void macvlan_broadcast(struct sk_buff *skb, }/* called under rcu_read_lock() from netif_receive_skb */ -static struct sk_buff *macvlan_handle_frame (struct macvlan_port *port, -struct sk_buff *skb) +static struct sk_buff *macvlan_handle_frame (struct sk_buff *skb) { +struct macvlan_port *port;const struct ethhdr *eth =eth_hdr (skb);const struct macvlan_dev *vlan;const struct macvlan_dev *src;struct net_device *dev;unsigned int len; + port =rcu_dereference (skb->dev->macvlan_port);if (is_multicast_ether_addr (eth->h_dest)) { src =macvlan_hash_lookup (port, eth->h_source);if (!src)
@@ -511,10 +512,16 @@ static void macvlan_setup(struct net_device *dev) dev->tx_queue_len =0 ; } +static const struct netdev_rx_handler macvlan_rx_handler = { + .order =NETDEV_RX_HANDLER_ORDER_MACVLAN , + .callback = macvlan_handle_frame, +}; +static int macvlan_port_create (struct net_device *dev) {struct macvlan_port *port;unsigned int i; +int err;if (dev->type !=ARPHRD_ETHER || dev->flags &IFF_LOOPBACK )return -EINVAL ;
@@ -528,13 +535,26 @@ static int macvlan_port_create(struct net_device *dev)for (i =0 ; i <MACVLAN_HASH_SIZE ; i++)INIT_HLIST_HEAD (&port->vlan_hash[i]);rcu_assign_pointer (dev->macvlan_port, port); + + err =netdev_rx_handler_register (dev, &macvlan_rx_handler); +if (err) + goto cleanup; +return 0 ; + +cleanup: +rcu_assign_pointer (dev->macvlan_port,NULL ); +synchronize_rcu (); +kfree (port); + +return err; }static void macvlan_port_destroy (struct net_device *dev) {struct macvlan_port *port = dev->macvlan_port; +netdev_rx_handler_unregister (dev, &macvlan_rx_handler);rcu_assign_pointer (dev->macvlan_port,NULL );synchronize_rcu ();kfree (port);
@@ -767,14 +787,12 @@ static int __init macvlan_init_module(void)int err;register_netdevice_notifier (&macvlan_notifier_block); - macvlan_handle_frame_hook = macvlan_handle_frame; err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ; err1: - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block);return err; }
@@ -782,7 +800,6 @@ err1: static void __exit macvlan_cleanup_module(void) {rtnl_link_unregister (&macvlan_link_ops); - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block); }
include/linux/if_bridge.h +0 -2
@@ -102,8 +102,6 @@ struct __fdb_entry {#include <linux/netdevice.h> extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *)); -extern struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb);extern int (*br_should_route_hook)(struct sk_buff *skb);#endif
include/linux/if_macvlan.h +0 -4
@@ -84,8 +84,4 @@ extern int macvlan_link_register(struct rtnl_link_ops *ops);extern netdev_tx_t macvlan_start_xmit (struct sk_buff *skb,struct net_device *dev); - -extern struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *, -struct sk_buff *); -#endif /* _LINUX_IF_MACVLAN_H */
include/linux/netdevice.h +18 -0
@@ -254,6 +254,16 @@ struct netdev_hw_addr_list {#define netdev_for_each_mc_addr (ha, dev) \ netdev_hw_addr_list_for_each(ha, &(dev)->mc) + +struct netdev_rx_handler { +struct list_head list; +unsigned int order; +#define NETDEV_RX_HANDLER_ORDER_BRIDGE 1 +#define NETDEV_RX_HANDLER_ORDER_MACVLAN 2 +struct sk_buff *(*callback)(struct sk_buff *skb); +struct rcu_head rcu; +}; +struct hh_cache {struct hh_cache *hh_next;/* Next entry */ atomic_t hh_refcnt;/* number of users */
@@ -958,6 +968,9 @@ struct net_device {struct netdev_queue rx_queue; +/* receive handlers (hooks) list */ +struct list_head rx_handlers; +struct netdev_queue *_tx____cacheline_aligned_in_smp ;/* Number of TX queues allocated at alloc_netdev_mq() time */
@@ -1685,6 +1698,11 @@ static inline void napi_free_frags(struct napi_struct *napi) napi->skb =NULL ; } +extern int netdev_rx_handler_register (struct net_device *dev, +const struct netdev_rx_handler *rh); +extern void netdev_rx_handler_unregister (struct net_device *dev, +const struct netdev_rx_handler *rh); +extern void netif_nit_deliver (struct sk_buff *skb);extern int dev_valid_name (const char *name);extern int dev_ioctl (struct net *net,unsigned int cmd,void __user *);
net/bridge/br.c +0 -2
@@ -63,7 +63,6 @@ static int __init br_init(void) goto err_out4;brioctl_set (br_ioctl_deviceless_stub); - br_handle_frame_hook = br_handle_frame;#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) br_fdb_test_addr_hook = br_fdb_test_addr;
@@ -100,7 +99,6 @@ static void __exit br_deinit(void) br_fdb_test_addr_hook =NULL ;#endif - br_handle_frame_hook =NULL ;br_fdb_fini (); }
net/bridge/br_if.c +14 -0
@@ -119,6 +119,11 @@ static void destroy_nbp_rcu(struct rcu_head *head)destroy_nbp (p); } +static const struct netdev_rx_handler br_rx_handler = { + .order =NETDEV_RX_HANDLER_ORDER_BRIDGE , + .callback = br_handle_frame, +}; +/* Delete port(interface) from bridge is done in two steps. * via RCU. First step, marks device as down. That deletes * all the timers and stops new packets from flowing through.
@@ -147,6 +152,7 @@ static void del_nbp(struct net_bridge_port *p)list_del_rcu (&p->list); +netdev_rx_handler_unregister (dev, &br_rx_handler);rcu_assign_pointer (dev->br_port,NULL );br_multicast_del_port (p);
@@ -429,6 +435,11 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) goto err2;rcu_assign_pointer (dev->br_port, p); + + err =netdev_rx_handler_register (dev, &br_rx_handler); +if (err) + goto err3; +dev_disable_lro (dev);list_add_rcu (&p->list, &br->port_list);
@@ -451,6 +462,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)br_netpoll_enable (br, dev);return 0 ; +err3: +rcu_assign_pointer (dev->br_port,NULL ); +synchronize_rcu (); err2:br_fdb_delete_by_port (br, p,1 ); err1:
net/bridge/br_input.c +9 -3
@@ -131,15 +131,19 @@ static inline int is_link_local(const unsigned char *dest) }/* -* Called via br_handle_frame_hook. * Return NULL if skb is handled -* note: already called with rcu_read_lock (preempt_disabled) +* note: already called with rcu_read_lock (preempt_disabled) from +* netif_receive_skb */ -struct sk_buff *br_handle_frame (struct net_bridge_port *p,struct sk_buff *skb) +struct sk_buff *br_handle_frame (struct sk_buff *skb) { +struct net_bridge_port *p;const unsigned char *dest =eth_hdr (skb)->h_dest;int (*rhook)(struct sk_buff *skb); +if (skb->pkt_type ==PACKET_LOOPBACK ) +return skb; +if (!is_valid_ether_addr (eth_hdr (skb)->h_source)) goto drop;
@@ -147,6 +151,8 @@ struct sk_buff *br_handle_frame(struct net_bridge_port *p, struct sk_buff *skb)if (!skb)return NULL ; + p =rcu_dereference (skb->dev->br_port); +if (unlikely (is_link_local (dest))) {/* Pause frames shouldn't be passed up by driver anyway */ if (skb->protocol ==htons (ETH_P_PAUSE ))
net/bridge/br_private.h +1 -2
@@ -331,8 +331,7 @@ extern void br_features_recompute(struct net_bridge *br);/* br_input.c */ extern int br_handle_frame_finish (struct sk_buff *skb); -extern struct sk_buff *br_handle_frame (struct net_bridge_port *p, -struct sk_buff *skb); +extern struct sk_buff *br_handle_frame (struct sk_buff *skb);/* br_ioctl.c */ extern int br_dev_ioctl (struct net_device *dev,struct ifreq *rq,int cmd);
net/core/dev.c +96 -64
196 lines not on this page · raw message
Signature · 1 line
1.6.6.1
net: replace hooks in __netif_receive_skb (v4)
From: Jiri Pirko <[email protected]>
What this patch does is it removes two receive frame hooks (for bridge and for macvlan) from __netif_receive_skb. These are replaced them with a single hook for both. It only supports one hook per device because it makes no sense to do bridging and macvlan on the same device.
Then a network driver (of virtual netdev like macvlan or bridge) can register an rx_handler for needed net device.
---v3->v4
- only one hook per device.v2->v3
- used GPL-ed exportsv1->v2
- writers are locked by rtnl_lock (removed unnecessary spinlock)
- using call_rcu in unregister
- nicer macvlan_port_create cleanup
- struct rx_hanler is made const in funtion parameters
drivers/net/macvlan.c | 19 ++++--- include/linux/if_bridge.h | 2 include/linux/if_macvlan.h | 4 - include/linux/netdevice.h | 8 +++ net/bridge/br.c | 2 net/bridge/br_if.c | 8 +++ net/bridge/br_input.c | 12 +++- net/bridge/br_private.h | 3 - net/core/dev.c | 119 ++++++++++++++++++++------------------------- 9 files changed, 94 insertions(+), 83 deletions(-)
drivers/net/macvlan.c +13 -6
@@ -145,15 +145,16 @@ static void macvlan_broadcast(struct sk_ }/* called under rcu_read_lock() from netif_receive_skb */ -static struct sk_buff *macvlan_handle_frame (struct macvlan_port *port, -struct sk_buff *skb) +static struct sk_buff *macvlan_handle_frame (struct sk_buff *skb) { +struct macvlan_port *port;const struct ethhdr *eth =eth_hdr (skb);const struct macvlan_dev *vlan;const struct macvlan_dev *src;struct net_device *dev;unsigned int len; + port =rcu_dereference (skb->dev->macvlan_port);if (is_multicast_ether_addr (eth->h_dest)) { src =macvlan_hash_lookup (port, eth->h_source);if (!src)
@@ -515,6 +516,7 @@ static int macvlan_port_create(struct ne {struct macvlan_port *port;unsigned int i; +int err;if (dev->type !=ARPHRD_ETHER || dev->flags &IFF_LOOPBACK )return -EINVAL ;
@@ -528,13 +530,21 @@ static int macvlan_port_create(struct nefor (i =0 ; i <MACVLAN_HASH_SIZE ; i++)INIT_HLIST_HEAD (&port->vlan_hash[i]);rcu_assign_pointer (dev->macvlan_port, port); -return 0 ; + + err =netdev_rx_handler_register (dev, macvlan_handle_frame); +if (err) { + dev->macvlan_port =NULL ; +kfree (port); + } + +return err; }static void macvlan_port_destroy (struct net_device *dev) {struct macvlan_port *port = dev->macvlan_port; +netdev_rx_handler_unregister (dev);rcu_assign_pointer (dev->macvlan_port,NULL );synchronize_rcu ();kfree (port);
@@ -767,14 +777,12 @@ static int __init macvlan_init_module(voint err;register_netdevice_notifier (&macvlan_notifier_block); - macvlan_handle_frame_hook = macvlan_handle_frame; err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ; err1: - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block);return err; }
@@ -782,7 +790,6 @@ err1: static void __exit macvlan_cleanup_module(void) {rtnl_link_unregister (&macvlan_link_ops); - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block); }
include/linux/if_bridge.h +0 -2
@@ -102,8 +102,6 @@ struct __fdb_entry {#include <linux/netdevice.h> extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *)); -extern struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb);extern int (*br_should_route_hook)(struct sk_buff *skb);#endif
include/linux/if_macvlan.h +0 -4
@@ -84,8 +84,4 @@ extern int macvlan_link_register(structextern netdev_tx_t macvlan_start_xmit (struct sk_buff *skb,struct net_device *dev); - -extern struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *, -struct sk_buff *); -#endif /* _LINUX_IF_MACVLAN_H */
include/linux/netdevice.h +8 -0
@@ -254,6 +254,7 @@ struct netdev_hw_addr_list {#define netdev_for_each_mc_addr (ha, dev) \ netdev_hw_addr_list_for_each(ha, &(dev)->mc) +struct hh_cache {struct hh_cache *hh_next;/* Next entry */ atomic_t hh_refcnt;/* number of users */
@@ -381,6 +382,8 @@ enum gro_result { };typedef enum gro_result gro_result_t ; +typedef struct sk_buff *rx_callback_func_t (struct sk_buff *skb); +extern void __napi_schedule (struct napi_struct *n);static inline int napi_disable_pending (struct napi_struct *n)
@@ -957,6 +960,7 @@ struct net_device {#endif struct netdev_queue rx_queue; +rx_callback_func_t *rx_handler;struct netdev_queue *_tx____cacheline_aligned_in_smp ;
@@ -1693,6 +1697,10 @@ static inline void napi_free_frags(struc napi->skb =NULL ; } +extern int netdev_rx_handler_register (struct net_device *dev, +rx_callback_func_t *hook); +extern void netdev_rx_handler_unregister (struct net_device *dev); +extern void netif_nit_deliver (struct sk_buff *skb);extern int dev_valid_name (const char *name);extern int dev_ioctl (struct net *net,unsigned int cmd,void __user *);
net/bridge/br.c +0 -2
@@ -63,7 +63,6 @@ static int __init br_init(void) goto err_out4;brioctl_set (br_ioctl_deviceless_stub); - br_handle_frame_hook = br_handle_frame;#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) br_fdb_test_addr_hook = br_fdb_test_addr;
@@ -100,7 +99,6 @@ static void __exit br_deinit(void) br_fdb_test_addr_hook =NULL ;#endif - br_handle_frame_hook =NULL ;br_fdb_fini (); }
net/bridge/br_if.c +8 -0
@@ -147,6 +147,7 @@ static void del_nbp(struct net_bridge_polist_del_rcu (&p->list); +netdev_rx_handler_unregister (dev);rcu_assign_pointer (dev->br_port,NULL );br_multicast_del_port (p);
@@ -429,6 +430,11 @@ int br_add_if(struct net_bridge *br, str goto err2;rcu_assign_pointer (dev->br_port, p); + + err =netdev_rx_handler_register (dev, br_handle_frame); +if (err) + goto err3; +dev_disable_lro (dev);list_add_rcu (&p->list, &br->port_list);
@@ -451,6 +457,8 @@ int br_add_if(struct net_bridge *br, strbr_netpoll_enable (br, dev);return 0 ; +err3: + dev->br_port =NULL ; err2:br_fdb_delete_by_port (br, p,1 ); err1:
net/bridge/br_input.c +9 -3
@@ -131,15 +131,19 @@ static inline int is_link_local(const un }/* -* Called via br_handle_frame_hook. * Return NULL if skb is handled -* note: already called with rcu_read_lock (preempt_disabled) +* note: already called with rcu_read_lock (preempt_disabled) from +* netif_receive_skb */ -struct sk_buff *br_handle_frame (struct net_bridge_port *p,struct sk_buff *skb) +struct sk_buff *br_handle_frame (struct sk_buff *skb) { +struct net_bridge_port *p;const unsigned char *dest =eth_hdr (skb)->h_dest;int (*rhook)(struct sk_buff *skb); +if (skb->pkt_type ==PACKET_LOOPBACK ) +return skb; +if (!is_valid_ether_addr (eth_hdr (skb)->h_source)) goto drop;
@@ -147,6 +151,8 @@ struct sk_buff *br_handle_frame(struct nif (!skb)return NULL ; + p =rcu_dereference (skb->dev->br_port); +if (unlikely (is_link_local (dest))) {/* Pause frames shouldn't be passed up by driver anyway */ if (skb->protocol ==htons (ETH_P_PAUSE ))
net/bridge/br_private.h +1 -2
@@ -331,8 +331,7 @@ extern void br_features_recompute(struct/* br_input.c */ extern int br_handle_frame_finish (struct sk_buff *skb); -extern struct sk_buff *br_handle_frame (struct net_bridge_port *p, -struct sk_buff *skb); +extern struct sk_buff *br_handle_frame (struct sk_buff *skb);/* br_ioctl.c */ extern int br_dev_ioctl (struct net_device *dev,struct ifreq *rq,int cmd);
net/core/dev.c +55 -64
149 lines not on this page · raw message
Re: net: replace hooks in __netif_receive_skb (v4)
Actually, I'm not happy about this (reducing to only one hook) and for two reasons:
1) I think it's a good behaviour to "mask" one handler by another in case device is for example used for macvlan and then added to bridge. Because when it's again removed from the bridge, the original functionality is restored. And also, this would be consistent with the current behaviour.
2) I would imagine a situation, when multiple handers are needed in cascade. Actually I'm working on a virtual device draft which uses two handlers, although in corner situation.
Regards,
Jirka
Quoted 420 lines · Tue, Jun 01, 2010 at 05:28:05PM CEST, [email protected] wrote:
From: Jiri Pirko <[email protected]>
What this patch does is it removes two receive frame hooks (for bridge and for macvlan) from
__netif_receive_skb. These are replaced them with a single hook for both. It only supports one hook per device because it makes no sense to do bridging and macvlan on the same device.Then a network driver (of virtual netdev like macvlan or bridge) can register an
rx_handlerfor needed net device.Signed-off-by Jiri Pirko <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>---
v3->v4
- only one hook per device.v2->v3
- used GPL-ed exportsv1->v2
- writers are locked byrtnl_lock(removed unnecessary spinlock)
- usingcall_rcuin unregister
- nicermacvlan_port_createcleanup
- structrx_hanleris made const in funtion parametersdrivers/net/macvlan.c | 19 ++++--- include/linux/if_bridge.h | 2 include/linux/if_macvlan.h | 4 - include/linux/netdevice.h | 8 +++ net/bridge/br.c | 2 net/bridge/br_if.c | 8 +++ net/bridge/br_input.c | 12 +++- net/bridge/br_private.h | 3 - net/core/dev.c | 119 ++++++++++++++++++++------------------------- 9 files changed, 94 insertions(+), 83 deletions(-)drivers/net/macvlan.c +13
-4@@ -145,15 +145,16 @@ static void macvlan_broadcast(struct sk_ }/* called under rcu_read_lock() from netif_receive_skb */ -static struct sk_buff *macvlan_handle_frame (struct macvlan_port *port, -struct sk_buff *skb) +static struct sk_buff *macvlan_handle_frame (struct sk_buff *skb) { +struct macvlan_port *port;const struct ethhdr *eth =eth_hdr (skb);const struct macvlan_dev *vlan;const struct macvlan_dev *src;struct net_device *dev;unsigned int len; + port =rcu_dereference (skb->dev->macvlan_port);if (is_multicast_ether_addr (eth->h_dest)) { src =macvlan_hash_lookup (port, eth->h_source);if (!src)@@ -515,6 +516,7 @@ static int macvlan_port_create(struct ne {struct macvlan_port *port;unsigned int i; +int err;if (dev->type !=ARPHRD_ETHER || dev->flags &IFF_LOOPBACK )return -EINVAL ;@@ -528,13 +530,21 @@ static int macvlan_port_create(struct nefor (i =0 ; i <MACVLAN_HASH_SIZE ; i++)INIT_HLIST_HEAD (&port->vlan_hash[i]);rcu_assign_pointer (dev->macvlan_port, port); -return 0 ; + + err =netdev_rx_handler_register (dev, macvlan_handle_frame); +if (err) { + dev->macvlan_port =NULL ; +kfree (port); + } + +return err; }static void macvlan_port_destroy (struct net_device *dev) {struct macvlan_port *port = dev->macvlan_port; +netdev_rx_handler_unregister (dev);rcu_assign_pointer (dev->macvlan_port,NULL );synchronize_rcu ();kfree (port);@@ -767,14 +777,12 @@ static int __init macvlan_init_module(voint err;register_netdevice_notifier (&macvlan_notifier_block); - macvlan_handle_frame_hook = macvlan_handle_frame; err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ;err1:
drivers/net/macvlan.c +0
-1- macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier(&macvlan_notifier_block); return err; }drivers/net/macvlan.c +0
-1@@ -782,7 +790,6 @@ err1: static void __exit macvlan_cleanup_module(void) {rtnl_link_unregister (&macvlan_link_ops); - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block); }include/linux/if_bridge.h +0
-2@@ -102,8 +102,6 @@ struct __fdb_entry {#include <linux/netdevice.h> extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *)); -extern struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb);extern int (*br_should_route_hook)(struct sk_buff *skb);#endif include/linux/if_macvlan.h +0
-4@@ -84,8 +84,4 @@ extern int macvlan_link_register(structextern netdev_tx_t macvlan_start_xmit (struct sk_buff *skb,struct net_device *dev); - -extern struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *, -struct sk_buff *); -#endif /* _LINUX_IF_MACVLAN_H */ include/linux/netdevice.h +8
-0@@ -254,6 +254,7 @@ struct netdev_hw_addr_list {#define netdev_for_each_mc_addr (ha, dev) \ netdev_hw_addr_list_for_each(ha, &(dev)->mc) +struct hh_cache {struct hh_cache *hh_next;/* Next entry */ atomic_t hh_refcnt;/* number of users */ @@ -381,6 +382,8 @@ enum gro_result { };typedef enum gro_result gro_result_t ; +typedef struct sk_buff *rx_callback_func_t (struct sk_buff *skb); +extern void __napi_schedule (struct napi_struct *n);static inline int napi_disable_pending (struct napi_struct *n)@@ -957,6 +960,7 @@ struct net_device {#endif struct netdev_queue rx_queue; +rx_callback_func_t *rx_handler;struct netdev_queue *_tx____cacheline_aligned_in_smp ;@@ -1693,6 +1697,10 @@ static inline void napi_free_frags(struc napi->skb =NULL ; } +extern int netdev_rx_handler_register (struct net_device *dev, +rx_callback_func_t *hook); +extern void netdev_rx_handler_unregister (struct net_device *dev); +extern void netif_nit_deliver (struct sk_buff *skb);extern int dev_valid_name (const char *name);extern int dev_ioctl (struct net *net,unsigned int cmd,void __user *);net/bridge/br.c +0
-2@@ -63,7 +63,6 @@ static int __init br_init(void) goto err_out4;brioctl_set (br_ioctl_deviceless_stub); - br_handle_frame_hook = br_handle_frame;#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) br_fdb_test_addr_hook = br_fdb_test_addr;@@ -100,7 +99,6 @@ static void __exit br_deinit(void) br_fdb_test_addr_hook =NULL ;#endif - br_handle_frame_hook =NULL ;br_fdb_fini (); }net/bridge/br_if.c +8
-0@@ -147,6 +147,7 @@ static void del_nbp(struct net_bridge_polist_del_rcu (&p->list); +netdev_rx_handler_unregister (dev);rcu_assign_pointer (dev->br_port,NULL );br_multicast_del_port (p);@@ -429,6 +430,11 @@ int br_add_if(struct net_bridge *br, str goto err2;rcu_assign_pointer (dev->br_port, p); + + err =netdev_rx_handler_register (dev, br_handle_frame); +if (err) + goto err3; +dev_disable_lro (dev);list_add_rcu (&p->list, &br->port_list);@@ -451,6 +457,8 @@ int br_add_if(struct net_bridge *br, strbr_netpoll_enable (br, dev);return 0 ; +err3: + dev->br_port =NULL ;err2:
br_fdb_delete_by_port(br, p, 1);err1:
net/bridge/br_input.c +9
-3@@ -131,15 +131,19 @@ static inline int is_link_local(const un }/* -* Called via br_handle_frame_hook. * Return NULL if skb is handled -* note: already called with rcu_read_lock (preempt_disabled) +* note: already called with rcu_read_lock (preempt_disabled) from +* netif_receive_skb */ -struct sk_buff *br_handle_frame (struct net_bridge_port *p,struct sk_buff *skb) +struct sk_buff *br_handle_frame (struct sk_buff *skb) { +struct net_bridge_port *p;const unsigned char *dest =eth_hdr (skb)->h_dest;int (*rhook)(struct sk_buff *skb); +if (skb->pkt_type ==PACKET_LOOPBACK ) +return skb; +if (!is_valid_ether_addr (eth_hdr (skb)->h_source)) goto drop;@@ -147,6 +151,8 @@ struct sk_buff *br_handle_frame(struct nif (!skb)return NULL ; + p =rcu_dereference (skb->dev->br_port); +if (unlikely (is_link_local (dest))) {/* Pause frames shouldn't be passed up by driver anyway */ if (skb->protocol ==htons (ETH_P_PAUSE ))net/bridge/br_private.h +1
-2@@ -331,8 +331,7 @@ extern void br_features_recompute(struct/* br_input.c */ extern int br_handle_frame_finish (struct sk_buff *skb); -extern struct sk_buff *br_handle_frame (struct net_bridge_port *p, -struct sk_buff *skb); +extern struct sk_buff *br_handle_frame (struct sk_buff *skb);/* br_ioctl.c */ extern int br_dev_ioctl (struct net_device *dev,struct ifreq *rq,int cmd);net/core/dev.c +45
-58@@ -2581,70 +2581,14 @@ static inline int deliver_skb(struct sk_return pt_prev->func (skb, skb->dev, pt_prev, orig_dev); } -#if defined(CONFIG_BRIDGE ) || defined (CONFIG_BRIDGE_MODULE ) - -#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) +#if (defined(CONFIG_BRIDGE ) || defined(CONFIG_BRIDGE_MODULE )) && \ + (defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ))/* This hook is defined here for ATM LANE */ int (*br_fdb_test_addr_hook)(struct net_device *dev,unsigned char *addr)__read_mostly ;EXPORT_SYMBOL_GPL (br_fdb_test_addr_hook);#endif -/* -* If bridge module is loaded call bridging hook. -* returns NULL if packet was consumed. -*/ -struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb)__read_mostly ; -EXPORT_SYMBOL_GPL (br_handle_frame_hook); - -static inline struct sk_buff *handle_bridge (struct sk_buff *skb, -struct packet_type **pt_prev,int *ret, -struct net_device *orig_dev) -{ -struct net_bridge_port *port; - -if (skb->pkt_type ==PACKET_LOOPBACK || - (port =rcu_dereference (skb->dev->br_port)) ==NULL ) -return skb; - -if (*pt_prev) { - *ret =deliver_skb (skb, *pt_prev, orig_dev); - *pt_prev =NULL ; - } - -return br_handle_frame_hook (port, skb); -} -#else -#define handle_bridge (skb, pt_prev, ret, orig_dev) (skb) -#endif - -#if defined(CONFIG_MACVLAN ) || defined(CONFIG_MACVLAN_MODULE ) -struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *p, -struct sk_buff *skb)__read_mostly ; -EXPORT_SYMBOL_GPL (macvlan_handle_frame_hook); - -static inline struct sk_buff *handle_macvlan (struct sk_buff *skb, -struct packet_type **pt_prev, -int *ret, -struct net_device *orig_dev) -{ -struct macvlan_port *port; - - port =rcu_dereference (skb->dev->macvlan_port); -if (!port) -return skb; - -if (*pt_prev) { - *ret =deliver_skb (skb, *pt_prev, orig_dev); - *pt_prev =NULL ; - } -return macvlan_handle_frame_hook (port, skb); -} -#else -#define handle_macvlan (skb, pt_prev, ret, orig_dev) (skb) -#endif -#ifdef CONFIG_NET_CLS_ACT /* TODO: Maybe we should just force sch_ingress to be compiled in * when CONFIG_NET_CLS_ACT is? otherwise some useless instructions @@ -2740,6 +2684,48 @@ void netif_nit_deliver(struct sk_buff *srcu_read_unlock (); } +/** +* netdev_rx_handler_register - register receive handler +* @dev: device to register a handler for +* @rh: receive handler to register +* +* Register a receive hander for a device. This handler will then be +* called from __netif_receive_skb. A negative errno code is returned +* on a failure. +* +* The caller must hold the rtnl_mutex. +*/ +int netdev_rx_handler_register (struct net_device *dev, +rx_callback_func_t *hook) +{ +ASSERT_RTNL (); + +if (dev->rx_handler) +return -EBUSY ; + +rcu_assign_pointer (dev->rx_handler, hook); + +return 0 ; +} +EXPORT_SYMBOL_GPL (netdev_rx_handler_register); + +/** +* netdev_rx_handler_unregister - unregister receive handler +* @dev: device to unregister a handler from +* @rh: receive handler to unregister +* +* Unregister a receive hander from a device. +* +* The caller must hold the rtnl_mutex. +*/ +void netdev_rx_handler_unregister (struct net_device *dev) +{ + +ASSERT_RTNL (); + dev->rx_handler =NULL ; +} +EXPORT_SYMBOL_GPL (netdev_rx_handler_unregister); +static inline void skb_bond_set_mac_by_master (struct sk_buff *skb,struct net_device *master) {@@ -2792,6 +2778,7 @@ EXPORT_SYMBOL(__skb_bond_should_drop);static int __netif_receive_skb (struct sk_buff *skb) {struct packet_type *ptype, *pt_prev; +rx_callback_func_t *rh;struct net_device *orig_dev;struct net_device *master;struct net_device *null_or_orig;@@ -2855,12 +2842,16 @@ static int __netif_receive_skb(struct skncls:
#endifnet/core/dev.c +10
-6- skb =handle_bridge (skb, &pt_prev, &ret, orig_dev); -if (!skb) - goto out; - skb =handle_macvlan (skb, &pt_prev, &ret, orig_dev); -if (!skb) - goto out; +/* Handle special case of bridge or macvlan */ +if ((rh =rcu_dereference (skb->dev->rx_handler)) !=NULL ) { +if (pt_prev) { + ret =deliver_skb (skb, pt_prev, orig_dev); + pt_prev =NULL ; + } + skb =rh (skb); +if (!skb) + goto out; + }/* * Make sure frames received on VLAN interfaces stacked on
Re: net: replace hooks in __netif_receive_skb (v4)
Quoted 8 lines · On Tue, 1 Jun 2010 17:41:07 +0200 Jiri Pirko <[email protected]> wrote:
Actually, I'm not happy about this (reducing to only one hook) and for two reasons:
1) I think it's a good behaviour to "mask" one handler by another in case device is for example used for macvlan and then added to bridge. Because when it's again removed from the bridge, the original functionality is restored. And also, this would be consistent with the current behaviour.
Quoted 6 lines
2) I would imagine a situation, when multiple handers are needed in cascade. Actually I'm working on a virtual device draft which uses two handlers, although in corner situation.
Regards,
Jirka
I don't like it because:
1) Adding macvlan to bridge makes no sense. The bridge is already in promicious mode.
2) I don't like to see functionality added when it is not needed.
3) The extra overhead of traversing list causes more cache activity in extreme hot path.
Wait and add the multiple handlers when your code is included.
Signature · 1 line
http://www.extremeprogramming.org/rules/early.html
Re: net: replace hooks in __netif_receive_skb (v4)
Quoted 15 lines · Tue, Jun 01, 2010 at 06:10:04PM CEST, [email protected] wrote:
Quoted 12 lines · On Tue, 1 Jun 2010 17:41:07 +0200 Jiri Pirko <[email protected]> wrote:
Actually, I'm not happy about this (reducing to only one hook) and for two reasons:
1) I think it's a good behaviour to "mask" one handler by another in case device is for example used for macvlan and then added to bridge. Because when it's again removed from the bridge, the original functionality is restored. And also, this would be consistent with the current behaviour.
2) I would imagine a situation, when multiple handers are needed in cascade. Actually I'm working on a virtual device draft which uses two handlers, although in corner situation.
Quoted 6 lines
Quoted 2 lines
Regards,
JirkaI don't like it because:
1) Adding macvlan to bridge makes no sense. The bridge is already in promicious mode.
Right, I'm not saying it has sense to have it together in the same moment. But you can to this:
# ip link add link eth0 type macvlan
You can use eth0 + macvlan0. Now you do:
# brctl addif br0 eth0
Direct use of eth0 and macvlan0 is not not possible now.
# brctl delif br0 eth0
Now the original functionality ot eth0 and macvlan0 is restored.
Quoted 6 lines
2) I don't like to see functionality added when it is not needed.
3) The extra overhead of traversing list causes more cache activity in extreme hot path.
But ok, I hear your arguments, I thought about them myself before I did the patch and I thought that added overhead (which is not too big, I see one more dereference, rx_handler pointer) would be acceptible.
Maybe someone other then us has opinion too :)
Quoted 4 lines
Wait and add the multiple handlers when your code is included.
Signature · 1 line
http://www.extremeprogramming.org/rules/early.html
RE: replace hooks in __netif_receive_skb (v4)
In reply to Stephen HemmingerQuoted 12 lines
Subject: net: replace hooks in
__netif_receive_skb(v4)From: Jiri Pirko <[email protected]>
What this patch does is it removes two receive frame hooks (for bridge and for
macvlan) from__netif_receive_skb. These are replaced them with a single
hook for both. It only supports one hook per device because it makes no sense to do bridging and macvlan on the same device.
Quoted 3 lines
Then a network driver (of virtual netdev like macvlan or bridge) can register
anrx_handlerfor needed net device.
I think the idea of this is really good, and it has been long required to get rid of the bridging hook and the "hack" for macvlan to get into the network stack.
However, I wonder, if this is to be used as a generic interface, then why the restriction of only having a single hook per device? Yes, it makes sense to do this for the bridge/macvlan combination, but in general there could be other cases where you would want to allow multiple receivers per device.
Quoted 12 lines
net/core/dev.c +1
-0@@ -2792,6 +2778,7 @@ EXPORT_SYMBOL(__skb_bond_should_drop);static int __netif_receive_skb (struct sk_buff *skb) {struct packet_type *ptype, *pt_prev; +rx_callback_func_t *rh;struct net_device *orig_dev;struct net_device *master;struct net_device *null_or_orig;@@ -2855,12 +2842,16 @@ static int __netif_receive_skb(struct sk ncls:#endif
net/core/dev.c static int __netif_receive_skb(struct sk · line 2848
Quoted 10 lines
net/core/dev.c +4
-6- skb =handle_bridge (skb, &pt_prev, &ret, orig_dev); -if (!skb) - goto out; - skb =handle_macvlan (skb, &pt_prev, &ret, orig_dev); -if (!skb) - goto out; +/* Handle special case of bridge or macvlan */ +if ((rh =rcu_dereference (skb->dev->rx_handler)) !=NULL ) { +if (pt_prev) { + ret =deliver_skb (skb, pt_prev, orig_dev);
What happens with 'ret' here? It is completely ignored, isn't it? Because you assume that there is only a single receiver per device? I think it would be much better to have return codes that indicate whether a packet has been consumed by a receive handler, or whether it is supposed to be processed further. The same actually applies for the packet handlers that are processed a bit further down in netif_receive_skb() - the return code is ignored and so a handler has no control over further processing of the packet.
Re: replace hooks in __netif_receive_skb (v4)
Quoted 12 lines · Tue, Jun 01, 2010 at 06:13:51PM CEST, [email protected] wrote:
Quoted 12 lines
Subject: net: replace hooks in
__netif_receive_skb(v4)From: Jiri Pirko <[email protected]>
What this patch does is it removes two receive frame hooks (for bridge and for
macvlan) from__netif_receive_skb. These are replaced them with a single
hook for both. It only supports one hook per device because it makes no sense to do bridging and macvlan on the same device.
Quoted 8 lines
Quoted 3 lines
Then a network driver (of virtual netdev like macvlan or bridge) can register
anrx_handlerfor needed net device.I think the idea of this is really good, and it has been long required to get rid of the bridging hook and the "hack" for macvlan to get into the network stack.
However, I wonder, if this is to be used as a generic interface, then why the restriction of only having a single hook per device? Yes, it makes sense to do this for the bridge/macvlan combination, but in general there could be other cases where you would want to allow multiple receivers per device.
Right, I agree.
Quoted 14 lines
Quoted 14 lines
net/core/dev.c +1
-0@@ -2792,6 +2778,7 @@ EXPORT_SYMBOL(__skb_bond_should_drop);static int __netif_receive_skb (struct sk_buff *skb) {struct packet_type *ptype, *pt_prev; +rx_callback_func_t *rh;struct net_device *orig_dev;struct net_device *master;struct net_device *null_or_orig;@@ -2855,12 +2842,16 @@ static int __netif_receive_skb(struct sk ncls:#endif
Quoted 12 lines
net/core/dev.c static int __netif_receive_skb(struct sk · line 2848
Quoted 10 lines
net/core/dev.c +4
-6- skb =handle_bridge (skb, &pt_prev, &ret, orig_dev); -if (!skb) - goto out; - skb =handle_macvlan (skb, &pt_prev, &ret, orig_dev); -if (!skb) - goto out; +/* Handle special case of bridge or macvlan */ +if ((rh =rcu_dereference (skb->dev->rx_handler)) !=NULL ) { +if (pt_prev) { + ret =deliver_skb (skb, pt_prev, orig_dev);What happens with 'ret' here? It is completely ignored, isn't it? Because you assume that there is only a single receiver per device? I think it would be much better to have return codes that indicate whether a packet has been consumed by a receive handler, or whether it is supposed to be processed further. The same actually applies for the packet handlers that are processed a bit further down in
netif_receive_skb()- the return code is ignored and so a handler has no control over further processing of the packet.
It's not ignored. it's returned at the end of the function __netif_receive_skb.
Re: net: replace hooks in __netif_receive_skb (v4)
In reply to Stephen HemmingerFew nitpicks, I'm going to send patch reflecting this later.
Quoted 65 lines · Tue, Jun 01, 2010 at 05:28:05PM CEST, [email protected] wrote:
From: Jiri Pirko <[email protected]>
What this patch does is it removes two receive frame hooks (for bridge and for macvlan) from
__netif_receive_skb. These are replaced them with a single hook for both. It only supports one hook per device because it makes no sense to do bridging and macvlan on the same device.Then a network driver (of virtual netdev like macvlan or bridge) can register an
rx_handlerfor needed net device.Signed-off-by Jiri Pirko <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>---
v3->v4
- only one hook per device.v2->v3
- used GPL-ed exportsv1->v2
- writers are locked byrtnl_lock(removed unnecessary spinlock)
- usingcall_rcuin unregister
- nicermacvlan_port_createcleanup
- structrx_hanleris made const in funtion parametersdrivers/net/macvlan.c | 19 ++++--- include/linux/if_bridge.h | 2 include/linux/if_macvlan.h | 4 - include/linux/netdevice.h | 8 +++ net/bridge/br.c | 2 net/bridge/br_if.c | 8 +++ net/bridge/br_input.c | 12 +++- net/bridge/br_private.h | 3 - net/core/dev.c | 119 ++++++++++++++++++++------------------------- 9 files changed, 94 insertions(+), 83 deletions(-)drivers/net/macvlan.c +4
-2@@ -145,15 +145,16 @@ static void macvlan_broadcast(struct sk_ }/* called under rcu_read_lock() from netif_receive_skb */ -static struct sk_buff *macvlan_handle_frame (struct macvlan_port *port, -struct sk_buff *skb) +static struct sk_buff *macvlan_handle_frame (struct sk_buff *skb) { +struct macvlan_port *port;const struct ethhdr *eth =eth_hdr (skb);const struct macvlan_dev *vlan;const struct macvlan_dev *src;struct net_device *dev;unsigned int len; + port =rcu_dereference (skb->dev->macvlan_port);if (is_multicast_ether_addr (eth->h_dest)) { src =macvlan_hash_lookup (port, eth->h_source);if (!src)@@ -515,6 +516,7 @@ static int macvlan_port_create(struct ne {struct macvlan_port *port;unsigned int i; +int err;if (dev->type !=ARPHRD_ETHER || dev->flags &IFF_LOOPBACK )return -EINVAL ;
drivers/net/macvlan.c static int macvlan_port_create(struct ne · line 536
Quoted 8 lines
drivers/net/macvlan.c +4
-1@@ -528,13 +530,21 @@ static int macvlan_port_create(struct nefor (i =0 ; i <MACVLAN_HASH_SIZE ; i++)INIT_HLIST_HEAD (&port->vlan_hash[i]);rcu_assign_pointer (dev->macvlan_port, port); -return 0 ; + + err =netdev_rx_handler_register (dev, macvlan_handle_frame); +if (err) { + dev->macvlan_port =NULL ;
Why not to use rcu_assign_pointer here? since this is not hot path, that would be nicer. It should be done for rcu-protected pointers (see rcu_assign_poiter comment).
Quoted 62 lines
drivers/net/macvlan.c +5
-1+kfree (port); + } + +return err; }static void macvlan_port_destroy (struct net_device *dev) {struct macvlan_port *port = dev->macvlan_port; +netdev_rx_handler_unregister (dev);rcu_assign_pointer (dev->macvlan_port,NULL );synchronize_rcu ();kfree (port);@@ -767,14 +777,12 @@ static int __init macvlan_init_module(voint err;register_netdevice_notifier (&macvlan_notifier_block); - macvlan_handle_frame_hook = macvlan_handle_frame; err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ;err1:
drivers/net/macvlan.c +0
-1- macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier(&macvlan_notifier_block); return err; }drivers/net/macvlan.c +0
-1@@ -782,7 +790,6 @@ err1: static void __exit macvlan_cleanup_module(void) {rtnl_link_unregister (&macvlan_link_ops); - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block); }include/linux/if_bridge.h +0
-2@@ -102,8 +102,6 @@ struct __fdb_entry {#include <linux/netdevice.h> extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *)); -extern struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb);extern int (*br_should_route_hook)(struct sk_buff *skb);#endif include/linux/if_macvlan.h +0
-4@@ -84,8 +84,4 @@ extern int macvlan_link_register(structextern netdev_tx_t macvlan_start_xmit (struct sk_buff *skb,struct net_device *dev); - -extern struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *, -struct sk_buff *); -#endif /* _LINUX_IF_MACVLAN_H */
include/linux/netdevice.h struct netdev_hw_addr_list { · line 257
Quoted 4 lines
include/linux/netdevice.h +1
-0@@ -254,6 +254,7 @@ struct netdev_hw_addr_list {#define netdev_for_each_mc_addr (ha, dev) \ netdev_hw_addr_list_for_each(ha, &(dev)->mc) +
Forgotten free line.
Quoted 73 lines
include/linux/netdevice.h +7
-0struct hh_cache {struct hh_cache *hh_next;/* Next entry */ atomic_t hh_refcnt;/* number of users */ @@ -381,6 +382,8 @@ enum gro_result { };typedef enum gro_result gro_result_t ; +typedef struct sk_buff *rx_callback_func_t (struct sk_buff *skb); +extern void __napi_schedule (struct napi_struct *n);static inline int napi_disable_pending (struct napi_struct *n)@@ -957,6 +960,7 @@ struct net_device {#endif struct netdev_queue rx_queue; +rx_callback_func_t *rx_handler;struct netdev_queue *_tx____cacheline_aligned_in_smp ;@@ -1693,6 +1697,10 @@ static inline void napi_free_frags(struc napi->skb =NULL ; } +extern int netdev_rx_handler_register (struct net_device *dev, +rx_callback_func_t *hook); +extern void netdev_rx_handler_unregister (struct net_device *dev); +extern void netif_nit_deliver (struct sk_buff *skb);extern int dev_valid_name (const char *name);extern int dev_ioctl (struct net *net,unsigned int cmd,void __user *);net/bridge/br.c +0
-2@@ -63,7 +63,6 @@ static int __init br_init(void) goto err_out4;brioctl_set (br_ioctl_deviceless_stub); - br_handle_frame_hook = br_handle_frame;#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) br_fdb_test_addr_hook = br_fdb_test_addr;@@ -100,7 +99,6 @@ static void __exit br_deinit(void) br_fdb_test_addr_hook =NULL ;#endif - br_handle_frame_hook =NULL ;br_fdb_fini (); }net/bridge/br_if.c +1
-0@@ -147,6 +147,7 @@ static void del_nbp(struct net_bridge_polist_del_rcu (&p->list); +netdev_rx_handler_unregister (dev);rcu_assign_pointer (dev->br_port,NULL );br_multicast_del_port(p);net/bridge/br_if.c +5
-0@@ -429,6 +430,11 @@ int br_add_if(struct net_bridge *br, str goto err2;rcu_assign_pointer (dev->br_port, p); + + err =netdev_rx_handler_register (dev, br_handle_frame); +if (err) + goto err3; +dev_disable_lro (dev);list_add_rcu (&p->list, &br->port_list);
net/bridge/br_if.c int br_add_if(struct net_bridge *br, str · line 461
Quoted 5 lines
net/bridge/br_if.c +2
-0@@ -451,6 +457,8 @@ int br_add_if(struct net_bridge *br, strbr_netpoll_enable (br, dev);return 0 ; +err3: + dev->br_port =NULL ;
I would like to see rcu_assing_pointer here too
Quoted 125 lines
err2:
br_fdb_delete_by_port(br, p, 1);err1:
net/bridge/br_input.c +9
-3@@ -131,15 +131,19 @@ static inline int is_link_local(const un }/* -* Called via br_handle_frame_hook. * Return NULL if skb is handled -* note: already called with rcu_read_lock (preempt_disabled) +* note: already called with rcu_read_lock (preempt_disabled) from +* netif_receive_skb */ -struct sk_buff *br_handle_frame (struct net_bridge_port *p,struct sk_buff *skb) +struct sk_buff *br_handle_frame (struct sk_buff *skb) { +struct net_bridge_port *p;const unsigned char *dest =eth_hdr (skb)->h_dest;int (*rhook)(struct sk_buff *skb); +if (skb->pkt_type ==PACKET_LOOPBACK ) +return skb; +if (!is_valid_ether_addr (eth_hdr (skb)->h_source)) goto drop;@@ -147,6 +151,8 @@ struct sk_buff *br_handle_frame(struct nif (!skb)return NULL ; + p =rcu_dereference (skb->dev->br_port); +if (unlikely (is_link_local (dest))) {/* Pause frames shouldn't be passed up by driver anyway */ if (skb->protocol ==htons (ETH_P_PAUSE ))net/bridge/br_private.h +1
-2@@ -331,8 +331,7 @@ extern void br_features_recompute(struct/* br_input.c */ extern int br_handle_frame_finish (struct sk_buff *skb); -extern struct sk_buff *br_handle_frame (struct net_bridge_port *p, -struct sk_buff *skb); +extern struct sk_buff *br_handle_frame (struct sk_buff *skb);/* br_ioctl.c */ extern int br_dev_ioctl (struct net_device *dev,struct ifreq *rq,int cmd);net/core/dev.c +2
-58@@ -2581,70 +2581,14 @@ static inline int deliver_skb(struct sk_return pt_prev->func (skb, skb->dev, pt_prev, orig_dev); } -#if defined(CONFIG_BRIDGE ) || defined (CONFIG_BRIDGE_MODULE ) - -#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) +#if (defined(CONFIG_BRIDGE ) || defined(CONFIG_BRIDGE_MODULE )) && \ + (defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ))/* This hook is defined here for ATM LANE */ int (*br_fdb_test_addr_hook)(struct net_device *dev,unsigned char *addr)__read_mostly ;EXPORT_SYMBOL_GPL (br_fdb_test_addr_hook);#endif -/* -* If bridge module is loaded call bridging hook. -* returns NULL if packet was consumed. -*/ -struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb)__read_mostly ; -EXPORT_SYMBOL_GPL (br_handle_frame_hook); - -static inline struct sk_buff *handle_bridge (struct sk_buff *skb, -struct packet_type **pt_prev,int *ret, -struct net_device *orig_dev) -{ -struct net_bridge_port *port; - -if (skb->pkt_type ==PACKET_LOOPBACK || - (port =rcu_dereference (skb->dev->br_port)) ==NULL ) -return skb; - -if (*pt_prev) { - *ret =deliver_skb (skb, *pt_prev, orig_dev); - *pt_prev =NULL ; - } - -return br_handle_frame_hook (port, skb); -} -#else -#define handle_bridge (skb, pt_prev, ret, orig_dev) (skb) -#endif - -#if defined(CONFIG_MACVLAN ) || defined(CONFIG_MACVLAN_MODULE ) -struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *p, -struct sk_buff *skb)__read_mostly ; -EXPORT_SYMBOL_GPL (macvlan_handle_frame_hook); - -static inline struct sk_buff *handle_macvlan (struct sk_buff *skb, -struct packet_type **pt_prev, -int *ret, -struct net_device *orig_dev) -{ -struct macvlan_port *port; - - port =rcu_dereference (skb->dev->macvlan_port); -if (!port) -return skb; - -if (*pt_prev) { - *ret =deliver_skb (skb, *pt_prev, orig_dev); - *pt_prev =NULL ; - } -return macvlan_handle_frame_hook (port, skb); -} -#else -#define handle_macvlan (skb, pt_prev, ret, orig_dev) (skb) -#endif -#ifdef CONFIG_NET_CLS_ACT /* TODO: Maybe we should just force sch_ingress to be compiled in * when CONFIG_NET_CLS_ACT is? otherwise some useless instructions
net/core/dev.c void netif_nit_deliver(struct sk_buff *s · line 2690
Quoted 7 lines
net/core/dev.c +4
-0@@ -2740,6 +2684,48 @@ void netif_nit_deliver(struct sk_buff *srcu_read_unlock (); } +/** +* netdev_rx_handler_register - register receive handler +* @dev: device to register a handler for +* @rh: receive handler to register
Old comment
Quoted 15 lines
net/core/dev.c +15
-0+* +* Register a receive hander for a device. This handler will then be +* called from __netif_receive_skb. A negative errno code is returned +* on a failure. +* +* The caller must hold the rtnl_mutex. +*/ +int netdev_rx_handler_register (struct net_device *dev, +rx_callback_func_t *hook) +{ +ASSERT_RTNL (); + +if (dev->rx_handler) +return -EBUSY ; +
net/core/dev.c void netif_nit_deliver(struct sk_buff *s · line 2715
Quoted 10 lines
net/core/dev.c +10
-0+rcu_assign_pointer (dev->rx_handler, hook); + +return 0 ; +} +EXPORT_SYMBOL_GPL (netdev_rx_handler_register); + +/** +* netdev_rx_handler_unregister - unregister receive handler +* @dev: device to unregister a handler from +* @rh: receive handler to unregister
old comment
net/core/dev.c void netif_nit_deliver(struct sk_buff *s · line 2725
Quoted 10 lines
net/core/dev.c +10
-0+* +* Unregister a receive hander from a device. +* +* The caller must hold the rtnl_mutex. +*/ +void netdev_rx_handler_unregister (struct net_device *dev) +{ + +ASSERT_RTNL (); + dev->rx_handler =NULL ;
Also rcu_assign pointer would be nicer here.
Quoted 37 lines
net/core/dev.c +4
-0+} +EXPORT_SYMBOL_GPL (netdev_rx_handler_unregister); +static inline void skb_bond_set_mac_by_master (struct sk_buff *skb,struct net_device *master) {@@ -2792,6 +2778,7 @@ EXPORT_SYMBOL(__skb_bond_should_drop);static int __netif_receive_skb (struct sk_buff *skb) {struct packet_type *ptype, *pt_prev; +rx_callback_func_t *rh;struct net_device *orig_dev;struct net_device *master;struct net_device *null_or_orig;@@ -2855,12 +2842,16 @@ static int __netif_receive_skb(struct skncls:
#endifnet/core/dev.c +10
-6- skb =handle_bridge (skb, &pt_prev, &ret, orig_dev); -if (!skb) - goto out; - skb =handle_macvlan (skb, &pt_prev, &ret, orig_dev); -if (!skb) - goto out; +/* Handle special case of bridge or macvlan */ +if ((rh =rcu_dereference (skb->dev->rx_handler)) !=NULL ) { +if (pt_prev) { + ret =deliver_skb (skb, pt_prev, orig_dev); + pt_prev =NULL ; + } + skb =rh (skb); +if (!skb) + goto out; + }/* * Make sure frames received on VLAN interfaces stacked on
PATCH 1/1 net: replace hooks in __netif_receive_skb V5
In reply to Stephen HemmingerWhat this patch does is it removes two receive frame hooks (for bridge and for macvlan) from __netif_receive_skb. These are replaced them with a single hook for both. It only supports one hook per device because it makes no sense to do bridging and macvlan on the same device.
Then a network driver (of virtual netdev like macvlan or bridge) can register an rx_handler for needed net device.
---v4->v5
- do rcu_assign_pointer even for assigning null for rcu-protected pointers
- cosmetics (renames, comments and __netif_receive_skb if part)
drivers/net/macvlan.c | 19 +++++-- include/linux/if_bridge.h | 2 - include/linux/if_macvlan.h | 4 -- include/linux/netdevice.h | 7 +++ net/bridge/br.c | 2 - net/bridge/br_if.c | 8 +++ net/bridge/br_input.c | 12 +++- net/bridge/br_private.h | 3 +- net/core/dev.c | 119 ++++++++++++++++++++----------------------- 9 files changed, 93 insertions(+), 83 deletions(-)
drivers/net/macvlan.c +13 -6
@@ -145,15 +145,16 @@ static void macvlan_broadcast(struct sk_buff *skb, }/* called under rcu_read_lock() from netif_receive_skb */ -static struct sk_buff *macvlan_handle_frame (struct macvlan_port *port, -struct sk_buff *skb) +static struct sk_buff *macvlan_handle_frame (struct sk_buff *skb) { +struct macvlan_port *port;const struct ethhdr *eth =eth_hdr (skb);const struct macvlan_dev *vlan;const struct macvlan_dev *src;struct net_device *dev;unsigned int len; + port =rcu_dereference (skb->dev->macvlan_port);if (is_multicast_ether_addr (eth->h_dest)) { src =macvlan_hash_lookup (port, eth->h_source);if (!src)
@@ -515,6 +516,7 @@ static int macvlan_port_create(struct net_device *dev) {struct macvlan_port *port;unsigned int i; +int err;if (dev->type !=ARPHRD_ETHER || dev->flags &IFF_LOOPBACK )return -EINVAL ;
@@ -528,13 +530,21 @@ static int macvlan_port_create(struct net_device *dev)for (i =0 ; i <MACVLAN_HASH_SIZE ; i++)INIT_HLIST_HEAD (&port->vlan_hash[i]);rcu_assign_pointer (dev->macvlan_port, port); -return 0 ; + + err =netdev_rx_handler_register (dev, macvlan_handle_frame); +if (err) { +rcu_assign_pointer (dev->macvlan_port,NULL ); +kfree (port); + } + +return err; }static void macvlan_port_destroy (struct net_device *dev) {struct macvlan_port *port = dev->macvlan_port; +netdev_rx_handler_unregister (dev);rcu_assign_pointer (dev->macvlan_port,NULL );synchronize_rcu ();kfree (port);
@@ -767,14 +777,12 @@ static int __init macvlan_init_module(void)int err;register_netdevice_notifier (&macvlan_notifier_block); - macvlan_handle_frame_hook = macvlan_handle_frame; err =macvlan_link_register (&macvlan_link_ops);if (err <0 ) goto err1;return 0 ; err1: - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block);return err; }
@@ -782,7 +790,6 @@ err1: static void __exit macvlan_cleanup_module(void) {rtnl_link_unregister (&macvlan_link_ops); - macvlan_handle_frame_hook =NULL ;unregister_netdevice_notifier (&macvlan_notifier_block); }
include/linux/if_bridge.h +0 -2
@@ -102,8 +102,6 @@ struct __fdb_entry {#include <linux/netdevice.h> extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *)); -extern struct sk_buff *(*br_handle_frame_hook)(struct net_bridge_port *p, -struct sk_buff *skb);extern int (*br_should_route_hook)(struct sk_buff *skb);#endif
include/linux/if_macvlan.h +0 -4
@@ -84,8 +84,4 @@ extern int macvlan_link_register(struct rtnl_link_ops *ops);extern netdev_tx_t macvlan_start_xmit (struct sk_buff *skb,struct net_device *dev); - -extern struct sk_buff *(*macvlan_handle_frame_hook)(struct macvlan_port *, -struct sk_buff *); -#endif /* _LINUX_IF_MACVLAN_H */
include/linux/netdevice.h +7 -0
@@ -381,6 +381,8 @@ enum gro_result { };typedef enum gro_result gro_result_t ; +typedef struct sk_buff *rx_handler_func_t (struct sk_buff *skb); +extern void __napi_schedule (struct napi_struct *n);static inline int napi_disable_pending (struct napi_struct *n)
@@ -957,6 +959,7 @@ struct net_device {#endif struct netdev_queue rx_queue; +rx_handler_func_t *rx_handler;struct netdev_queue *_tx____cacheline_aligned_in_smp ;
@@ -1693,6 +1696,10 @@ static inline void napi_free_frags(struct napi_struct *napi) napi->skb =NULL ; } +extern int netdev_rx_handler_register (struct net_device *dev, +rx_handler_func_t *rx_handler); +extern void netdev_rx_handler_unregister (struct net_device *dev); +extern void netif_nit_deliver (struct sk_buff *skb);extern int dev_valid_name (const char *name);extern int dev_ioctl (struct net *net,unsigned int cmd,void __user *);
net/bridge/br.c +0 -2
@@ -63,7 +63,6 @@ static int __init br_init(void) goto err_out4;brioctl_set (br_ioctl_deviceless_stub); - br_handle_frame_hook = br_handle_frame;#if defined(CONFIG_ATM_LANE ) || defined(CONFIG_ATM_LANE_MODULE ) br_fdb_test_addr_hook = br_fdb_test_addr;
@@ -100,7 +99,6 @@ static void __exit br_deinit(void) br_fdb_test_addr_hook =NULL ;#endif - br_handle_frame_hook =NULL ;br_fdb_fini (); }
net/bridge/br_if.c +8 -0
@@ -147,6 +147,7 @@ static void del_nbp(struct net_bridge_port *p)list_del_rcu (&p->list); +netdev_rx_handler_unregister (dev);rcu_assign_pointer (dev->br_port,NULL );br_multicast_del_port (p);
@@ -429,6 +430,11 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) goto err2;rcu_assign_pointer (dev->br_port, p); + + err =netdev_rx_handler_register (dev, br_handle_frame); +if (err) + goto err3; +dev_disable_lro (dev);list_add_rcu (&p->list, &br->port_list);
@@ -451,6 +457,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)br_netpoll_enable (br, dev);return 0 ; +err3: +rcu_assign_pointer (dev->br_port,NULL ); err2:br_fdb_delete_by_port (br, p,1 ); err1:
net/bridge/br_input.c +9 -3
@@ -131,15 +131,19 @@ static inline int is_link_local(const unsigned char *dest) }/* -* Called via br_handle_frame_hook. * Return NULL if skb is handled -* note: already called with rcu_read_lock (preempt_disabled) +* note: already called with rcu_read_lock (preempt_disabled) from +* netif_receive_skb */ -struct sk_buff *br_handle_frame (struct net_bridge_port *p,struct sk_buff *skb) +struct sk_buff *br_handle_frame (struct sk_buff *skb) { +struct net_bridge_port *p;const unsigned char *dest =eth_hdr (skb)->h_dest;int (*rhook)(struct sk_buff *skb); +if (skb->pkt_type ==PACKET_LOOPBACK ) +return skb; +if (!is_valid_ether_addr (eth_hdr (skb)->h_source)) goto drop;
@@ -147,6 +151,8 @@ struct sk_buff *br_handle_frame(struct net_bridge_port *p, struct sk_buff *skb)if (!skb)return NULL ; + p =rcu_dereference (skb->dev->br_port); +if (unlikely (is_link_local (dest))) {/* Pause frames shouldn't be passed up by driver anyway */ if (skb->protocol ==htons (ETH_P_PAUSE ))
net/bridge/br_private.h +1 -2
@@ -331,8 +331,7 @@ extern void br_features_recompute(struct net_bridge *br);/* br_input.c */ extern int br_handle_frame_finish (struct sk_buff *skb); -extern struct sk_buff *br_handle_frame (struct net_bridge_port *p, -struct sk_buff *skb); +extern struct sk_buff *br_handle_frame (struct sk_buff *skb);/* br_ioctl.c */ extern int br_dev_ioctl (struct net_device *dev,struct ifreq *rq,int cmd);
net/core/dev.c +55 -64
149 lines not on this page · raw message
Signature · 1 line
1.7.0.1
David Miller
maintainerRe: [PATCH net-next-2.6] net: replace hooks in __netif_receive_skb V5
From: Jiri Pirko <[email protected]>
Date: Wed, 2 Jun 2010 09:52:08 +0200
Quoted 10 lines
What this patch does is it removes two receive frame hooks (for bridge and for macvlan) from
__netif_receive_skb. These are replaced them with a single hook for both. It only supports one hook per device because it makes no sense to do bridging and macvlan on the same device.Then a network driver (of virtual netdev like macvlan or bridge) can register an
rx_handlerfor needed net device.Signed-off-by Jiri Pirko <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>
Ok, this looks good to me. Applied, thanks everyone.
Re: [PATCH net-next-2.6] net: replace hooks in __netif_receive_skb V5
In reply to Jiri Pirkodrivers/net/macvlan.c static int macvlan_port_create(struct net_device *dev) · line 541
Quoted 9 lines · On Wed, 2 Jun 2010 09:52:08 +0200 Jiri Pirko <[email protected]> wrote:
drivers/net/macvlan.c +8
-0+ + err =netdev_rx_handler_register (dev, macvlan_handle_frame); +if (err) { +rcu_assign_pointer (dev->macvlan_port,NULL ); +kfree (port); + } + +return err; }
Rcu assign is not necessary here for because the hook didn't get registered so there is no way for other CPU to see it.
Re: [PATCH net-next-2.6] net: replace hooks in __netif_receive_skb V5
Quoted 16 lines · Stephen Hemminger
drivers/net/macvlan.c static int macvlan_port_create(struct net_device *dev) · line 541
Quoted 9 lines · On Wed, 2 Jun 2010 09:52:08 +0200 Jiri Pirko <[email protected]> wrote:
drivers/net/macvlan.c +8
-0+ + err =netdev_rx_handler_register (dev, macvlan_handle_frame); +if (err) { +rcu_assign_pointer (dev->macvlan_port,NULL ); +kfree (port); + } + +return err; }Rcu assign is not necessary here for because the hook didn't get registered so there is no way for other CPU to see it.
Thats a valid point, but we should use it, and not care of this litle detail. Compiler generates same code anyway, since NULL value is tested by rcu_assign_pointer().
If we dont use rcu_assign_pointer() ourself, Paul or Arnd will put it one day or another :)
Re: [PATCH net-next-2.6] net: replace hooks in __netif_receive_skb V5
Quoted 18 lines · Eric Dumazet
Quoted 16 lines · Stephen Hemminger
drivers/net/macvlan.c static int macvlan_port_create(struct net_device *dev) · line 541
Quoted 9 lines · On Wed, 2 Jun 2010 09:52:08 +0200 Jiri Pirko <[email protected]> wrote:
drivers/net/macvlan.c +8
-0+ + err =netdev_rx_handler_register (dev, macvlan_handle_frame); +if (err) { +rcu_assign_pointer (dev->macvlan_port,NULL ); +kfree (port); + } + +return err; }Rcu assign is not necessary here for because the hook didn't get registered so there is no way for other CPU to see it.
Quoted 8 lines
Thats a valid point, but we should use it, and not care of this litle detail. Compiler generates same code anyway, since NULL value is tested by
rcu_assign_pointer().If we dont use
rcu_assign_pointer()ourself, Paul or Arnd will put it one day or another :)
Some year soon, I hope. ;-)
Thanx, Paul
RE: [PATCH net-next-2.6] net: replace hooks in __netif_receive_skb V5
In reply to Jiri PirkoQuoted 9 lines
Subject: [PATCH net-next-2.6] net: replace hooks in
__netif_receive_skbV5What this patch does is it removes two receive frame hooks (for bridge and for
macvlan) from__netif_receive_skb. These are replaced them with a single hook for both. It only supports one hook per device because it makes no sense to do bridging and macvlan on the same device.
Quoted 6 lines
Then a network driver (of virtual netdev like macvlan or bridge) can register
anrx_handlerfor needed net device.Signed-off-by Jiri Pirko <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>
I would like to see this being accepted. As mentioned before I would love to be able to support multiple receive frame hooks per device, but I think this is a good start.
I think especially with virtualization coming into Linux and new network virtualization approaches being developed it would be nice to have a more generic function for packet receive handlers to hook into the network stack more cleanly. Current approaches rely on 'misusing' the bridging hook into the kernel, because the packet 'sniffing' hooks (via dev_pack_add()) do not give enough control over packet processing.