| Message ID | 1440606998-11072-4-git-send-email-jiri@resnulli.us |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 26/08/15 09:36, Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Add this helper so code can easily figure out if netdev is openswitch. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > include/linux/netdevice.h | 8 ++++++++ > net/openvswitch/vport-internal_dev.c | 2 +- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index be625f4..0a884e6 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1264,6 +1264,7 @@ struct net_device_ops { > * @IFF_MACVLAN: Macvlan device > * @IFF_VRF_MASTER: device is a VRF master > * @IFF_NO_QUEUE: device can run without qdisc attached > + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master Typo, the flag you introduced is named IFF_OPENVSWITCH, not VFR_OPENSWITCH. > */ > enum netdev_priv_flags { > IFF_802_1Q_VLAN = 1<<0, > @@ -1293,6 +1294,7 @@ enum netdev_priv_flags { > IFF_IPVLAN_SLAVE = 1<<24, > IFF_VRF_MASTER = 1<<25, > IFF_NO_QUEUE = 1<<26, > + IFF_OPENVSWITCH = 1<<27, > }; > > #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN > @@ -1322,6 +1324,7 @@ enum netdev_priv_flags { > #define IFF_IPVLAN_SLAVE IFF_IPVLAN_SLAVE > #define IFF_VRF_MASTER IFF_VRF_MASTER > #define IFF_NO_QUEUE IFF_NO_QUEUE > +#define IFF_OPENVSWITCH IFF_OPENVSWITCH > > /** > * struct net_device - The DEVICE structure. > @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev) > return dev->priv_flags & IFF_EBRIDGE; > } > > +static inline bool netif_is_ovs_master(const struct net_device *dev) > +{ > + return dev->priv_flags & IFF_OPENVSWITCH; > +} > + > static inline bool netif_index_is_vrf(struct net *net, int ifindex) > { > bool rc = false; > diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c > index c058bbf..80b3e12 100644 > --- a/net/openvswitch/vport-internal_dev.c > +++ b/net/openvswitch/vport-internal_dev.c > @@ -135,7 +135,7 @@ static void do_setup(struct net_device *netdev) > netdev->netdev_ops = &internal_dev_netdev_ops; > > netdev->priv_flags &= ~IFF_TX_SKB_SHARING; > - netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE; > + netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_OPENVSWITCH; > netdev->destructor = internal_dev_destructor; > netdev->ethtool_ops = &internal_dev_ethtool_ops; > netdev->rtnl_link_ops = &internal_dev_link_ops; >
On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Add this helper so code can easily figure out if netdev is openswitch. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > include/linux/netdevice.h | 8 ++++++++ > net/openvswitch/vport-internal_dev.c | 2 +- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index be625f4..0a884e6 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1264,6 +1264,7 @@ struct net_device_ops { > * @IFF_MACVLAN: Macvlan device > * @IFF_VRF_MASTER: device is a VRF master > * @IFF_NO_QUEUE: device can run without qdisc attached > + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master > */ > enum netdev_priv_flags { > IFF_802_1Q_VLAN = 1<<0, > @@ -1293,6 +1294,7 @@ enum netdev_priv_flags { > IFF_IPVLAN_SLAVE = 1<<24, > IFF_VRF_MASTER = 1<<25, > IFF_NO_QUEUE = 1<<26, > + IFF_OPENVSWITCH = 1<<27, > }; > > #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN > @@ -1322,6 +1324,7 @@ enum netdev_priv_flags { > #define IFF_IPVLAN_SLAVE IFF_IPVLAN_SLAVE > #define IFF_VRF_MASTER IFF_VRF_MASTER > #define IFF_NO_QUEUE IFF_NO_QUEUE > +#define IFF_OPENVSWITCH IFF_OPENVSWITCH > > /** > * struct net_device - The DEVICE structure. > @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev) > return dev->priv_flags & IFF_EBRIDGE; > } > > +static inline bool netif_is_ovs_master(const struct net_device *dev) > +{ > + return dev->priv_flags & IFF_OPENVSWITCH; > +} We're going to run out of priv_flags bits. This flag doesn't seem like something that will be checked lots of places. How about using rtnl_link_ops->kind to save a bit in priv_flags? static inline bool netif_is_ovs_master(const struct net_device *dev) { return !strcmp(dev->rtnl_link_ops->kind, "openvswitch")); } > + > static inline bool netif_index_is_vrf(struct net *net, int ifindex) > { > bool rc = false; > diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c > index c058bbf..80b3e12 100644 > --- a/net/openvswitch/vport-internal_dev.c > +++ b/net/openvswitch/vport-internal_dev.c > @@ -135,7 +135,7 @@ static void do_setup(struct net_device *netdev) > netdev->netdev_ops = &internal_dev_netdev_ops; > > netdev->priv_flags &= ~IFF_TX_SKB_SHARING; > - netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE; > + netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_OPENVSWITCH; > netdev->destructor = internal_dev_destructor; > netdev->ethtool_ops = &internal_dev_ethtool_ops; > netdev->rtnl_link_ops = &internal_dev_link_ops; > -- > 1.9.3 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wed, Aug 26, 2015 at 07:24:55PM CEST, f.fainelli@gmail.com wrote: >On 26/08/15 09:36, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Add this helper so code can easily figure out if netdev is openswitch. >> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> --- >> include/linux/netdevice.h | 8 ++++++++ >> net/openvswitch/vport-internal_dev.c | 2 +- >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index be625f4..0a884e6 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1264,6 +1264,7 @@ struct net_device_ops { >> * @IFF_MACVLAN: Macvlan device >> * @IFF_VRF_MASTER: device is a VRF master >> * @IFF_NO_QUEUE: device can run without qdisc attached >> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master > >Typo, the flag you introduced is named IFF_OPENVSWITCH, not VFR_OPENSWITCH. Oups, will fix. Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote: >On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Add this helper so code can easily figure out if netdev is openswitch. >> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> --- >> include/linux/netdevice.h | 8 ++++++++ >> net/openvswitch/vport-internal_dev.c | 2 +- >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index be625f4..0a884e6 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1264,6 +1264,7 @@ struct net_device_ops { >> * @IFF_MACVLAN: Macvlan device >> * @IFF_VRF_MASTER: device is a VRF master >> * @IFF_NO_QUEUE: device can run without qdisc attached >> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master >> */ >> enum netdev_priv_flags { >> IFF_802_1Q_VLAN = 1<<0, >> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags { >> IFF_IPVLAN_SLAVE = 1<<24, >> IFF_VRF_MASTER = 1<<25, >> IFF_NO_QUEUE = 1<<26, >> + IFF_OPENVSWITCH = 1<<27, >> }; >> >> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN >> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags { >> #define IFF_IPVLAN_SLAVE IFF_IPVLAN_SLAVE >> #define IFF_VRF_MASTER IFF_VRF_MASTER >> #define IFF_NO_QUEUE IFF_NO_QUEUE >> +#define IFF_OPENVSWITCH IFF_OPENVSWITCH >> >> /** >> * struct net_device - The DEVICE structure. >> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev) >> return dev->priv_flags & IFF_EBRIDGE; >> } >> >> +static inline bool netif_is_ovs_master(const struct net_device *dev) >> +{ >> + return dev->priv_flags & IFF_OPENVSWITCH; >> +} > >We're going to run out of priv_flags bits. This flag doesn't seem >like something that will be checked lots of places. How about using >rtnl_link_ops->kind to save a bit in priv_flags? > >static inline bool netif_is_ovs_master(const struct net_device *dev) >{ > return !strcmp(dev->rtnl_link_ops->kind, "openvswitch")); >} There are lot of helpers like this for other soft-devices. I think that is okay to have it this way. The thing is that sometimes you need to use thi helper in fast path and in that case, you do not want to strcmp. There is plenty of priv_flags bits for now when I killed the bonding stuff. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote: > Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote: >>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>> From: Jiri Pirko <jiri@mellanox.com> >>> >>> Add this helper so code can easily figure out if netdev is openswitch. >>> >>> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >>> --- >>> include/linux/netdevice.h | 8 ++++++++ >>> net/openvswitch/vport-internal_dev.c | 2 +- >>> 2 files changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>> index be625f4..0a884e6 100644 >>> --- a/include/linux/netdevice.h >>> +++ b/include/linux/netdevice.h >>> @@ -1264,6 +1264,7 @@ struct net_device_ops { >>> * @IFF_MACVLAN: Macvlan device >>> * @IFF_VRF_MASTER: device is a VRF master >>> * @IFF_NO_QUEUE: device can run without qdisc attached >>> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master >>> */ >>> enum netdev_priv_flags { >>> IFF_802_1Q_VLAN = 1<<0, >>> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags { >>> IFF_IPVLAN_SLAVE = 1<<24, >>> IFF_VRF_MASTER = 1<<25, >>> IFF_NO_QUEUE = 1<<26, >>> + IFF_OPENVSWITCH = 1<<27, >>> }; >>> >>> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN >>> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags { >>> #define IFF_IPVLAN_SLAVE IFF_IPVLAN_SLAVE >>> #define IFF_VRF_MASTER IFF_VRF_MASTER >>> #define IFF_NO_QUEUE IFF_NO_QUEUE >>> +#define IFF_OPENVSWITCH IFF_OPENVSWITCH >>> >>> /** >>> * struct net_device - The DEVICE structure. >>> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev) >>> return dev->priv_flags & IFF_EBRIDGE; >>> } >>> >>> +static inline bool netif_is_ovs_master(const struct net_device *dev) >>> +{ >>> + return dev->priv_flags & IFF_OPENVSWITCH; >>> +} >> >>We're going to run out of priv_flags bits. This flag doesn't seem >>like something that will be checked lots of places. How about using >>rtnl_link_ops->kind to save a bit in priv_flags? >> >>static inline bool netif_is_ovs_master(const struct net_device *dev) >>{ >> return !strcmp(dev->rtnl_link_ops->kind, "openvswitch")); >>} > > There are lot of helpers like this for other soft-devices. I think that > is okay to have it this way. The thing is that sometimes you need to use > thi helper in fast path and in that case, you do not want to strcmp. > > There is plenty of priv_flags bits for now when I killed the bonding > stuff. Ya, but think about the bit: you (and others) used a bit in priv_flags to indicate the netdev type. Can you add an enum field to rtnl_link_ops->type to indicate link type? Then it's not a strcmp. You can write your helper using strcmp first, and then later migrate to using rtnl_link_ops->type. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thu, Aug 27, 2015 at 08:23:13AM CEST, sfeldma@gmail.com wrote: >On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote: >> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote: >>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>>> From: Jiri Pirko <jiri@mellanox.com> >>>> >>>> Add this helper so code can easily figure out if netdev is openswitch. >>>> >>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >>>> --- >>>> include/linux/netdevice.h | 8 ++++++++ >>>> net/openvswitch/vport-internal_dev.c | 2 +- >>>> 2 files changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>>> index be625f4..0a884e6 100644 >>>> --- a/include/linux/netdevice.h >>>> +++ b/include/linux/netdevice.h >>>> @@ -1264,6 +1264,7 @@ struct net_device_ops { >>>> * @IFF_MACVLAN: Macvlan device >>>> * @IFF_VRF_MASTER: device is a VRF master >>>> * @IFF_NO_QUEUE: device can run without qdisc attached >>>> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master >>>> */ >>>> enum netdev_priv_flags { >>>> IFF_802_1Q_VLAN = 1<<0, >>>> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags { >>>> IFF_IPVLAN_SLAVE = 1<<24, >>>> IFF_VRF_MASTER = 1<<25, >>>> IFF_NO_QUEUE = 1<<26, >>>> + IFF_OPENVSWITCH = 1<<27, >>>> }; >>>> >>>> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN >>>> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags { >>>> #define IFF_IPVLAN_SLAVE IFF_IPVLAN_SLAVE >>>> #define IFF_VRF_MASTER IFF_VRF_MASTER >>>> #define IFF_NO_QUEUE IFF_NO_QUEUE >>>> +#define IFF_OPENVSWITCH IFF_OPENVSWITCH >>>> >>>> /** >>>> * struct net_device - The DEVICE structure. >>>> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev) >>>> return dev->priv_flags & IFF_EBRIDGE; >>>> } >>>> >>>> +static inline bool netif_is_ovs_master(const struct net_device *dev) >>>> +{ >>>> + return dev->priv_flags & IFF_OPENVSWITCH; >>>> +} >>> >>>We're going to run out of priv_flags bits. This flag doesn't seem >>>like something that will be checked lots of places. How about using >>>rtnl_link_ops->kind to save a bit in priv_flags? >>> >>>static inline bool netif_is_ovs_master(const struct net_device *dev) >>>{ >>> return !strcmp(dev->rtnl_link_ops->kind, "openvswitch")); >>>} >> >> There are lot of helpers like this for other soft-devices. I think that >> is okay to have it this way. The thing is that sometimes you need to use >> thi helper in fast path and in that case, you do not want to strcmp. >> >> There is plenty of priv_flags bits for now when I killed the bonding >> stuff. > >Ya, but think about the bit: you (and others) used a bit in priv_flags >to indicate the netdev type. Can you add an enum field to >rtnl_link_ops->type to indicate link type? Then it's not a strcmp. >You can write your helper using strcmp first, and then later migrate >to using rtnl_link_ops->type. But how different this would be to the priv_flags? You have to have "central storage" for these types anyway, same as for flags. + it's one more pointer dereference and null check on fastpath. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thu, Aug 27, 2015 at 08:23:13AM CEST, sfeldma@gmail.com wrote: >On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote: >> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote: >>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>>> From: Jiri Pirko <jiri@mellanox.com> >>>> >>>> Add this helper so code can easily figure out if netdev is openswitch. >>>> >>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >>>> --- >>>> include/linux/netdevice.h | 8 ++++++++ >>>> net/openvswitch/vport-internal_dev.c | 2 +- >>>> 2 files changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>>> index be625f4..0a884e6 100644 >>>> --- a/include/linux/netdevice.h >>>> +++ b/include/linux/netdevice.h >>>> @@ -1264,6 +1264,7 @@ struct net_device_ops { >>>> * @IFF_MACVLAN: Macvlan device >>>> * @IFF_VRF_MASTER: device is a VRF master >>>> * @IFF_NO_QUEUE: device can run without qdisc attached >>>> + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master >>>> */ >>>> enum netdev_priv_flags { >>>> IFF_802_1Q_VLAN = 1<<0, >>>> @@ -1293,6 +1294,7 @@ enum netdev_priv_flags { >>>> IFF_IPVLAN_SLAVE = 1<<24, >>>> IFF_VRF_MASTER = 1<<25, >>>> IFF_NO_QUEUE = 1<<26, >>>> + IFF_OPENVSWITCH = 1<<27, >>>> }; >>>> >>>> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN >>>> @@ -1322,6 +1324,7 @@ enum netdev_priv_flags { >>>> #define IFF_IPVLAN_SLAVE IFF_IPVLAN_SLAVE >>>> #define IFF_VRF_MASTER IFF_VRF_MASTER >>>> #define IFF_NO_QUEUE IFF_NO_QUEUE >>>> +#define IFF_OPENVSWITCH IFF_OPENVSWITCH >>>> >>>> /** >>>> * struct net_device - The DEVICE structure. >>>> @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev) >>>> return dev->priv_flags & IFF_EBRIDGE; >>>> } >>>> >>>> +static inline bool netif_is_ovs_master(const struct net_device *dev) >>>> +{ >>>> + return dev->priv_flags & IFF_OPENVSWITCH; >>>> +} >>> >>>We're going to run out of priv_flags bits. This flag doesn't seem >>>like something that will be checked lots of places. How about using >>>rtnl_link_ops->kind to save a bit in priv_flags? >>> >>>static inline bool netif_is_ovs_master(const struct net_device *dev) >>>{ >>> return !strcmp(dev->rtnl_link_ops->kind, "openvswitch")); >>>} >> >> There are lot of helpers like this for other soft-devices. I think that >> is okay to have it this way. The thing is that sometimes you need to use >> thi helper in fast path and in that case, you do not want to strcmp. >> >> There is plenty of priv_flags bits for now when I killed the bonding >> stuff. > >Ya, but think about the bit: you (and others) used a bit in priv_flags >to indicate the netdev type. Can you add an enum field to >rtnl_link_ops->type to indicate link type? Then it's not a strcmp. >You can write your helper using strcmp first, and then later migrate >to using rtnl_link_ops->type. Also, dev can be multiple things, it can be bridge port and vlan dev at the same time. Flags are good for this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 26, 2015 at 11:30 PM, Jiri Pirko <jiri@resnulli.us> wrote: > Thu, Aug 27, 2015 at 08:23:13AM CEST, sfeldma@gmail.com wrote: >>On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote: >>> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote: >>>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>>>> From: Jiri Pirko <jiri@mellanox.com> >>>>We're going to run out of priv_flags bits. This flag doesn't seem >>>>like something that will be checked lots of places. How about using >>>>rtnl_link_ops->kind to save a bit in priv_flags? >>>> >>>>static inline bool netif_is_ovs_master(const struct net_device *dev) >>>>{ >>>> return !strcmp(dev->rtnl_link_ops->kind, "openvswitch")); >>>>} >>> >>> There are lot of helpers like this for other soft-devices. I think that >>> is okay to have it this way. The thing is that sometimes you need to use >>> thi helper in fast path and in that case, you do not want to strcmp. >>> >>> There is plenty of priv_flags bits for now when I killed the bonding >>> stuff. >> >>Ya, but think about the bit: you (and others) used a bit in priv_flags >>to indicate the netdev type. Can you add an enum field to >>rtnl_link_ops->type to indicate link type? Then it's not a strcmp. >>You can write your helper using strcmp first, and then later migrate >>to using rtnl_link_ops->type. > > > Also, dev can be multiple things, it can be bridge port and vlan dev at > the same time. Flags are good for this. priv_flags bits are three types: 1) dev attribute (IFF_XMIT_DST_RELEASE, IFF_DISABLE_NETPOLL, etc) 2) dev type (IFF_802_1Q_VLAN, IFF_EBRIDGE, etc) 3) and dev membership (IFF_BRIDGE_PORT, IFF_TEAM_PORT, etc) Are there types 2 or 3 in any fast paths? Type 2 can move to enum; they're mutually exclusive. Type 3 is the dev's master's type 2, and since dev can have only one master, no flag is needed: just look up master type to know if dev is bridged or ovs'ed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thu, Aug 27, 2015 at 09:51:52AM CEST, sfeldma@gmail.com wrote: >On Wed, Aug 26, 2015 at 11:30 PM, Jiri Pirko <jiri@resnulli.us> wrote: >> Thu, Aug 27, 2015 at 08:23:13AM CEST, sfeldma@gmail.com wrote: >>>On Wed, Aug 26, 2015 at 10:43 PM, Jiri Pirko <jiri@resnulli.us> wrote: >>>> Wed, Aug 26, 2015 at 07:43:18PM CEST, sfeldma@gmail.com wrote: >>>>>On Wed, Aug 26, 2015 at 9:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>>>>> From: Jiri Pirko <jiri@mellanox.com> > >>>>>We're going to run out of priv_flags bits. This flag doesn't seem >>>>>like something that will be checked lots of places. How about using >>>>>rtnl_link_ops->kind to save a bit in priv_flags? >>>>> >>>>>static inline bool netif_is_ovs_master(const struct net_device *dev) >>>>>{ >>>>> return !strcmp(dev->rtnl_link_ops->kind, "openvswitch")); >>>>>} >>>> >>>> There are lot of helpers like this for other soft-devices. I think that >>>> is okay to have it this way. The thing is that sometimes you need to use >>>> thi helper in fast path and in that case, you do not want to strcmp. >>>> >>>> There is plenty of priv_flags bits for now when I killed the bonding >>>> stuff. >>> >>>Ya, but think about the bit: you (and others) used a bit in priv_flags >>>to indicate the netdev type. Can you add an enum field to >>>rtnl_link_ops->type to indicate link type? Then it's not a strcmp. >>>You can write your helper using strcmp first, and then later migrate >>>to using rtnl_link_ops->type. >> >> >> Also, dev can be multiple things, it can be bridge port and vlan dev at >> the same time. Flags are good for this. > >priv_flags bits are three types: > >1) dev attribute (IFF_XMIT_DST_RELEASE, IFF_DISABLE_NETPOLL, etc) >2) dev type (IFF_802_1Q_VLAN, IFF_EBRIDGE, etc) >3) and dev membership (IFF_BRIDGE_PORT, IFF_TEAM_PORT, etc) > >Are there types 2 or 3 in any fast paths? Type 2 can move to enum; >they're mutually exclusive. Type 3 is the dev's master's type 2, and >since dev can have only one master, no flag is needed: just look up >master type to know if dev is bridged or ovs'ed. 3 is certainly used in fast-path. This priv_flags stuff could certainly get better somehow. I don't think that using rtnl_link_ops is the answer here. It is certainly out of scope of this patchset. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index be625f4..0a884e6 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1264,6 +1264,7 @@ struct net_device_ops { * @IFF_MACVLAN: Macvlan device * @IFF_VRF_MASTER: device is a VRF master * @IFF_NO_QUEUE: device can run without qdisc attached + * @IFF_VRF_OPENVSWITCH: device is a Open vSwitch master */ enum netdev_priv_flags { IFF_802_1Q_VLAN = 1<<0, @@ -1293,6 +1294,7 @@ enum netdev_priv_flags { IFF_IPVLAN_SLAVE = 1<<24, IFF_VRF_MASTER = 1<<25, IFF_NO_QUEUE = 1<<26, + IFF_OPENVSWITCH = 1<<27, }; #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN @@ -1322,6 +1324,7 @@ enum netdev_priv_flags { #define IFF_IPVLAN_SLAVE IFF_IPVLAN_SLAVE #define IFF_VRF_MASTER IFF_VRF_MASTER #define IFF_NO_QUEUE IFF_NO_QUEUE +#define IFF_OPENVSWITCH IFF_OPENVSWITCH /** * struct net_device - The DEVICE structure. @@ -3853,6 +3856,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev) return dev->priv_flags & IFF_EBRIDGE; } +static inline bool netif_is_ovs_master(const struct net_device *dev) +{ + return dev->priv_flags & IFF_OPENVSWITCH; +} + static inline bool netif_index_is_vrf(struct net *net, int ifindex) { bool rc = false; diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c index c058bbf..80b3e12 100644 --- a/net/openvswitch/vport-internal_dev.c +++ b/net/openvswitch/vport-internal_dev.c @@ -135,7 +135,7 @@ static void do_setup(struct net_device *netdev) netdev->netdev_ops = &internal_dev_netdev_ops; netdev->priv_flags &= ~IFF_TX_SKB_SHARING; - netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE; + netdev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_OPENVSWITCH; netdev->destructor = internal_dev_destructor; netdev->ethtool_ops = &internal_dev_ethtool_ops; netdev->rtnl_link_ops = &internal_dev_link_ops;