From: David Ahern <dsa@cumulusnetworks•com>
To: nicolas.dichtel@6wind•com, netdev@vger•kernel.org
Cc: shm@cumulusnetworks•com, roopa@cumulusnetworks•com,
gospo@cumulusnetworks•com, jtoppins@cumulusnetworks•com,
nikolay@cumulusnetworks•com, ddutt@cumulusnetworks•com,
hannes@stressinduktion•org, stephen@networkplumber•org,
hadi@mojatatu•com, ebiederm@xmission•com, davem@davemloft•net
Subject: Re: [RFC net-next 3/6] net: Introduce VRF device driver - v2
Date: Wed, 08 Jul 2015 10:38:13 -0600 [thread overview]
Message-ID: <559D51F5.4000602@cumulusnetworks.com> (raw)
In-Reply-To: <559CED1B.7090008@6wind.com>
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)
next prev parent reply other threads:[~2015-07-08 16:38 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-06 15:03 [RFC net-next 0/6] Proposal for VRF-lite - v2 David Ahern
2015-07-06 15:03 ` [RFC net-next 1/6] fib: export symbols David Ahern
2015-07-06 15:03 ` [RFC net-next 2/6] net: Preparation for vrf device David Ahern
2015-07-08 8:37 ` Nicolas Dichtel
2015-07-08 8:40 ` Nicolas Dichtel
2015-07-08 16:10 ` David Ahern
2015-07-06 15:03 ` [RFC net-next 3/6] net: Introduce VRF device driver - v2 David Ahern
2015-07-06 15:42 ` Nicolas Dichtel
2015-07-06 16:37 ` Nikolay Aleksandrov
2015-07-06 16:46 ` David Ahern
2015-07-08 9:27 ` Nicolas Dichtel
2015-07-08 16:38 ` David Ahern [this message]
2015-07-08 18:34 ` Sowmini Varadhan
2015-07-09 17:19 ` David Ahern
2015-07-09 17:28 ` Sowmini Varadhan
2015-07-10 1:36 ` Eric W. Biederman
2015-07-10 2:12 ` David Ahern
2015-07-10 3:55 ` Eric W. Biederman
2015-07-10 4:20 ` David Ahern
2015-07-10 4:56 ` Eric W. Biederman
2015-07-10 18:42 ` David Ahern
2015-07-10 2:39 ` David Ahern
2015-07-10 3:28 ` Sowmini Varadhan
2015-07-10 3:44 ` David Ahern
2015-07-06 15:03 ` [RFC net-next 4/6] net: Modifications to ipv4 stack for VRF devices David Ahern
2015-07-06 15:03 ` [RFC net-next 5/6] net: Add sk_bind_dev_if to task_struct David Ahern
2015-07-06 15:03 ` [RFC net-next 6/6] net: Add chvrf command David Ahern
2015-07-06 15:03 ` [RFC PATCH] iproute2: Add support for VRF device David Ahern
2015-07-06 15:40 ` [RFC net-next 0/6] Proposal for VRF-lite - v2 Nicolas Dichtel
2015-07-06 17:53 ` Shrijeet Mukherjee
2015-07-08 9:30 ` Nicolas Dichtel
2015-07-10 5:14 ` Scott Feldman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=559D51F5.4000602@cumulusnetworks.com \
--to=dsa@cumulusnetworks$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=ddutt@cumulusnetworks$(echo .)com \
--cc=ebiederm@xmission$(echo .)com \
--cc=gospo@cumulusnetworks$(echo .)com \
--cc=hadi@mojatatu$(echo .)com \
--cc=hannes@stressinduktion$(echo .)org \
--cc=jtoppins@cumulusnetworks$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nicolas.dichtel@6wind$(echo .)com \
--cc=nikolay@cumulusnetworks$(echo .)com \
--cc=roopa@cumulusnetworks$(echo .)com \
--cc=shm@cumulusnetworks$(echo .)com \
--cc=stephen@networkplumber$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox