| Message ID | 1440096383-8915-2-git-send-email-anjali.singhai@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Jeff Kirsher |
| Headers | show |
On Thu, 2015-08-20 at 11:46 -0700, Anjali Singhai Jain wrote: > Add ndo_ops to add/del UDP ports to a device that supports geneve > offload. > > v2:Added comments for new ndo_ops and minor format fix. > > Signed-off-by: Kiran Patil <kiran.patil@intel.com> > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com> > --- > include/linux/netdevice.h | 20 +++++++++++++++++++- > net/ipv4/geneve_core.c | 22 ++++++++++++++++++++++ > 2 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index f3bb290..d6f00c7 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1016,6 +1016,19 @@ typedef u16 (*select_queue_fallback_t)(struct > net_device *dev, > * address family that vxlan is not listening to anymore. The > operation > * is protected by the vxlan_net->sock_lock. > * > + * void (*ndo_add_geneve_port)(struct net_device *dev, > + * sa_family_t sa_family, __be16 port); > + * Called by geneve to notiy a driver about the UDP port and > socket > + * address family that geneve is listnening to. It is called > only when > + * a new port starts listening. The operation is protected by > the > + * geneve_net->sock_lock. > + * > + * void (*ndo_del_geneve_port)(struct net_device *dev, > + * sa_family_t sa_family, __be16 port); > + * Called by geneve to notify the driver about a UDP port and > socket > + * address family that geneve is not listening to anymore. > The operation > + * is protected by the geneve_net->sock_lock. > + * Would it make more sense to generalize the ndo op for future protocol extension instead of continuing to add separate tunnel offload functions for each one? ie: generalize ndo_add_vxlan_port into "ndo_add_tunnel_port"? Maybe it's not worth it though..? Regards, Jake
On Thu, 2015-08-20 at 11:46 -0700, Anjali Singhai Jain wrote: > Add ndo_ops to add/del UDP ports to a device that supports geneve > offload. > > v2:Added comments for new ndo_ops and minor format fix. > > Signed-off-by: Kiran Patil <kiran.patil@intel.com> > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com> > --- Am I missing something about how this would enable Tx offloads? Or is that enabled already? Regards, Jake
This is in preparation for making the HW aware of which UDP ports are being used for Geneve packets so that it can do the Geneve offloads. The TX/RX offload path is common between VXLAN and Geneve where in the skb is marked for encapsulation and the outer transport header determines if it’s a UDP tunnel. Anjali > -----Original Message----- > From: Keller, Jacob E > Sent: Thursday, August 20, 2015 1:37 PM > To: intel-wired-lan@lists.osuosl.org; Singhai, Anjali > Subject: Re: [Intel-wired-lan] [PATCH 2/4] geneve: Add geneve udp port > offload for ethernet devices > > On Thu, 2015-08-20 at 11:46 -0700, Anjali Singhai Jain wrote: > > Add ndo_ops to add/del UDP ports to a device that supports geneve > > offload. > > > > v2:Added comments for new ndo_ops and minor format fix. > > > > Signed-off-by: Kiran Patil <kiran.patil@intel.com> > > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com> > > --- > > Am I missing something about how this would enable Tx offloads? Or is that > enabled already? > > Regards, > Jake
> -----Original Message----- > From: Keller, Jacob E > Sent: Thursday, August 20, 2015 1:32 PM > To: intel-wired-lan@lists.osuosl.org; Singhai, Anjali > Subject: Re: [Intel-wired-lan] [PATCH 2/4] geneve: Add geneve udp port > offload for ethernet devices > > On Thu, 2015-08-20 at 11:46 -0700, Anjali Singhai Jain wrote: > > Add ndo_ops to add/del UDP ports to a device that supports geneve > > offload. > > > > v2:Added comments for new ndo_ops and minor format fix. > > > > Signed-off-by: Kiran Patil <kiran.patil@intel.com> > > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com> > > --- > > include/linux/netdevice.h | 20 +++++++++++++++++++- > > net/ipv4/geneve_core.c | 22 ++++++++++++++++++++++ > > 2 files changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > index f3bb290..d6f00c7 100644 > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -1016,6 +1016,19 @@ typedef u16 (*select_queue_fallback_t)(struct > > net_device *dev, > > * address family that vxlan is not listening to anymore. The > > operation > > * is protected by the vxlan_net->sock_lock. > > * > > + * void (*ndo_add_geneve_port)(struct net_device *dev, > > + * sa_family_t sa_family, __be16 port); > > + * Called by geneve to notiy a driver about the UDP port and > > socket > > + * address family that geneve is listnening to. It is called > > only when > > + * a new port starts listening. The operation is protected by > > the > > + * geneve_net->sock_lock. > > + * > > + * void (*ndo_del_geneve_port)(struct net_device *dev, > > + * sa_family_t sa_family, __be16 port); > > + * Called by geneve to notify the driver about a UDP port and > > socket > > + * address family that geneve is not listening to anymore. > > The operation > > + * is protected by the geneve_net->sock_lock. > > + * > > > Would it make more sense to generalize the ndo op for future protocol > extension instead of continuing to add separate tunnel offload functions for > each one? > > > ie: generalize ndo_add_vxlan_port into "ndo_add_tunnel_port"? > > Maybe it's not worth it though..? > Not worth it at this point, should be done as a separate refactor patch involving cleaning up both vxlan and geneve modules plus all the drivers that use them. > Regards, > Jake
On Thu, Aug 20, 2015 at 4:03 PM, Singhai, Anjali <anjali.singhai@intel.com> wrote: >> -----Original Message----- >> From: Keller, Jacob E >> Sent: Thursday, August 20, 2015 1:32 PM >> To: intel-wired-lan@lists.osuosl.org; Singhai, Anjali >> Subject: Re: [Intel-wired-lan] [PATCH 2/4] geneve: Add geneve udp port >> offload for ethernet devices >> >> On Thu, 2015-08-20 at 11:46 -0700, Anjali Singhai Jain wrote: >> > Add ndo_ops to add/del UDP ports to a device that supports geneve >> > offload. >> > >> > v2:Added comments for new ndo_ops and minor format fix. >> > >> > Signed-off-by: Kiran Patil <kiran.patil@intel.com> >> > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com> >> > --- >> > include/linux/netdevice.h | 20 +++++++++++++++++++- >> > net/ipv4/geneve_core.c | 22 ++++++++++++++++++++++ >> > 2 files changed, 41 insertions(+), 1 deletion(-) >> > >> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> > index f3bb290..d6f00c7 100644 >> > --- a/include/linux/netdevice.h >> > +++ b/include/linux/netdevice.h >> > @@ -1016,6 +1016,19 @@ typedef u16 (*select_queue_fallback_t)(struct >> > net_device *dev, >> > * address family that vxlan is not listening to anymore. The >> > operation >> > * is protected by the vxlan_net->sock_lock. >> > * >> > + * void (*ndo_add_geneve_port)(struct net_device *dev, >> > + * sa_family_t sa_family, __be16 port); >> > + * Called by geneve to notiy a driver about the UDP port and >> > socket >> > + * address family that geneve is listnening to. It is called >> > only when >> > + * a new port starts listening. The operation is protected by >> > the >> > + * geneve_net->sock_lock. >> > + * >> > + * void (*ndo_del_geneve_port)(struct net_device *dev, >> > + * sa_family_t sa_family, __be16 port); >> > + * Called by geneve to notify the driver about a UDP port and >> > socket >> > + * address family that geneve is not listening to anymore. >> > The operation >> > + * is protected by the geneve_net->sock_lock. >> > + * >> >> >> Would it make more sense to generalize the ndo op for future protocol >> extension instead of continuing to add separate tunnel offload functions for >> each one? >> >> >> ie: generalize ndo_add_vxlan_port into "ndo_add_tunnel_port"? >> >> Maybe it's not worth it though..? >> > > Not worth it at this point, should be done as a separate refactor patch involving cleaning up both vxlan and geneve modules plus all the drivers that use them. I would strongly recommend making this generalization now. This has specifically come up this week at Linux Plumbers and there have been more general discussions about making this generic in the past, so you'll likely get push back without it. I would also suggest trying to get this to the netdev mailing as soon as possible, in case there are other comments on the core infrastructure.
> -----Original Message----- > From: Jesse Gross [mailto:jesse@nicira.com] > Sent: Friday, August 21, 2015 5:14 PM > To: Singhai, Anjali > Cc: Keller, Jacob E; intel-wired-lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [PATCH 2/4] geneve: Add geneve udp port > offload for ethernet devices > > On Thu, Aug 20, 2015 at 4:03 PM, Singhai, Anjali > <anjali.singhai@intel.com> wrote: > >> -----Original Message----- > >> From: Keller, Jacob E > >> Sent: Thursday, August 20, 2015 1:32 PM > >> To: intel-wired-lan@lists.osuosl.org; Singhai, Anjali > >> Subject: Re: [Intel-wired-lan] [PATCH 2/4] geneve: Add geneve udp port > >> offload for ethernet devices > >> > >> On Thu, 2015-08-20 at 11:46 -0700, Anjali Singhai Jain wrote: > >> > Add ndo_ops to add/del UDP ports to a device that supports geneve > >> > offload. > >> > > >> > v2:Added comments for new ndo_ops and minor format fix. > >> > > >> > Signed-off-by: Kiran Patil <kiran.patil@intel.com> > >> > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com> > >> > --- > >> > include/linux/netdevice.h | 20 +++++++++++++++++++- > >> > net/ipv4/geneve_core.c | 22 ++++++++++++++++++++++ > >> > 2 files changed, 41 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > >> > index f3bb290..d6f00c7 100644 > >> > --- a/include/linux/netdevice.h > >> > +++ b/include/linux/netdevice.h > >> > @@ -1016,6 +1016,19 @@ typedef u16 > (*select_queue_fallback_t)(struct > >> > net_device *dev, > >> > * address family that vxlan is not listening to anymore. The > >> > operation > >> > * is protected by the vxlan_net->sock_lock. > >> > * > >> > + * void (*ndo_add_geneve_port)(struct net_device *dev, > >> > + * sa_family_t sa_family, __be16 port); > >> > + * Called by geneve to notiy a driver about the UDP port and > >> > socket > >> > + * address family that geneve is listnening to. It is called > >> > only when > >> > + * a new port starts listening. The operation is protected by > >> > the > >> > + * geneve_net->sock_lock. > >> > + * > >> > + * void (*ndo_del_geneve_port)(struct net_device *dev, > >> > + * sa_family_t sa_family, __be16 port); > >> > + * Called by geneve to notify the driver about a UDP port and > >> > socket > >> > + * address family that geneve is not listening to anymore. > >> > The operation > >> > + * is protected by the geneve_net->sock_lock. > >> > + * > >> > >> > >> Would it make more sense to generalize the ndo op for future protocol > >> extension instead of continuing to add separate tunnel offload functions > for > >> each one? > >> > >> > >> ie: generalize ndo_add_vxlan_port into "ndo_add_tunnel_port"? > >> > >> Maybe it's not worth it though..? > >> > > > > Not worth it at this point, should be done as a separate refactor patch > involving cleaning up both vxlan and geneve modules plus all the drivers that > use them. > > I would strongly recommend making this generalization now. This has > specifically come up this week at Linux Plumbers and there have been > more general discussions about making this generic in the past, so > you'll likely get push back without it. > > I would also suggest trying to get this to the netdev mailing as soon > as possible, in case there are other comments on the core > infrastructure. Ok, I will re-spin with a generic api, although I would be just compile testing the other drivers with the new api that use the present vxlan interface. Hope that would suffice.
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index f3bb290..d6f00c7 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1016,6 +1016,19 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev, * address family that vxlan is not listening to anymore. The operation * is protected by the vxlan_net->sock_lock. * + * void (*ndo_add_geneve_port)(struct net_device *dev, + * sa_family_t sa_family, __be16 port); + * Called by geneve to notiy a driver about the UDP port and socket + * address family that geneve is listnening to. It is called only when + * a new port starts listening. The operation is protected by the + * geneve_net->sock_lock. + * + * void (*ndo_del_geneve_port)(struct net_device *dev, + * sa_family_t sa_family, __be16 port); + * Called by geneve to notify the driver about a UDP port and socket + * address family that geneve is not listening to anymore. The operation + * is protected by the geneve_net->sock_lock. + * * void* (*ndo_dfwd_add_station)(struct net_device *pdev, * struct net_device *dev) * Called by upper layer devices to accelerate switching or other @@ -1210,7 +1223,12 @@ struct net_device_ops { void (*ndo_del_vxlan_port)(struct net_device *dev, sa_family_t sa_family, __be16 port); - + void (*ndo_add_geneve_port)(struct net_device *dev, + sa_family_t sa_family, + __be16 port); + void (*ndo_del_geneve_port)(struct net_device *dev, + sa_family_t sa_family, + __be16 port); void* (*ndo_dfwd_add_station)(struct net_device *pdev, struct net_device *dev); void (*ndo_dfwd_del_station)(struct net_device *pdev, diff --git a/net/ipv4/geneve_core.c b/net/ipv4/geneve_core.c index 311a4ba..9ea9082 100644 --- a/net/ipv4/geneve_core.c +++ b/net/ipv4/geneve_core.c @@ -234,8 +234,11 @@ static int geneve_gro_complete(struct sk_buff *skb, int nhoff, static void geneve_notify_add_rx_port(struct geneve_sock *gs) { + struct net_device *dev; struct sock *sk = gs->sock->sk; + struct net *net = sock_net(sk); sa_family_t sa_family = sk->sk_family; + __be16 port = inet_sk(sk)->inet_sport; int err; if (sa_family == AF_INET) { @@ -244,12 +247,31 @@ static void geneve_notify_add_rx_port(struct geneve_sock *gs) pr_warn("geneve: udp_add_offload failed with status %d\n", err); } + + rcu_read_lock(); + for_each_netdev_rcu(net, dev) { + if (dev->netdev_ops->ndo_add_geneve_port) + dev->netdev_ops->ndo_add_geneve_port(dev, sa_family, + port); + } + rcu_read_unlock(); } static void geneve_notify_del_rx_port(struct geneve_sock *gs) { + struct net_device *dev; struct sock *sk = gs->sock->sk; + struct net *net = sock_net(sk); sa_family_t sa_family = sk->sk_family; + __be16 port = inet_sk(sk)->inet_sport; + + rcu_read_lock(); + for_each_netdev_rcu(net, dev) { + if (dev->netdev_ops->ndo_del_geneve_port) + dev->netdev_ops->ndo_del_geneve_port(dev, sa_family, + port); + } + rcu_read_unlock(); if (sa_family == AF_INET) udp_del_offload(&gs->udp_offloads);