* Re: [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports
From: Vlad Yasevich @ 2012-08-23 19:41 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20120823123809.1a4adfbe@nehalam.linuxnetplumber.net>
On 08/23/2012 03:38 PM, Stephen Hemminger wrote:
> On Thu, 23 Aug 2012 15:29:54 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>> Add a private ioctl to add and remove vlan configuration on bridge port.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>> include/linux/if_bridge.h | 2 +
>> net/bridge/br_if.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>> net/bridge/br_ioctl.c | 31 ++++++++++++++++++++
>> net/bridge/br_private.h | 2 +
>> 4 files changed, 104 insertions(+), 0 deletions(-)
>
>
> Don't go down this dead end.
>
> The ioctl interface to bridge is deprecated because private ioctl's
> can not be handled when running 32 bit user space on 64 bit kernel.
>
Went down this road because is was easier to do. The plan is to do this
over netlink
-vlad
^ permalink raw reply
* Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries
From: Stephen Hemminger @ 2012-08-23 19:39 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <1345750195-31598-3-git-send-email-vyasevic@redhat.com>
On Thu, 23 Aug 2012 15:29:52 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index dd3f201..288ff10 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -95,6 +95,7 @@ struct __fdb_entry {
> __u8 port_hi;
> __u8 pad0;
> __u16 unused;
> +#define fdb_vid unused
That is seriously ugly. Just change the structure definition
to use the value.
^ permalink raw reply
* Re: [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports
From: Stephen Hemminger @ 2012-08-23 19:38 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <1345750195-31598-5-git-send-email-vyasevic@redhat.com>
On Thu, 23 Aug 2012 15:29:54 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:
> Add a private ioctl to add and remove vlan configuration on bridge port.
>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
> include/linux/if_bridge.h | 2 +
> net/bridge/br_if.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
> net/bridge/br_ioctl.c | 31 ++++++++++++++++++++
> net/bridge/br_private.h | 2 +
> 4 files changed, 104 insertions(+), 0 deletions(-)
Don't go down this dead end.
The ioctl interface to bridge is deprecated because private ioctl's
can not be handled when running 32 bit user space on 64 bit kernel.
^ permalink raw reply
* [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS
From: Vlad Yasevich @ 2012-08-23 19:29 UTC (permalink / raw)
To: netdev; +Cc: Vlad Yasevich
In-Reply-To: <1345750195-31598-1-git-send-email-vyasevic@redhat.com>
Add a binary sysfs file that will dump out vlans currently configured on the
port.
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
include/linux/if_bridge.h | 1 +
net/bridge/br_if.c | 34 ++++++++++++++++++++++++++++++++++
net/bridge/br_private.h | 2 ++
net/bridge/br_sysfs_if.c | 28 ++++++++++++++++++++++++++++
4 files changed, 65 insertions(+), 0 deletions(-)
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index ab750dd..d0f869b 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -20,6 +20,7 @@
#define SYSFS_BRIDGE_PORT_SUBDIR "brif"
#define SYSFS_BRIDGE_PORT_ATTR "brport"
#define SYSFS_BRIDGE_PORT_LINK "bridge"
+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
#define BRCTL_VERSION 1
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 90c1038..3963748 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
return 0;
}
+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
+ unsigned long max, unsigned long skip)
+{
+ unsigned long *map;
+ unsigned short *vid = (unsigned short *)buf;
+ unsigned short i;
+ int num = 0;
+
+ if (skip > (VLAN_N_VID+1))
+ return -EINVAL;
+
+ memset(buf, 0, max * sizeof(unsigned short));
+
+ rcu_read_lock();
+ map = rcu_dereference(p->vlan_map);
+ if (!map)
+ goto out;
+
+ for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
+ if (test_bit(i, map)) {
+ if (num > max)
+ goto out;
+
+ *vid = i-1;
+ vid++;
+ num++;
+ }
+ }
+out:
+ rcu_read_unlock();
+
+ return num*sizeof(unsigned short);
+}
+
void __net_exit br_net_exit(struct net *net)
{
struct net_device *dev;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 5639c1c..cf95cd7 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
netdev_features_t features);
extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
+ unsigned long max, unsigned long skip);
/* br_input.c */
extern int br_handle_frame_finish(struct sk_buff *skb);
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 13b36bd..a81e2ef 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
};
/*
+ * Export the vlan table for a given port as a binary file.
+ * The records are unsgined shorts.
+ *
+ * Returns the number of bytes read.
+ */
+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr,
+ char *buf, loff_t off, size_t count)
+{
+ struct net_bridge_port *p = to_brport(kobj);
+
+ return br_port_fill_vlans(p, buf,
+ count/sizeof(unsigned short),
+ off/sizeof(unsigned short));
+}
+
+static struct bin_attribute port_vlans = {
+ .attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
+ .mode = S_IRUGO, },
+ .read = brport_vlans_read,
+};
+
+/*
* Add sysfs entries to ethernet device added to a bridge.
* Creates a brport subdirectory with bridge attributes.
* Puts symlink in bridge's brif subdirectory
@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
return err;
}
+ err = sysfs_create_bin_file(&p->kobj, &port_vlans);
+ if (err) {
+ return err;
+ }
+
strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
}
--
1.7.7.6
^ permalink raw reply related
* [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports
From: Vlad Yasevich @ 2012-08-23 19:29 UTC (permalink / raw)
To: netdev; +Cc: Vlad Yasevich
In-Reply-To: <1345750195-31598-1-git-send-email-vyasevic@redhat.com>
Add a private ioctl to add and remove vlan configuration on bridge port.
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
include/linux/if_bridge.h | 2 +
net/bridge/br_if.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
net/bridge/br_ioctl.c | 31 ++++++++++++++++++++
net/bridge/br_private.h | 2 +
4 files changed, 104 insertions(+), 0 deletions(-)
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 288ff10..ab750dd 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -42,6 +42,8 @@
#define BRCTL_SET_PORT_PRIORITY 16
#define BRCTL_SET_PATH_COST 17
#define BRCTL_GET_FDB_ENTRIES 18
+#define BRCTL_ADD_VLAN 19
+#define BRCTL_DEL_VLAN 20
#define BR_STATE_DISABLED 0
#define BR_STATE_LISTENING 1
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index e1144e1..90c1038 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -23,6 +23,7 @@
#include <linux/if_ether.h>
#include <linux/slab.h>
#include <net/sock.h>
+#include <linux/if_vlan.h>
#include "br_private.h"
@@ -441,6 +442,74 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
return 0;
}
+/* Called with RTNL */
+int br_set_port_vlan(struct net_bridge_port *p, unsigned long vlan)
+{
+ unsigned long table_size = (VLAN_N_VID/sizeof(unsigned long)) + 1;
+ unsigned long *vid_map = NULL;
+ __u16 vid = (__u16) vlan + 1;
+
+ /* We are under lock so we can check this without rcu.
+ * The vlan map is indexed by vid+1. This way we can store
+ * vid 0 (untagged) into the map as well.
+ */
+ if (!p->vlan_map) {
+ vid_map = kzalloc(table_size, GFP_KERNEL);
+ if (!vid_map)
+ return -ENOMEM;
+
+ set_bit(vid, vid_map);
+ rcu_assign_pointer(p->vlan_map, vid_map);
+
+ } else {
+ /* Map is already allocated */
+ set_bit(vid, p->vlan_map);
+ }
+
+ return 0;
+}
+
+
+/* Called with RTNL */
+int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
+{
+ unsigned long first_bit;
+ unsigned long next_bit;
+ __u16 vid = (__u16) vlan+1;
+ unsigned long tbl_len = VLAN_N_VID+1;
+
+ if (!p->vlan_map)
+ return -EINVAL;
+
+ if (!test_bit(vlan, p->vlan_map))
+ return -EINVAL;
+
+ /* Check to see if any other vlans are in this table. If this
+ * is the last vlan, delete the whole table. If this is not the
+ * last vlan, just clear the bit.
+ */
+ first_bit = find_first_bit(p->vlan_map, tbl_len);
+ next_bit = find_next_bit(p->vlan_map, tbl_len, (tbl_len - vid));
+
+ if ((__u16)first_bit != vid || (__u16)next_bit < tbl_len) {
+ /* There are other vlans still configured. We can simply
+ * clear our bit and be safe.
+ */
+ clear_bit(vid, p->vlan_map);
+ } else {
+ /* This is the last vlan we are removing. Replace the
+ * map with a NULL pointer and free the old map
+ */
+ unsigned long *map = rcu_dereference(p->vlan_map);
+
+ rcu_assign_pointer(p->vlan_map, NULL);
+ synchronize_net();
+ kfree(map);
+ }
+
+ return 0;
+}
+
void __net_exit br_net_exit(struct net *net)
{
struct net_device *dev;
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index 7222fe1..3a5b1f9 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -289,6 +289,37 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
case BRCTL_GET_FDB_ENTRIES:
return get_fdb_entries(br, (void __user *)args[1],
args[2], args[3]);
+ case BRCTL_ADD_VLAN:
+ {
+ struct net_bridge_port *p;
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ rcu_read_lock();
+ if ((p = br_get_port(br, args[1])) == NULL) {
+ rcu_read_unlock();
+ return -EINVAL;
+ }
+ rcu_read_unlock();
+ return br_set_port_vlan(p, args[2]);
+ }
+
+ case BRCTL_DEL_VLAN:
+ {
+ struct net_bridge_port *p;
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ rcu_read_lock();
+ if ((p = br_get_port(br, args[1])) == NULL) {
+ rcu_read_unlock();
+ return -EINVAL;
+ }
+ rcu_read_unlock();
+ br_set_port_vlan(p, args[2]);
+ }
}
return -EOPNOTSUPP;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index b6c56ab..5639c1c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -402,6 +402,8 @@ extern int br_del_if(struct net_bridge *br,
extern int br_min_mtu(const struct net_bridge *br);
extern netdev_features_t br_features_recompute(struct net_bridge *br,
netdev_features_t features);
+extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
+extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
/* br_input.c */
extern int br_handle_frame_finish(struct sk_buff *skb);
--
1.7.7.6
^ permalink raw reply related
* [RFC PATCH bridge 3/5] bridge: Add vlan id to multicast groups
From: Vlad Yasevich @ 2012-08-23 19:29 UTC (permalink / raw)
To: netdev; +Cc: Vlad Yasevich
In-Reply-To: <1345750195-31598-1-git-send-email-vyasevic@redhat.com>
Add vlan_id to multicasts groups so that we know which vlan each group belongs
to and can correctly forward to appropriate vlan.
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
net/bridge/br_multicast.c | 64 +++++++++++++++++++++++++++++++--------------
net/bridge/br_private.h | 1 +
2 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2417434..2976a2b 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -51,6 +51,8 @@ static inline int br_ip_equal(const struct br_ip *a, const struct br_ip *b)
{
if (a->proto != b->proto)
return 0;
+ if (a->vid != b->vid)
+ return 0;
switch (a->proto) {
case htons(ETH_P_IP):
return a->u.ip4 == b->u.ip4;
@@ -62,16 +64,19 @@ static inline int br_ip_equal(const struct br_ip *a, const struct br_ip *b)
return 0;
}
-static inline int __br_ip4_hash(struct net_bridge_mdb_htable *mdb, __be32 ip)
+static inline int __br_ip4_hash(struct net_bridge_mdb_htable *mdb, __be32 ip,
+ __u16 vid)
{
- return jhash_1word(mdb->secret, (__force u32)ip) & (mdb->max - 1);
+ return jhash_2words((__force u32)ip, vid, mdb->secret) & (mdb->max - 1);
}
#if IS_ENABLED(CONFIG_IPV6)
static inline int __br_ip6_hash(struct net_bridge_mdb_htable *mdb,
- const struct in6_addr *ip)
+ const struct in6_addr *ip,
+ __u16 vid)
{
- return jhash2((__force u32 *)ip->s6_addr32, 4, mdb->secret) & (mdb->max - 1);
+ u32 addr = *(__force u32 *)ip->s6_addr32;
+ return jhash_2words(addr, vid, mdb->secret) & (mdb->max - 1);
}
#endif
@@ -80,10 +85,10 @@ static inline int br_ip_hash(struct net_bridge_mdb_htable *mdb,
{
switch (ip->proto) {
case htons(ETH_P_IP):
- return __br_ip4_hash(mdb, ip->u.ip4);
+ return __br_ip4_hash(mdb, ip->u.ip4, ip->vid);
#if IS_ENABLED(CONFIG_IPV6)
case htons(ETH_P_IPV6):
- return __br_ip6_hash(mdb, &ip->u.ip6);
+ return __br_ip6_hash(mdb, &ip->u.ip6, ip->vid);
#endif
}
return 0;
@@ -113,24 +118,27 @@ static struct net_bridge_mdb_entry *br_mdb_ip_get(
}
static struct net_bridge_mdb_entry *br_mdb_ip4_get(
- struct net_bridge_mdb_htable *mdb, __be32 dst)
+ struct net_bridge_mdb_htable *mdb, __be32 dst, __u16 vlan_tci)
{
struct br_ip br_dst;
br_dst.u.ip4 = dst;
br_dst.proto = htons(ETH_P_IP);
+ br_dst.vid = (vlan_tci & VLAN_VID_MASK);
return br_mdb_ip_get(mdb, &br_dst);
}
#if IS_ENABLED(CONFIG_IPV6)
static struct net_bridge_mdb_entry *br_mdb_ip6_get(
- struct net_bridge_mdb_htable *mdb, const struct in6_addr *dst)
+ struct net_bridge_mdb_htable *mdb, const struct in6_addr *dst,
+ __u16 vlan_tci)
{
struct br_ip br_dst;
br_dst.u.ip6 = *dst;
br_dst.proto = htons(ETH_P_IPV6);
+ br_dst.vid = vlan_tci & VLAN_VID_MASK;
return br_mdb_ip_get(mdb, &br_dst);
}
@@ -692,7 +700,8 @@ err:
static int br_ip4_multicast_add_group(struct net_bridge *br,
struct net_bridge_port *port,
- __be32 group)
+ __be32 group,
+ __u16 vlan_tci)
{
struct br_ip br_group;
@@ -701,6 +710,7 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
br_group.u.ip4 = group;
br_group.proto = htons(ETH_P_IP);
+ br_group.vid = vlan_tci & VLAN_VID_MASK;
return br_multicast_add_group(br, port, &br_group);
}
@@ -708,7 +718,8 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
#if IS_ENABLED(CONFIG_IPV6)
static int br_ip6_multicast_add_group(struct net_bridge *br,
struct net_bridge_port *port,
- const struct in6_addr *group)
+ const struct in6_addr *group,
+ __u16 vlan_tci)
{
struct br_ip br_group;
@@ -717,6 +728,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
br_group.u.ip6 = *group;
br_group.proto = htons(ETH_P_IPV6);
+ br_group.vid = vlan_tci & VLAN_VID_MASK;
return br_multicast_add_group(br, port, &br_group);
}
@@ -928,7 +940,8 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
continue;
}
- err = br_ip4_multicast_add_group(br, port, group);
+ err = br_ip4_multicast_add_group(br, port, group,
+ skb->vlan_tci);
if (err)
break;
}
@@ -988,7 +1001,8 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
continue;
}
- err = br_ip6_multicast_add_group(br, port, &grec->grec_mca);
+ err = br_ip6_multicast_add_group(br, port, &grec->grec_mca,
+ skb->vlan_tci);
if (!err)
break;
}
@@ -1106,7 +1120,8 @@ static int br_ip4_multicast_query(struct net_bridge *br,
if (!group)
goto out;
- mp = br_mdb_ip4_get(mlock_dereference(br->mdb, br), group);
+ mp = br_mdb_ip4_get(mlock_dereference(br->mdb, br), group,
+ skb->vlan_tci);
if (!mp)
goto out;
@@ -1178,7 +1193,8 @@ static int br_ip6_multicast_query(struct net_bridge *br,
if (!group)
goto out;
- mp = br_mdb_ip6_get(mlock_dereference(br->mdb, br), group);
+ mp = br_mdb_ip6_get(mlock_dereference(br->mdb, br), group,
+ skb->vlan_tci);
if (!mp)
goto out;
@@ -1262,7 +1278,8 @@ out:
static void br_ip4_multicast_leave_group(struct net_bridge *br,
struct net_bridge_port *port,
- __be32 group)
+ __be32 group,
+ __u16 vlan_tci)
{
struct br_ip br_group;
@@ -1271,6 +1288,7 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
br_group.u.ip4 = group;
br_group.proto = htons(ETH_P_IP);
+ br_group.vid = (vlan_tci & VLAN_VID_MASK);
br_multicast_leave_group(br, port, &br_group);
}
@@ -1278,7 +1296,8 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
#if IS_ENABLED(CONFIG_IPV6)
static void br_ip6_multicast_leave_group(struct net_bridge *br,
struct net_bridge_port *port,
- const struct in6_addr *group)
+ const struct in6_addr *group,
+ __u16 vlan_tci)
{
struct br_ip br_group;
@@ -1287,6 +1306,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge *br,
br_group.u.ip6 = *group;
br_group.proto = htons(ETH_P_IPV6);
+ br_group.vid = (vlan_tci & VLAN_VID_MASK);
br_multicast_leave_group(br, port, &br_group);
}
@@ -1369,7 +1389,8 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
case IGMP_HOST_MEMBERSHIP_REPORT:
case IGMPV2_HOST_MEMBERSHIP_REPORT:
BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
- err = br_ip4_multicast_add_group(br, port, ih->group);
+ err = br_ip4_multicast_add_group(br, port, ih->group,
+ skb2->vlan_tci);
break;
case IGMPV3_HOST_MEMBERSHIP_REPORT:
err = br_ip4_multicast_igmp3_report(br, port, skb2);
@@ -1378,7 +1399,8 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
err = br_ip4_multicast_query(br, port, skb2);
break;
case IGMP_HOST_LEAVE_MESSAGE:
- br_ip4_multicast_leave_group(br, port, ih->group);
+ br_ip4_multicast_leave_group(br, port, ih->group,
+ skb2->vlan_tci);
break;
}
@@ -1498,7 +1520,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
}
mld = (struct mld_msg *)skb_transport_header(skb2);
BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
- err = br_ip6_multicast_add_group(br, port, &mld->mld_mca);
+ err = br_ip6_multicast_add_group(br, port, &mld->mld_mca,
+ skb2->vlan_tci);
break;
}
case ICMPV6_MLD2_REPORT:
@@ -1515,7 +1538,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
goto out;
}
mld = (struct mld_msg *)skb_transport_header(skb2);
- br_ip6_multicast_leave_group(br, port, &mld->mld_mca);
+ br_ip6_multicast_leave_group(br, port, &mld->mld_mca,
+ skb2->vlan_tci);
}
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 921b927..b6c56ab 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -61,6 +61,7 @@ struct br_ip
#endif
} u;
__be16 proto;
+ __u16 vid;
};
struct net_bridge_fdb_entry
--
1.7.7.6
^ permalink raw reply related
* [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries
From: Vlad Yasevich @ 2012-08-23 19:29 UTC (permalink / raw)
To: netdev; +Cc: Vlad Yasevich
In-Reply-To: <1345750195-31598-1-git-send-email-vyasevic@redhat.com>
This patch adds vlan to unicast fdb entries that are created for
learned addresses (not the manually configured ones). It adds
vlan id into the hash mix and uses vlan as an addditional parameter
for an entry match.
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
include/linux/if_bridge.h | 1 +
net/bridge/br_device.c | 2 +-
net/bridge/br_fdb.c | 71 ++++++++++++++++++++++++++------------------
net/bridge/br_input.c | 14 ++++++---
net/bridge/br_private.h | 7 +++-
5 files changed, 58 insertions(+), 37 deletions(-)
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index dd3f201..288ff10 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -95,6 +95,7 @@ struct __fdb_entry {
__u8 port_hi;
__u8 pad0;
__u16 unused;
+#define fdb_vid unused
};
#ifdef __KERNEL__
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 3334845..e321b9d 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -66,7 +66,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
br_multicast_deliver(mdst, skb);
else
br_flood_deliver(br, skb);
- } else if ((dst = __br_fdb_get(br, dest)) != NULL)
+ } else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) != NULL)
br_deliver(dst->dst, skb);
else
br_flood_deliver(br, skb);
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index d21f323..478ca1d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -23,6 +23,7 @@
#include <linux/slab.h>
#include <linux/atomic.h>
#include <asm/unaligned.h>
+#include <linux/if_vlan.h>
#include "br_private.h"
static struct kmem_cache *br_fdb_cache __read_mostly;
@@ -67,11 +68,12 @@ static inline int has_expired(const struct net_bridge *br,
time_before_eq(fdb->updated + hold_time(br), jiffies);
}
-static inline int br_mac_hash(const unsigned char *mac)
+static inline int br_mac_hash(const unsigned char *mac, __u16 vlan_tci)
{
- /* use 1 byte of OUI cnd 3 bytes of NIC */
+ /* use 1 byte of OUI and 3 bytes of NIC */
u32 key = get_unaligned((u32 *)(mac + 2));
- return jhash_1word(key, fdb_salt) & (BR_HASH_SIZE - 1);
+ return jhash_2words(key, (vlan_tci & VLAN_VID_MASK),
+ fdb_salt) & (BR_HASH_SIZE - 1);
}
static void fdb_rcu_free(struct rcu_head *head)
@@ -132,7 +134,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
struct net_bridge_fdb_entry *f;
/* If old entry was unassociated with any port, then delete it. */
- f = __br_fdb_get(br, br->dev->dev_addr);
+ f = __br_fdb_get(br, br->dev->dev_addr, 0);
if (f && f->is_local && !f->dst)
fdb_delete(br, f);
@@ -231,13 +233,16 @@ void br_fdb_delete_by_port(struct net_bridge *br,
/* No locking or refcounting, assumes caller has rcu_read_lock */
struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
- const unsigned char *addr)
+ const unsigned char *addr,
+ __u16 vlan_tci)
{
struct hlist_node *h;
struct net_bridge_fdb_entry *fdb;
- hlist_for_each_entry_rcu(fdb, h, &br->hash[br_mac_hash(addr)], hlist) {
- if (ether_addr_equal(fdb->addr.addr, addr)) {
+ hlist_for_each_entry_rcu(fdb, h,
+ &br->hash[br_mac_hash(addr, vlan_tci)], hlist) {
+ if (ether_addr_equal(fdb->addr.addr, addr) &&
+ fdb->vlan_id == (vlan_tci & VLAN_VID_MASK) ) {
if (unlikely(has_expired(br, fdb)))
break;
return fdb;
@@ -261,7 +266,7 @@ int br_fdb_test_addr(struct net_device *dev, unsigned char *addr)
if (!port)
ret = 0;
else {
- fdb = __br_fdb_get(port->br, addr);
+ fdb = __br_fdb_get(port->br, addr, 0);
ret = fdb && fdb->dst && fdb->dst->dev != dev &&
fdb->dst->state == BR_STATE_FORWARDING;
}
@@ -313,6 +318,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
fe->is_local = f->is_local;
if (!f->is_static)
fe->ageing_timer_value = jiffies_to_clock_t(jiffies - f->updated);
+ fe->fdb_vid = f->vlan_id;
++fe;
++num;
}
@@ -325,26 +331,30 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
}
static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
- const unsigned char *addr)
+ const unsigned char *addr,
+ __u16 vlan_tci)
{
struct hlist_node *h;
struct net_bridge_fdb_entry *fdb;
hlist_for_each_entry(fdb, h, head, hlist) {
- if (ether_addr_equal(fdb->addr.addr, addr))
+ if (ether_addr_equal(fdb->addr.addr, addr) &&
+ fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
return fdb;
}
return NULL;
}
static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
- const unsigned char *addr)
+ const unsigned char *addr,
+ __u16 vlan_tci)
{
struct hlist_node *h;
struct net_bridge_fdb_entry *fdb;
hlist_for_each_entry_rcu(fdb, h, head, hlist) {
- if (ether_addr_equal(fdb->addr.addr, addr))
+ if (ether_addr_equal(fdb->addr.addr, addr) &&
+ fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
return fdb;
}
return NULL;
@@ -352,7 +362,8 @@ static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
struct net_bridge_port *source,
- const unsigned char *addr)
+ const unsigned char *addr,
+ __u16 vlan_tci)
{
struct net_bridge_fdb_entry *fdb;
@@ -360,6 +371,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
if (fdb) {
memcpy(fdb->addr.addr, addr, ETH_ALEN);
fdb->dst = source;
+ fdb->vlan_id = (vlan_tci & VLAN_VID_MASK);
fdb->is_local = 0;
fdb->is_static = 0;
fdb->updated = fdb->used = jiffies;
@@ -371,13 +383,13 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
const unsigned char *addr)
{
- struct hlist_head *head = &br->hash[br_mac_hash(addr)];
+ struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
struct net_bridge_fdb_entry *fdb;
if (!is_valid_ether_addr(addr))
return -EINVAL;
- fdb = fdb_find(head, addr);
+ fdb = fdb_find(head, addr, 0);
if (fdb) {
/* it is okay to have multiple ports with same
* address, just use the first one.
@@ -390,7 +402,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
fdb_delete(br, fdb);
}
- fdb = fdb_create(head, source, addr);
+ fdb = fdb_create(head, source, addr, 0);
if (!fdb)
return -ENOMEM;
@@ -412,9 +424,9 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
}
void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
- const unsigned char *addr)
+ const unsigned char *addr, __u16 vlan_tci)
{
- struct hlist_head *head = &br->hash[br_mac_hash(addr)];
+ struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
struct net_bridge_fdb_entry *fdb;
/* some users want to always flood. */
@@ -426,7 +438,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
source->state == BR_STATE_FORWARDING))
return;
- fdb = fdb_find_rcu(head, addr);
+ fdb = fdb_find_rcu(head, addr, vlan_tci);
if (likely(fdb)) {
/* attempt to update an entry for a local interface */
if (unlikely(fdb->is_local)) {
@@ -441,8 +453,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
}
} else {
spin_lock(&br->hash_lock);
- if (likely(!fdb_find(head, addr))) {
- fdb = fdb_create(head, source, addr);
+ if (likely(!fdb_find(head, addr, vlan_tci))) {
+ fdb = fdb_create(head, source, addr, vlan_tci);
if (fdb)
fdb_notify(br, fdb, RTM_NEWNEIGH);
}
@@ -571,18 +583,18 @@ out:
/* Update (create or replace) forwarding database entry */
static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
- __u16 state, __u16 flags)
+ __u16 state, __u16 flags, __u16 vlan_tci)
{
struct net_bridge *br = source->br;
- struct hlist_head *head = &br->hash[br_mac_hash(addr)];
+ struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
struct net_bridge_fdb_entry *fdb;
- fdb = fdb_find(head, addr);
+ fdb = fdb_find(head, addr, vlan_tci);
if (fdb == NULL) {
if (!(flags & NLM_F_CREATE))
return -ENOENT;
- fdb = fdb_create(head, source, addr);
+ fdb = fdb_create(head, source, addr, vlan_tci);
if (!fdb)
return -ENOMEM;
fdb_notify(br, fdb, RTM_NEWNEIGH);
@@ -628,11 +640,12 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
if (ndm->ndm_flags & NTF_USE) {
rcu_read_lock();
- br_fdb_update(p->br, p, addr);
+ br_fdb_update(p->br, p, addr, 0);
rcu_read_unlock();
} else {
spin_lock_bh(&p->br->hash_lock);
- err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags);
+ err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags,
+ 0);
spin_unlock_bh(&p->br->hash_lock);
}
@@ -642,10 +655,10 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
static int fdb_delete_by_addr(struct net_bridge_port *p, u8 *addr)
{
struct net_bridge *br = p->br;
- struct hlist_head *head = &br->hash[br_mac_hash(addr)];
+ struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
struct net_bridge_fdb_entry *fdb;
- fdb = fdb_find(head, addr);
+ fdb = fdb_find(head, addr, 0);
if (!fdb)
return -ENOENT;
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index b7ca3b3..a4579ee 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -62,12 +62,15 @@ int br_handle_frame_finish(struct sk_buff *skb)
* only traffic matching the VLAN filter.
*/
vlan_map = rcu_dereference(p->vlan_map);
- if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))
- goto drop;
+ if (vlan_map && vlan_tx_tag_present(skb)) {
+ unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
+ if (!test_bit(vid+1, vlan_map))
+ goto drop;
+ }
/* insert into forwarding database after filtering to avoid spoofing */
br = p->br;
- br_fdb_update(br, p, eth_hdr(skb)->h_source);
+ br_fdb_update(br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
if (!is_broadcast_ether_addr(dest) && is_multicast_ether_addr(dest) &&
br_multicast_rcv(br, p, skb))
@@ -102,7 +105,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
skb2 = skb;
br->dev->stats.multicast++;
- } else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
+ } else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) &&
+ dst->is_local) {
skb2 = skb;
/* Do not forward the packet since it's local. */
skb = NULL;
@@ -131,7 +135,7 @@ static int br_handle_local_finish(struct sk_buff *skb)
{
struct net_bridge_port *p = br_port_get_rcu(skb->dev);
- br_fdb_update(p->br, p, eth_hdr(skb)->h_source);
+ br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
return 0; /* process further */
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 8da90e8..921b927 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -72,6 +72,7 @@ struct net_bridge_fdb_entry
unsigned long updated;
unsigned long used;
mac_addr addr;
+ __u16 vlan_id;
unsigned char is_local;
unsigned char is_static;
};
@@ -352,7 +353,8 @@ extern void br_fdb_cleanup(unsigned long arg);
extern void br_fdb_delete_by_port(struct net_bridge *br,
const struct net_bridge_port *p, int do_all);
extern struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
- const unsigned char *addr);
+ const unsigned char *addr,
+ __u16 vlan_tci);
extern int br_fdb_test_addr(struct net_device *dev, unsigned char *addr);
extern int br_fdb_fillbuf(struct net_bridge *br, void *buf,
unsigned long count, unsigned long off);
@@ -361,7 +363,8 @@ extern int br_fdb_insert(struct net_bridge *br,
const unsigned char *addr);
extern void br_fdb_update(struct net_bridge *br,
struct net_bridge_port *source,
- const unsigned char *addr);
+ const unsigned char *addr,
+ __u16 vlan_tci);
extern int br_fdb_delete(struct ndmsg *ndm,
struct net_device *dev,
--
1.7.7.6
^ permalink raw reply related
* [RFC PATCH bridge 1/5] bridge: Add vlan check to forwarding path
From: Vlad Yasevich @ 2012-08-23 19:29 UTC (permalink / raw)
To: netdev; +Cc: Vlad Yasevich
In-Reply-To: <1345750195-31598-1-git-send-email-vyasevic@redhat.com>
When forwarding packets make sure vlan matches any configured vlan for
the port.
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
net/bridge/br_forward.c | 17 ++++++++++++++++-
net/bridge/br_input.c | 8 ++++++++
net/bridge/br_private.h | 2 ++
3 files changed, 26 insertions(+), 1 deletions(-)
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index e9466d4..ab81954 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -26,11 +26,26 @@ static int deliver_clone(const struct net_bridge_port *prev,
void (*__packet_hook)(const struct net_bridge_port *p,
struct sk_buff *skb));
-/* Don't forward packets to originating port or forwarding diasabled */
+/* check to see that the vlan is allowed to be forwarded on this interface */
+static inline int vlan_match(const struct net_bridge_port *p,
+ const struct sk_buff *skb)
+{
+ unsigned long *vlan_map = rcu_dereference(p->vlan_map);
+ unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
+
+ /* The map keeps the vlans off by 1 so adjust for that */
+ return (vlan_map && vlan_tx_tag_present(skb) &&
+ test_bit(vid+1, vlan_map));
+}
+
+/* Don't forward packets to originating port or forwarding diasabled.
+ */
static inline int should_deliver(const struct net_bridge_port *p,
const struct sk_buff *skb)
{
+
return (((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
+ vlan_match(p, skb) &&
p->state == BR_STATE_FORWARDING);
}
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 76f15fd..b7ca3b3 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -53,10 +53,18 @@ int br_handle_frame_finish(struct sk_buff *skb)
struct net_bridge_fdb_entry *dst;
struct net_bridge_mdb_entry *mdst;
struct sk_buff *skb2;
+ unsigned long *vlan_map;
if (!p || p->state == BR_STATE_DISABLED)
goto drop;
+ /* If VLAN filter is configured on the port, make sure we accept
+ * only traffic matching the VLAN filter.
+ */
+ vlan_map = rcu_dereference(p->vlan_map);
+ if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))
+ goto drop;
+
/* insert into forwarding database after filtering to avoid spoofing */
br = p->br;
br_fdb_update(br, p, eth_hdr(skb)->h_source);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index a768b24..8da90e8 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -152,6 +152,8 @@ struct net_bridge_port
#ifdef CONFIG_NET_POLL_CONTROLLER
struct netpoll *np;
#endif
+ unsigned long __rcu *vlan_map;
+
};
#define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
--
1.7.7.6
^ permalink raw reply related
* [RFC PATCH bridge 0/5] Add basic VLAN support to bridges
From: Vlad Yasevich @ 2012-08-23 19:29 UTC (permalink / raw)
To: netdev; +Cc: Vlad Yasevich
This series of patches provides an ability to add VLAN IDs to the bridge
ports. This is similar to what can be found in most switches. The bridge
port may have any number of VLANs added to it including vlan 0 for untagged
traffic. When vlans are added to the port, only traffic tagged with particular
vlan will forwarded over this port. Additionally, vlan ids are added to FDB
entries and become part of the lookup. This way we correctly identify the FDB
entry.
There are still pieces missing. I don't yet support adding a static fdb entry
with a particular vlan. There is no netlink support for carrying a vlan id.
I'd like to hear thoughts of whether this is usufull and something we should
persue.
The default behavior ofthe bridge is unchanged if no vlans have been
configured.
Vlad Yasevich (5):
bridge: Add vlan check to forwarding path
bridge: Add vlan to unicast fdb entries
bridge: Add vlan id to multicast groups
bridge: Add private ioctls to configure vlans on bridge ports
bridge: Add sysfs interface to display VLANS
include/linux/if_bridge.h | 4 ++
net/bridge/br_device.c | 2 +-
net/bridge/br_fdb.c | 71 ++++++++++++++++++-------------
net/bridge/br_forward.c | 17 +++++++-
net/bridge/br_if.c | 103 +++++++++++++++++++++++++++++++++++++++++++++
net/bridge/br_input.c | 18 +++++++-
net/bridge/br_ioctl.c | 31 +++++++++++++
net/bridge/br_multicast.c | 64 +++++++++++++++++++---------
net/bridge/br_private.h | 14 +++++-
net/bridge/br_sysfs_if.c | 28 ++++++++++++
10 files changed, 296 insertions(+), 56 deletions(-)
--
1.7.7.6
^ permalink raw reply
* RE: [net-next 11/13] igb: Update PTP function names/variables and locations.
From: Vick, Matthew @ 2012-08-23 18:58 UTC (permalink / raw)
To: Keller, Jacob E, Richard Cochran
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <02874ECE860811409154E81DA85FBB5807857FFC@ORSMSX105.amr.corp.intel.com>
> -----Original Message-----
> From: Keller, Jacob E
> Sent: Thursday, August 23, 2012 11:48 AM
> To: Vick, Matthew; Richard Cochran
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: RE: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
>
> [...]
>
> The history isn't lost, it is just obscured. I agree if the change is
> necessary it should be done. I think we all disagree on what is
> necessary. I would prefer function names to match. Richard has a point
> regarding the time stamping all packets, however again the ioctl
> turning that on tends to be quite PTP centric already. I think that
> value isn't necessarily added due to the current coupling of the
> features. It is partially coupled by the hardware anyways.
>
> Effectively with the PTP framework disabled you have a half useful
> feature that is enabled by a strange ioctl that has a lot of PTP
> specific names, and doesn't always do what you want.
>
> I am not sure how much work would be required to get to a state where
> the separation of function makes sense.
>
> - Jake
Fair point about the history.
Since these functions are tightly coupled with PTP, it makes sense to call them that for now. Long-term, I completely agree with Richard--once the function is independent of PTP, we should give it a generic name.
Cheers,
Matthew
^ permalink raw reply
* RE: [net-next 11/13] igb: Update PTP function names/variables and locations.
From: Keller, Jacob E @ 2012-08-23 18:48 UTC (permalink / raw)
To: Vick, Matthew, Richard Cochran
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <06DFBC1E25D8024DB214DC7F41A3CD34488DD32D@ORSMSX101.amr.corp.intel.com>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Vick, Matthew
> Sent: Thursday, August 23, 2012 11:35 AM
> To: Richard Cochran
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: RE: [net-next 11/13] igb: Update PTP function names/variables and
> locations.
>
> > -----Original Message-----
> > From: Richard Cochran [mailto:richardcochran@gmail.com]
> > Sent: Thursday, August 23, 2012 10:54 AM
> > To: Vick, Matthew
> > Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> > gospo@redhat.com; sassmann@redhat.com
> > Subject: Re: [net-next 11/13] igb: Update PTP function names/variables
> > and locations.
> >
> > On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
> >
> > > > > #ifdef CONFIG_IGB_PTP
> > > > > -static int igb_ethtool_get_ts_info(struct net_device *dev,
> > > > > +static int igb_get_ts_info(struct net_device *dev,
> > > >
> > > > I like the old name better.
> > >
> > > The old name is out of the coding style of igb. Every other function
> > is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and
> > igb_ethtool_complete. Are you suggesting we add _ethtool to every
> > ethtool function in igb?
> >
> > No, just leave the names alone, and keep the functions where they are.
> > It is just churn.
> >
> > One of the most useful ways to understand code (at least for me) is to
> > use git blame. It tells you when code was added, what the reason was,
> > and how the change looks in context. By moving and renaming willy
> > nilly, you are obscuring this valuable information.
>
> Repairing is not churn. I think it's better to make the code clearer than
> require developers use git blame to figure out who named a function in a
> silly way. I agree, it kind of stinks to lose that history, but I'd rather
> not skip making the right change because of git blame concerns.
The history isn't lost, it is just obscured. I agree if the change is necessary it should be done. I think we all disagree on what is necessary. I would prefer function names to match. Richard has a point regarding the time stamping all packets, however again the ioctl turning that on tends to be quite PTP centric already. I think that value isn't necessarily added due to the current coupling of the features. It is partially coupled by the hardware anyways.
Effectively with the PTP framework disabled you have a half useful feature that is enabled by a strange ioctl that has a lot of PTP specific names, and doesn't always do what you want.
I am not sure how much work would be required to get to a state where the separation of function makes sense.
- Jake
>
> >
> > [...]
> >
> > >
> > > Which, this function calls igb_systim_to_hwtstamp anyway, which is
> > > global.
> >
> > So how does calling two global functions in series improve performance?
>
> It isn't calling two global functions. It's calling a global function that
> calls a static function.
>
> > > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> > > even be called in clean_tx_irq, FWIW.
> >
> > If this is part of some larger plan, then it would help to see that
> > plan.
>
> Definitely fair enough. I will work with Jeff to try and push the whole
> series next time (at the very least, I'd like to re-spin the series for
> the ethtool query patch).
>
> > > > > /**
> > > > > * igb_clean_tx_irq - Reclaim resources after transmit completes
> > > > > * @q_vector: pointer to q_vector containing needed info @@
> > > > > -5827,7
> > > > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector
> > > > *q_vector)
> > > > >
> > > > > #ifdef CONFIG_IGB_PTP
> > > > > /* retrieve hardware timestamp */
> > > > > - igb_tx_hwtstamp(q_vector, tx_buffer);
> > > > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer);
> > > >
> > > > This name stinks, too. You know that you can have time stamping
> > > > all by itself, right? It is logically separate from the ptp clock
> > stuff.
> > > >
> > > > This patch doesn't really improve the driver at all, IMHO.
> > > >
> > > > Thanks,
> > > > Richard
> > >
> > > Yes, I'm aware. But, as it stands today, we don't use it for
> > > anything
> > else. If the function is feature specific, then we should be calling
> > it out as such.
> >
> > Right now the time stamping is being equated with the clock functions,
> > but it really should be decoupled. The 82580 can time stamp every
> > received packet, which can be interesting for performance monitoring,
> > even without PTP (and adding *that* would be a useful change).
>
> As Jake said, there's no support for hardware timestamping without PTP
> today. As such, if the function is only useful for that feature, then I
> think it's less ambiguous to leave it the way I've coded it today. If we
> de-couple hardware timestamping and clock synchronization, which I
> wouldn't be opposed to, then the function name can reflect becoming more
> generic. As it stands, I'd rather not call something generic when it
> isn't. I think it's misleading, personally.
>
> > > I'm sorry you feel like this patch doesn't improve the driver. The
> > goal is code cleanup and consistency, both of which I consider to be
> > driver improvements and is why I made the patches.
> >
> > But the code wasn't dirty in the first place. It doesn't need this
> > "cleaning." This series undoes the inline-able functions for no good
> > reason. As far as ixgbe goes, this driver came first, so you might as
> > well be making *that* driver consistent with this one.
> >
> > Thanks,
> > Richard
>
> Actually, yes, the code *was* dirty, because it breaks the coding style of
> the rest of the driver and isn't as self-documenting as it could be, and
> the inline-able functions you claim I'm destroying called global functions
> anyway. Also, just because igb came first doesn't mean we should ignore
> porting better or clearer solutions from later drivers. If something is
> better or clearer, I think we should use be using it.
>
> Cheers,
> Matthew
> --
> 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
^ permalink raw reply
* RE: [net-next 11/13] igb: Update PTP function names/variables and locations.
From: Vick, Matthew @ 2012-08-23 18:44 UTC (permalink / raw)
To: Richard Cochran, Keller, Jacob E
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <20120823181117.GD2192@netboy.at.omicron.at>
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, August 23, 2012 11:11 AM
> To: Keller, Jacob E
> Cc: Vick, Matthew; Kirsher, Jeffrey T; davem@davemloft.net;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
>
> [...]
>
> Relative, high resolution time stamps can be interesting all by
> themselves. That is why wireshark has a whole menu of timing choices
> including relative since start, inter-packet, and so on.
>
> [...]
>
> Yes, I agree that igb is a bit oddly synced WRT clock and time
> stamping. I would welcome a change to let it have HW time stamping as
> an independent feature.
>
> Thanks,
> Richard
Isn't this discussion creeping outside the scope of this patch? My aim here with this patch is to help tidy up PTP code as it is today. As it is today, igb has no support for hardware timestamping without PTP. If you or someone else writes a follow-on patch to add hardware timestamping without PTP, then they can move and rename the functions accordingly (and I'd personally really appreciate it if they did rename it, since a generic function should be named like a generic function and a PTP function should be named like a PTP function).
Cheers,
Matthew
^ permalink raw reply
* RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
From: Vick, Matthew @ 2012-08-23 18:40 UTC (permalink / raw)
To: Keller, Jacob E, Richard Cochran
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <02874ECE860811409154E81DA85FBB5807857EFE@ORSMSX105.amr.corp.intel.com>
> -----Original Message-----
> From: Keller, Jacob E
> Sent: Thursday, August 23, 2012 11:04 AM
> To: Richard Cochran; Vick, Matthew
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
>
> [...]
>
> Personally disagree here. I do agree that the churn is annoying with
> how it breaks git blame, however, in general I prefer tags at the end
> of #ifdefs even for short ones because it increases my ability to
> quickly spot matches. The end comment aligns with the starting comment,
> and even for small blocks makes it easier to process. It is less
> necessary the smaller the block, but I always prefer to have it than
> not.
>
> That said, it is nice when git blame points to the right place, and the
> comments aren't super necessary for such short blocks.
>
> - Jake
I tend to agree with Jake here--I like having the information. I'm fine removing them, but I'd like to do it for all CONFIG_IGB_PTP wrapping if we're going to do it. What do you think, Richard?
Cheers,
Matthew
^ permalink raw reply
* RE: [net-next 11/13] igb: Update PTP function names/variables and locations.
From: Vick, Matthew @ 2012-08-23 18:35 UTC (permalink / raw)
To: Richard Cochran
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <20120823175335.GB2192@netboy.at.omicron.at>
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, August 23, 2012 10:54 AM
> To: Vick, Matthew
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 11/13] igb: Update PTP function names/variables
> and locations.
>
> On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
>
> > > > #ifdef CONFIG_IGB_PTP
> > > > -static int igb_ethtool_get_ts_info(struct net_device *dev,
> > > > +static int igb_get_ts_info(struct net_device *dev,
> > >
> > > I like the old name better.
> >
> > The old name is out of the coding style of igb. Every other function
> is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and
> igb_ethtool_complete. Are you suggesting we add _ethtool to every
> ethtool function in igb?
>
> No, just leave the names alone, and keep the functions where they are.
> It is just churn.
>
> One of the most useful ways to understand code (at least for me) is to
> use git blame. It tells you when code was added, what the reason was,
> and how the change looks in context. By moving and renaming willy
> nilly, you are obscuring this valuable information.
Repairing is not churn. I think it's better to make the code clearer than require developers use git blame to figure out who named a function in a silly way. I agree, it kind of stinks to lose that history, but I'd rather not skip making the right change because of git blame concerns.
>
> [...]
>
> >
> > Which, this function calls igb_systim_to_hwtstamp anyway, which is
> > global.
>
> So how does calling two global functions in series improve performance?
It isn't calling two global functions. It's calling a global function that calls a static function.
> > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> > even be called in clean_tx_irq, FWIW.
>
> If this is part of some larger plan, then it would help to see that
> plan.
Definitely fair enough. I will work with Jeff to try and push the whole series next time (at the very least, I'd like to re-spin the series for the ethtool query patch).
> > > > /**
> > > > * igb_clean_tx_irq - Reclaim resources after transmit completes
> > > > * @q_vector: pointer to q_vector containing needed info @@
> > > > -5827,7
> > > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector
> > > *q_vector)
> > > >
> > > > #ifdef CONFIG_IGB_PTP
> > > > /* retrieve hardware timestamp */
> > > > - igb_tx_hwtstamp(q_vector, tx_buffer);
> > > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer);
> > >
> > > This name stinks, too. You know that you can have time stamping all
> > > by itself, right? It is logically separate from the ptp clock
> stuff.
> > >
> > > This patch doesn't really improve the driver at all, IMHO.
> > >
> > > Thanks,
> > > Richard
> >
> > Yes, I'm aware. But, as it stands today, we don't use it for anything
> else. If the function is feature specific, then we should be calling it
> out as such.
>
> Right now the time stamping is being equated with the clock functions,
> but it really should be decoupled. The 82580 can time stamp every
> received packet, which can be interesting for performance monitoring,
> even without PTP (and adding *that* would be a useful change).
As Jake said, there's no support for hardware timestamping without PTP today. As such, if the function is only useful for that feature, then I think it's less ambiguous to leave it the way I've coded it today. If we de-couple hardware timestamping and clock synchronization, which I wouldn't be opposed to, then the function name can reflect becoming more generic. As it stands, I'd rather not call something generic when it isn't. I think it's misleading, personally.
> > I'm sorry you feel like this patch doesn't improve the driver. The
> goal is code cleanup and consistency, both of which I consider to be
> driver improvements and is why I made the patches.
>
> But the code wasn't dirty in the first place. It doesn't need this
> "cleaning." This series undoes the inline-able functions for no good
> reason. As far as ixgbe goes, this driver came first, so you might as
> well be making *that* driver consistent with this one.
>
> Thanks,
> Richard
Actually, yes, the code *was* dirty, because it breaks the coding style of the rest of the driver and isn't as self-documenting as it could be, and the inline-able functions you claim I'm destroying called global functions anyway. Also, just because igb came first doesn't mean we should ignore porting better or clearer solutions from later drivers. If something is better or clearer, I think we should use be using it.
Cheers,
Matthew
^ permalink raw reply
* RE: [PATCH] mwifiex: use is_zero_ether_addr() instead of memcmp()
From: Bing Zhao @ 2012-08-23 18:28 UTC (permalink / raw)
To: Wei Yongjun, linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org
Cc: yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org,
linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAPgLHd9iXQf3_W5b4kO1Mv68udqnmfFO8n+R-0ZqzCrhg1Z_WQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Wei,
Thanks for the patch.
> From: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
>
> Using is_zero_ether_addr() instead of directly use
> memcmp() to determine if the ethernet address is all
> zeros.
>
> spatch with a semantic match is used to found this problem.
> (http://coccinelle.lip6.fr/)
Apparently there are two more occurrences of zero_mac (scan.c:843) and bc_mac (sta_cmd.c:596).
But I can fix them when I get the chance.
>
> Signed-off-by: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
Acked-by: Bing Zhao <bzhao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Thanks,
Bing
> ---
> drivers/net/wireless/mwifiex/sta_cmdresp.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/mwifiex/sta_cmdresp.c b/drivers/net/wireless/mwifiex/sta_cmdresp.c
> index 0b09004..a2a33dc 100644
> --- a/drivers/net/wireless/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/mwifiex/sta_cmdresp.c
> @@ -17,6 +17,8 @@
> * this warranty disclaimer.
> */
>
> +#include <linux/etherdevice.h>
> +
> #include "decl.h"
> #include "ioctl.h"
> #include "util.h"
> @@ -736,7 +738,6 @@ static int mwifiex_ret_ibss_coalescing_status(struct mwifiex_private *priv,
> {
> struct host_cmd_ds_802_11_ibss_status *ibss_coal_resp =
> &(resp->params.ibss_coalescing);
> - u8 zero_mac[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
>
> if (le16_to_cpu(ibss_coal_resp->action) == HostCmd_ACT_GEN_SET)
> return 0;
> @@ -745,7 +746,7 @@ static int mwifiex_ret_ibss_coalescing_status(struct mwifiex_private *priv,
> "info: new BSSID %pM\n", ibss_coal_resp->bssid);
>
> /* If rsp has NULL BSSID, Just return..... No Action */
> - if (!memcmp(ibss_coal_resp->bssid, zero_mac, ETH_ALEN)) {
> + if (is_zero_ether_addr(ibss_coal_resp->bssid)) {
> dev_warn(priv->adapter->dev, "new BSSID is NULL\n");
> return 0;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next v3] net: remove delay at device dismantle
From: Ben Hutchings @ 2012-08-23 18:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, gaofeng, netdev, maheshb, therbert, ebiederm
In-Reply-To: <1345691986.5904.40.camel@edumazet-glaptop>
On Thu, 2012-08-23 at 05:19 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> I noticed extra one second delay in device dismantle, tracked down to
> a call to dst_dev_event() while some call_rcu() are still in RCU queues.
>
> These call_rcu() were posted by rt_free(struct rtable *rt) calls.
>
> We then wait a little (but one second) in netdev_wait_allrefs() before
> kicking again NETDEV_UNREGISTER.
>
> As the call_rcu() are now completed, dst_dev_event() can do the needed
> device swap on busy dst.
>
> To solve this problem, add a new NETDEV_UNREGISTER_FINAL, called
> after a rcu_barrier(), but outside of RTNL lock.
[...]
So what happens when this races with register_netdevice_notifier()?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
From: Love, Robert W @ 2012-08-23 18:15 UTC (permalink / raw)
To: Naresh Kumar Inna
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
dm@chelsio.com, netdev@vger.kernel.org, chethan@chelsio.com
In-Reply-To: <1345760873-12101-7-git-send-email-naresh@chelsio.com>
On 8/23/2012 3:27 PM, Naresh Kumar Inna wrote:
> This patch contains the first set of the header files for csiostor driver.
>
> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
> ---
> drivers/scsi/csiostor/csio_defs.h | 143 ++++++
> drivers/scsi/csiostor/csio_fcoe_proto.h | 843 +++++++++++++++++++++++++++++++
I think most of these protocol definitions can be found in
include/scsi/fc/ and possibly incluse/scsi/scsi_transport_fc.h. I think
any protocol stuff missing should be added to the common header files.
Thanks, //Rob
^ permalink raw reply
* Re: [net-next 11/13] igb: Update PTP function names/variables and locations.
From: Richard Cochran @ 2012-08-23 18:11 UTC (permalink / raw)
To: Keller, Jacob E
Cc: Vick, Matthew, Kirsher, Jeffrey T, davem@davemloft.net,
netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <02874ECE860811409154E81DA85FBB5807857EEA@ORSMSX105.amr.corp.intel.com>
On Thu, Aug 23, 2012 at 06:00:37PM +0000, Keller, Jacob E wrote:
> >
> > Right now the time stamping is being equated with the clock functions, but
> > it really should be decoupled. The 82580 can time stamp every received
> > packet, which can be interesting for performance monitoring, even without
> > PTP (and adding *that* would be a useful change).
> >
> The timestamp all does not really work with the ptp clock features
> gone, because you don't have the clock. You can't equate the time
> values of the packets when the clock isn't synched to something
> meaningful. Yes that does not require PTP adjustment functions, but
> it does require the SYSTIME setup and some method to get the clock
> correct, which currently is only done in the PTP init sequence.
Relative, high resolution time stamps can be interesting all by
themselves. That is why wireshark has a whole menu of timing choices
including relative since start, inter-packet, and so on.
> Timestamp all packets also can cause a performance hit when used with certain workloads.
Only when enabled.
> ixgbe hardware is (currently) even more closely synched with PTP for the register bits so it does make some sense for ixgbe to remain the way it is. Right now the igb features are partially synched (even before this change) in odd ways. The time values returned when PHC information is disabled are basically only useful for comparing between themselves, not with any meaningful clock on the device.
Yes, I agree that igb is a bit oddly synced WRT clock and time
stamping. I would welcome a change to let it have HW time stamping as
an independent feature.
Thanks,
Richard
^ permalink raw reply
* RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
From: Keller, Jacob E @ 2012-08-23 18:03 UTC (permalink / raw)
To: Richard Cochran, Vick, Matthew
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <20120823172950.GA2192@netboy.at.omicron.at>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Richard Cochran
> Sent: Thursday, August 23, 2012 10:30 AM
> To: Vick, Matthew
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
>
> On Thu, Aug 23, 2012 at 04:09:30PM +0000, Vick, Matthew wrote:
> > > -----Original Message-----
> > > From: Richard Cochran [mailto:richardcochran@gmail.com]
> > > Sent: Thursday, August 23, 2012 4:04 AM
> > > To: Kirsher, Jeffrey T
> > > Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
> > > gospo@redhat.com; sassmann@redhat.com
> > > Subject: Re: [net-next 10/13] igb: Tidy up wrapping for
> CONFIG_IGB_PTP.
> > >
> > > On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:
> > > > From: Matthew Vick <matthew.vick@intel.com>
> > > >
> > > > For users without CONFIG_IGB_PTP=y, we should not be compiling any
> > > PTP
> > > > code into the driver. Tidy up the wrapping in igb to support this.
> > >
> > > Actually, you are doing more than that. You are adding a bunch of
> > > comments onto the already existing #endifs.
> >
> > Fair enough. Would you like me to update the patch description?
>
> Better would be to drop of the pendantic #endif /*CONFIG_FOO*/ stuff.
> It is just churn.
>
Personally disagree here. I do agree that the churn is annoying with how it breaks git blame, however, in general I prefer tags at the end of #ifdefs even for short ones because it increases my ability to quickly spot matches. The end comment aligns with the starting comment, and even for small blocks makes it easier to process. It is less necessary the smaller the block, but I always prefer to have it than not.
That said, it is nice when git blame points to the right place, and the comments aren't super necessary for such short blocks.
- Jake
> Thanks,
> Richard
> --
> 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
^ permalink raw reply
* Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
From: Richard Cochran @ 2012-08-23 18:03 UTC (permalink / raw)
To: Vick, Matthew
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <06DFBC1E25D8024DB214DC7F41A3CD34488DD257@ORSMSX101.amr.corp.intel.com>
On Thu, Aug 23, 2012 at 05:40:01PM +0000, Vick, Matthew wrote:
> > Better would be to drop of the pendantic #endif /*CONFIG_FOO*/ stuff.
> > It is just churn.
> >
> > Thanks,
> > Richard
>
> I'm willing to drop it, but I would like to drop it universally in the driver if that's the case. Is that acceptable? There's no overly long or complex wrapping section in the driver that I think merits being sloppy with the end comments.
Well, I don't know about the other cases.
drivers/net/ethernet/intel/igb$ grep \#endif *.c | grep -v CONFIG | wc -l
28
drivers/net/ethernet/intel/igb$ grep \#endif *.c | grep CONFIG | wc -l
8
(none of these are for _IGB_PTP)
Having #endif comments is useful when it makes the code more clear. It
is a matter of taste, but I do think having such comments for just two
lines in between is ugly and silly.
Thanks,
Richard
^ permalink raw reply
* RE: [net-next 11/13] igb: Update PTP function names/variables and locations.
From: Keller, Jacob E @ 2012-08-23 18:00 UTC (permalink / raw)
To: Richard Cochran, Vick, Matthew
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <20120823175335.GB2192@netboy.at.omicron.at>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Richard Cochran
> Sent: Thursday, August 23, 2012 10:54 AM
> To: Vick, Matthew
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 11/13] igb: Update PTP function names/variables and
> locations.
>
> On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
>
> > > > #ifdef CONFIG_IGB_PTP
> > > > -static int igb_ethtool_get_ts_info(struct net_device *dev,
> > > > +static int igb_get_ts_info(struct net_device *dev,
> > >
> > > I like the old name better.
> >
> > The old name is out of the coding style of igb. Every other function is
> igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and
> igb_ethtool_complete. Are you suggesting we add _ethtool to every ethtool
> function in igb?
>
> No, just leave the names alone, and keep the functions where they are. It
> is just churn.
>
> One of the most useful ways to understand code (at least for me) is to use
> git blame. It tells you when code was added, what the reason was, and how
> the change looks in context. By moving and renaming willy nilly, you are
> obscuring this valuable information.
>
> > > > -#ifdef CONFIG_IGB_PTP
> > > > -/**
> > > > - * igb_tx_hwtstamp - utility function which checks for TX time
> > > > stamp
> > > > - * @q_vector: pointer to q_vector containing needed info
> > > > - * @buffer: pointer to igb_tx_buffer structure
> > > > - *
> > > > - * If we were asked to do hardware stamping and such a time stamp
> > > > is
> > > > - * available, then it must have been for this skb here because we
> > > > only
> > > > - * allow only one such packet into the queue.
> > > > - */
> > > > -static void igb_tx_hwtstamp(struct igb_q_vector *q_vector,
> > > > - struct igb_tx_buffer *buffer_info)
> > > > -{
> > > > - struct igb_adapter *adapter = q_vector->adapter;
> > > > - struct e1000_hw *hw = &adapter->hw;
> > > > - struct skb_shared_hwtstamps shhwtstamps;
> > > > - u64 regval;
> > > > -
> > > > - /* if skb does not support hw timestamp or TX stamp not valid
> > > exit */
> > > > - if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) ||
> > > > - !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID))
> > > > - return;
> > > > -
> > > > - regval = rd32(E1000_TXSTMPL);
> > > > - regval |= (u64)rd32(E1000_TXSTMPH) << 32;
> > > > -
> > > > - igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
> > > > - skb_tstamp_tx(buffer_info->skb, &shhwtstamps);
> > > > -}
> > > > -#endif /* CONFIG_IGB_PTP */
> > > > -
> > >
> > > Here you have taken a static local function and made it into a
> > > global function. This can have performance impacts.
> >
> > Which, this function calls igb_systim_to_hwtstamp anyway, which is
> > global.
>
> So how does calling two global functions in series improve performance?
>
> > Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> > even be called in clean_tx_irq, FWIW.
>
> If this is part of some larger plan, then it would help to see that plan.
>
> > > > /**
> > > > * igb_clean_tx_irq - Reclaim resources after transmit completes
> > > > * @q_vector: pointer to q_vector containing needed info @@
> > > > -5827,7
> > > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector
> > > *q_vector)
> > > >
> > > > #ifdef CONFIG_IGB_PTP
> > > > /* retrieve hardware timestamp */
> > > > - igb_tx_hwtstamp(q_vector, tx_buffer);
> > > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer);
> > >
> > > This name stinks, too. You know that you can have time stamping all
> > > by itself, right? It is logically separate from the ptp clock stuff.
> > >
> > > This patch doesn't really improve the driver at all, IMHO.
> > >
> > > Thanks,
> > > Richard
> >
> > Yes, I'm aware. But, as it stands today, we don't use it for anything
> else. If the function is feature specific, then we should be calling it
> out as such.
>
> Right now the time stamping is being equated with the clock functions, but
> it really should be decoupled. The 82580 can time stamp every received
> packet, which can be interesting for performance monitoring, even without
> PTP (and adding *that* would be a useful change).
>
The timestamp all does not really work with the ptp clock features gone, because you don't have the clock. You can't equate the time values of the packets when the clock isn't synched to something meaningful. Yes that does not require PTP adjustment functions, but it does require the SYSTIME setup and some method to get the clock correct, which currently is only done in the PTP init sequence. Timestamp all packets also can cause a performance hit when used with certain workloads.
> > I'm sorry you feel like this patch doesn't improve the driver. The goal
> is code cleanup and consistency, both of which I consider to be driver
> improvements and is why I made the patches.
>
> But the code wasn't dirty in the first place. It doesn't need this
> "cleaning." This series undoes the inline-able functions for no good
> reason. As far as ixgbe goes, this driver came first, so you might as well
> be making *that* driver consistent with this one.
ixgbe hardware is (currently) even more closely synched with PTP for the register bits so it does make some sense for ixgbe to remain the way it is. Right now the igb features are partially synched (even before this change) in odd ways. The time values returned when PHC information is disabled are basically only useful for comparing between themselves, not with any meaningful clock on the device.
>
> Thanks,
> Richard
>
> --
> 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
^ permalink raw reply
* Re: Best way to set kernel thread affinity for handling a socket?
From: Chris Friesen @ 2012-08-23 17:56 UTC (permalink / raw)
To: Roland Dreier; +Cc: Ben Hutchings, netdev
In-Reply-To: <CAG4TOxOdTiahqVo6+cD=mCN5Ov_hLa_m5gBTuj5sUYsM+GgnYw@mail.gmail.com>
On 08/23/2012 11:04 AM, Roland Dreier wrote:
> On Wed, Aug 22, 2012 at 9:21 AM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
>> With RFS we try to do the reverse: move the packets to match the socket
>> user. But it's not (yet) turned on by default. See
>> Documentation/networking/scaling.txt
>
> Fair enough. However I think at least in this case it sounds like extra
> overhead: it should be easy for us to do everything on the CPU where
> the packets are being received.
In the general case though RFS can be useful. We have a server with one
application instance per core. It's beneficial to be able to steer
packets destined for a given instance to an eth driver queue whose
interrupt is handled by the cpu running that instance.
Depending on the hardware being used, it may be possible to do this more
efficiently. The ixgbe driver (if the feature is enabled) will
periodically look at the outgoing tcp traffic and set up hardware flow
filtering rules to direct incoming traffic for that connection to the
queue that sent out the messages.
Chris
^ permalink raw reply
* Re: [net-next 11/13] igb: Update PTP function names/variables and locations.
From: Richard Cochran @ 2012-08-23 17:53 UTC (permalink / raw)
To: Vick, Matthew
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <06DFBC1E25D8024DB214DC7F41A3CD34488DD079@ORSMSX101.amr.corp.intel.com>
On Thu, Aug 23, 2012 at 04:22:02PM +0000, Vick, Matthew wrote:
> > > #ifdef CONFIG_IGB_PTP
> > > -static int igb_ethtool_get_ts_info(struct net_device *dev,
> > > +static int igb_get_ts_info(struct net_device *dev,
> >
> > I like the old name better.
>
> The old name is out of the coding style of igb. Every other function is igb_get_* or igb_set_*, with the exception of igb_ethtool_begin and igb_ethtool_complete. Are you suggesting we add _ethtool to every ethtool function in igb?
No, just leave the names alone, and keep the functions where they
are. It is just churn.
One of the most useful ways to understand code (at least for me) is to
use git blame. It tells you when code was added, what the reason was,
and how the change looks in context. By moving and renaming willy
nilly, you are obscuring this valuable information.
> > > -#ifdef CONFIG_IGB_PTP
> > > -/**
> > > - * igb_tx_hwtstamp - utility function which checks for TX time stamp
> > > - * @q_vector: pointer to q_vector containing needed info
> > > - * @buffer: pointer to igb_tx_buffer structure
> > > - *
> > > - * If we were asked to do hardware stamping and such a time stamp is
> > > - * available, then it must have been for this skb here because we
> > > only
> > > - * allow only one such packet into the queue.
> > > - */
> > > -static void igb_tx_hwtstamp(struct igb_q_vector *q_vector,
> > > - struct igb_tx_buffer *buffer_info)
> > > -{
> > > - struct igb_adapter *adapter = q_vector->adapter;
> > > - struct e1000_hw *hw = &adapter->hw;
> > > - struct skb_shared_hwtstamps shhwtstamps;
> > > - u64 regval;
> > > -
> > > - /* if skb does not support hw timestamp or TX stamp not valid
> > exit */
> > > - if (likely(!(buffer_info->tx_flags & IGB_TX_FLAGS_TSTAMP)) ||
> > > - !(rd32(E1000_TSYNCTXCTL) & E1000_TSYNCTXCTL_VALID))
> > > - return;
> > > -
> > > - regval = rd32(E1000_TXSTMPL);
> > > - regval |= (u64)rd32(E1000_TXSTMPH) << 32;
> > > -
> > > - igb_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
> > > - skb_tstamp_tx(buffer_info->skb, &shhwtstamps);
> > > -}
> > > -#endif /* CONFIG_IGB_PTP */
> > > -
> >
> > Here you have taken a static local function and made it into a global
> > function. This can have performance impacts.
>
> Which, this function calls igb_systim_to_hwtstamp anyway, which is
> global.
So how does calling two global functions in series improve performance?
> Also, in a follow-on patch I have coming, igb_ptp_tx_hwtstamp won't
> even be called in clean_tx_irq, FWIW.
If this is part of some larger plan, then it would help to see that
plan.
> > > /**
> > > * igb_clean_tx_irq - Reclaim resources after transmit completes
> > > * @q_vector: pointer to q_vector containing needed info @@ -5827,7
> > > +5796,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector
> > *q_vector)
> > >
> > > #ifdef CONFIG_IGB_PTP
> > > /* retrieve hardware timestamp */
> > > - igb_tx_hwtstamp(q_vector, tx_buffer);
> > > + igb_ptp_tx_hwtstamp(q_vector, tx_buffer);
> >
> > This name stinks, too. You know that you can have time stamping all by
> > itself, right? It is logically separate from the ptp clock stuff.
> >
> > This patch doesn't really improve the driver at all, IMHO.
> >
> > Thanks,
> > Richard
>
> Yes, I'm aware. But, as it stands today, we don't use it for anything else. If the function is feature specific, then we should be calling it out as such.
Right now the time stamping is being equated with the clock functions,
but it really should be decoupled. The 82580 can time stamp every
received packet, which can be interesting for performance monitoring,
even without PTP (and adding *that* would be a useful change).
> I'm sorry you feel like this patch doesn't improve the driver. The goal is code cleanup and consistency, both of which I consider to be driver improvements and is why I made the patches.
But the code wasn't dirty in the first place. It doesn't need this
"cleaning." This series undoes the inline-able functions for no good
reason. As far as ixgbe goes, this driver came first, so you might as
well be making *that* driver consistent with this one.
Thanks,
Richard
^ permalink raw reply
* Re: Best way to set kernel thread affinity for handling a socket?
From: Ben Hutchings @ 2012-08-23 17:51 UTC (permalink / raw)
To: Roland Dreier; +Cc: netdev
In-Reply-To: <CAG4TOxOdTiahqVo6+cD=mCN5Ov_hLa_m5gBTuj5sUYsM+GgnYw@mail.gmail.com>
On Thu, 2012-08-23 at 10:04 -0700, Roland Dreier wrote:
> On Wed, Aug 22, 2012 at 9:21 AM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
> > With RFS we try to do the reverse: move the packets to match the socket
> > user. But it's not (yet) turned on by default. See
> > Documentation/networking/scaling.txt
>
> Fair enough. However I think at least in this case it sounds like extra
> overhead: it should be easy for us to do everything on the CPU where
> the packets are being received.
>
> >> I'm thinking about this in the context of the kernel's iSCSI target
> >> code (drivers/target/iscsi), which creates threads to handle each
> >> iSCSI connection and sets their CPU affinity pretty much randomly
> >> (well, based on some "thread id", cf iscsit_thread_get_cpumask()).
> >
> > Why set the affinity at all?
>
> It's quite possible that, like a lot of other drivers/target code, this
> doesn't actually make any sense and the right answer is to rip it
> out completely.
>
> Is the affinity due to waking up from a network receive event
> enough to keep the threads local to the right CPU?
I expect it to have some influence on where the receiving task is
scheduled, but it doesn't necessarily outweigh other factors that the
scheduler takes into account. So you would really have to test this
yourself.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
From: Vick, Matthew @ 2012-08-23 17:40 UTC (permalink / raw)
To: Richard Cochran
Cc: Kirsher, Jeffrey T, davem@davemloft.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <20120823172950.GA2192@netboy.at.omicron.at>
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, August 23, 2012 10:30 AM
> To: Vick, Matthew
> Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
>
> On Thu, Aug 23, 2012 at 04:09:30PM +0000, Vick, Matthew wrote:
> > > -----Original Message-----
> > > From: Richard Cochran [mailto:richardcochran@gmail.com]
> > > Sent: Thursday, August 23, 2012 4:04 AM
> > > To: Kirsher, Jeffrey T
> > > Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
> > > gospo@redhat.com; sassmann@redhat.com
> > > Subject: Re: [net-next 10/13] igb: Tidy up wrapping for
> CONFIG_IGB_PTP.
> > >
> > > On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:
> > > > From: Matthew Vick <matthew.vick@intel.com>
> > > >
> > > > For users without CONFIG_IGB_PTP=y, we should not be compiling
> any
> > > PTP
> > > > code into the driver. Tidy up the wrapping in igb to support
> this.
> > >
> > > Actually, you are doing more than that. You are adding a bunch of
> > > comments onto the already existing #endifs.
> >
> > Fair enough. Would you like me to update the patch description?
>
> Better would be to drop of the pendantic #endif /*CONFIG_FOO*/ stuff.
> It is just churn.
>
> Thanks,
> Richard
I'm willing to drop it, but I would like to drop it universally in the driver if that's the case. Is that acceptable? There's no overly long or complex wrapping section in the driver that I think merits being sloppy with the end comments.
Cheers,
Matthew
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox