On 7/8/15 3:27 AM, Nicolas Dichtel wrote:
+
+struct pcpu_dstats {
+ u64 tx_pkts;
+ u64 tx_bytes;
+ u64 tx_drps;
+ u64 rx_pkts;
+ u64 rx_bytes;
+ struct u64_stats_sync syncp;
+};
Why not using 'struct pcpu_sw_netstats' (dev->tstats), like most virtual
interfaces? Not sure that it's really needed to have tx_drps per cpu (and
I don't see anyone using it into this patch).
Alex asked the same question on the first RFC. Shrijeet had opinions on
why this version versus netdev's. Shrijeet?
-----8<-----
+/* queue->lock must be held */
+static void __vrf_insert_slave(struct slave_queue *queue, struct
slave *slave,
+ struct net_device *master)
+{
+ struct slave *duplicate_slave = NULL;
+
+ duplicate_slave = __vrf_find_slave_dev(queue, slave->dev);
+ if (duplicate_slave)
+ __vrf_kill_slave(queue, duplicate_slave);
I miss the point here. Why removing the slave if it is already there?
not sure. I'll investigate.
+
+ dev_hold(slave->dev);
+ list_add(&slave->list, &queue->all_slaves);
+ queue->num_slaves++;
+}
+
+/* netlink lock is assumed here */
ASSERT_RTNL()?
done.
+static int vrf_add_slave(struct net_device *dev,
+ struct net_device *port_dev)
+{
+ if (!dev || !port_dev || dev_net(dev) != dev_net(port_dev))
+ return -ENODEV;
+
+ if (!vrf_is_master(port_dev) && !vrf_is_slave(port_dev)) {
+ struct slave *s = kzalloc(sizeof(*s), GFP_KERNEL);
+ struct net_vrf *vrf = netdev_priv(dev);
+ struct slave_queue *queue = &vrf->queue;
+ bool is_running = netif_running(port_dev);
+ unsigned int flags = port_dev->flags;
+ int ret;
+
+ if (!s)
+ return -ENOMEM;
+
+ s->dev = port_dev;
+
+ spin_lock_bh(&queue->lock);
+ __vrf_insert_slave(queue, s, dev);
+ spin_unlock_bh(&queue->lock);
+
+ port_dev->vrf_ptr = kmalloc(sizeof(*port_dev->vrf_ptr),
+ GFP_KERNEL);
+ if (!port_dev->vrf_ptr)
+ return -ENOMEM;
+
+ port_dev->vrf_ptr->ifindex = dev->ifindex;
+ port_dev->vrf_ptr->tb_id = vrf->tb_id;
+
+ /* register the packet handler for slave ports */
+ ret = netdev_rx_handler_register(port_dev, vrf_handle_frame,
+ (void *)dev);
So, it won't be possible to add a slave which already has a
macvlan or ipvlan (or team?) interface registered.
Shrijeet, thoughts?
-----8<-----
+
+static struct rtnl_link_ops vrf_link_ops __read_mostly = {
+ .kind = DRV_NAME,
+ .priv_size = sizeof(struct net_vrf),
nit: tabs ^^^^^^
+
+ .get_size = vrf_nl_getsize,
nit: tabs ^^^^^^^
+ .policy = vrf_nl_policy,
nit: tabs ^^^^^^^^^
+ .validate = vrf_validate,
+ .fill_info = vrf_fillinfo,
nit: tabs ^^^^^^
+
+ .newlink = vrf_newlink,
nit: tabs ^^^^^^^^
+ .dellink = vrf_dellink,
nit: tabs ^^^^^^^^
+ .setup = vrf_setup,
+ .maxtype = IFLA_VRF_MAX,
nit: tabs ^^^^^^^^
+};
ACK on all tab comments; fixed. Ditto for bool on is_tx_frame.
-----8<-----
+#if IS_ENABLED(CONFIG_NET_VRF)
+static inline int vrf_master_dev_idx(const struct net_device *dev)
+{
+ int idx = 0;
+
+ if (dev && dev->vrf_ptr)
+ idx = dev->vrf_ptr->ifindex;
+
+ return idx;
+}
+
+static inline int vrf_get_master_dev_idx(struct net *net, int idx)
ifindex instead idx for the whole file?
done.
Thanks for the review.
David
PS. comments addressed while consuming a croque-monsieur (my daughter
just returned from a European trip; loves the sandwich)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
