From: Ben Hutchings <bhutchings@solarflare.com>
To: Chris Metcalf <cmetcalf@tilera.com>
Cc: David Miller <davem@davemloft.net>, <arnd@arndb.de>,
<linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>
Subject: Re: [PATCH v5] tilegx network driver: initial support
Date: Fri, 11 May 2012 14:54:05 +0100 [thread overview]
Message-ID: <1336744445.2874.57.camel@bwh-desktop.uk.solarflarecom.com> (raw)
In-Reply-To: <201205091452.q49EqcO7005975@farm-0027.internal.tilera.com>
Here's another very incomplete review for you.
On Wed, 2012-05-09 at 06:42 -0400, Chris Metcalf wrote:
> This change adds support for the tilegx network driver based on the
> GXIO IORPC support in the tilegx software stack, using the on-chip
> mPIPE packet processing engine.
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/tile/tilegx.c
[...]
> +/* Define to support GSO. */
> +#undef TILE_NET_GSO
GSO is always enabled by the networking core.
> +/* Define to support TSO. */
> +#define TILE_NET_TSO
No, put NETIF_F_TSO in hw_features so it can be switched at run-time.
(Currently that won't work if you don't set dev->ethtool_ops, but that's
a bug that can be fixed.)
> +/* Use 3000 to enable the Linux Traffic Control (QoS) layer, else 0. */
> +#define TILE_NET_TX_QUEUE_LEN 0
This can be changed through sysfs, so there is no need for a compile-
time option.
> +/* Define to dump packets (prints out the whole packet on tx and rx). */
> +#undef TILE_NET_DUMP_PACKETS
Should really be controlled through a 'debug' module parameter (see
netif_msg_init(), netif_msg_pktdata(), etc.)
[...]
> +/* Total header bytes per equeue slot. Must be big enough for 2 bytes
> + * of NET_IP_ALIGN alignment, plus 14 bytes (?) of L2 header, plus up to
> + * 60 bytes of actual TCP header. We round up to align to cache lines.
> + */
> +#define HEADER_BYTES 128
> +
> +/* Maximum completions per cpu per device (must be a power of two).
> + * ISSUE: What is the right number here?
> + */
> +#define TILE_NET_MAX_COMPS 64
> +
> +#define MAX_FRAGS (65536 / PAGE_SIZE + 2 + 1)
Should be MAX_SKB_FRAGS + 1.
[...]
> +/* Help the kernel transmit a packet. */
> +static int tile_net_tx(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct tile_net_priv *priv = netdev_priv(dev);
> +
> + struct tile_net_info *info = &__get_cpu_var(per_cpu_info);
> +
> + struct tile_net_egress *egress = &egress_for_echannel[priv->echannel];
> + gxio_mpipe_equeue_t *equeue = egress->equeue;
> +
> + struct tile_net_comps *comps =
> + info->comps_for_echannel[priv->echannel];
> +
> + struct skb_shared_info *sh = skb_shinfo(skb);
> +
> + unsigned int len = skb->len;
> + unsigned char *data = skb->data;
> +
> + unsigned int num_frags;
> + struct frag frags[MAX_FRAGS];
> + gxio_mpipe_edesc_t edescs[MAX_FRAGS];
> +
> + unsigned int i;
> +
> + int cid;
> +
> + s64 slot;
> +
> + unsigned long irqflags;
Please, no blank lines between your declarations.
[...]
> + /* Reserve slots, or return NETDEV_TX_BUSY if "full". */
> + slot = gxio_mpipe_equeue_try_reserve(equeue, num_frags);
> + if (slot < 0) {
> + local_irq_restore(irqflags);
> + /* ISSUE: "Virtual device xxx asks to queue packet". */
> + return NETDEV_TX_BUSY;
> + }
You're supposed to stop queues when they're full. And since that state
appears to be per-CPU, I think this device needs to be multiqueue with
one TX queue per CPU and ndo_select_queue defined accordingly.
> + for (i = 0; i < num_frags; i++)
> + gxio_mpipe_equeue_put_at(equeue, edescs[i], slot + i);
> +
> + /* Wait for a free completion entry.
> + * ISSUE: Is this the best logic?
> + * ISSUE: Can this cause undesirable "blocking"?
> + */
> + while (comps->comp_next - comps->comp_last >= TILE_NET_MAX_COMPS - 1)
> + tile_net_free_comps(equeue, comps, 32, false);
I'm not convinced you should be processing completions here at all. But
certainly you should have stopped the queue earlier rather than having
to wait here.
> + /* Update the completions array. */
> + cid = comps->comp_next % TILE_NET_MAX_COMPS;
> + comps->comp_queue[cid].when = slot + num_frags;
> + comps->comp_queue[cid].skb = skb;
> + comps->comp_next++;
> +
> + /* HACK: Track "expanded" size for short packets (e.g. 42 < 60). */
> + atomic_add(1, (atomic_t *)&priv->stats.tx_packets);
> + atomic_add((len >= ETH_ZLEN) ? len : ETH_ZLEN,
> + (atomic_t *)&priv->stats.tx_bytes);
You mustn't treat random fields to atomic_t. For one thing, atomic_t
contains an int while stats are unsigned long...
Also, you're adding cache contention between all your CPUs here. You
should maintain these stats per-CPU and then sum them in
tile_net_get_stats(). Then you can just use ordinary additions.
[...]
> +/* Ioctl commands. */
> +static int tile_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> +{
> + return -EOPNOTSUPP;
> +}
So why define it at all?
[...]
> +static void tile_net_dev_init(const char *name, const uint8_t *mac)
> +{
[...]
> + /* Register the network device. */
> + ret = register_netdev(dev);
> + if (ret) {
> + netdev_err(dev, "register_netdev failed %d\n", ret);
> + free_netdev(dev);
> + return;
> + }
> +
> + /* Get the MAC address and set it in the device struct; this must
> + * be done before the device is opened.
[...]
So you had better do this before calling register_netdev(), as the
device can be opened immediately after that...
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.
next prev parent reply other threads:[~2012-05-11 13:54 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-04 20:39 [PATCH 0/6] arch/tile: provide tilegx networking support Chris Metcalf
2012-04-04 20:39 ` [PATCH 1/6] arch/tile: introduce GXIO IORPC framework for tilegx Chris Metcalf
2012-04-04 20:58 ` [PATCH 4/6] arch/tile: common DMA code for the GXIO IORPC subsystem Chris Metcalf
2012-04-06 17:41 ` [PATCH 2/6] arch/tile: fix set_pte() to properly handle kernel MMIO mappings Chris Metcalf
2012-04-06 17:52 ` [PATCH 3/6] arch/tile: support MMIO-based readb/writeb etc Chris Metcalf
2012-04-09 13:24 ` Arnd Bergmann
2012-04-09 20:53 ` Chris Metcalf
2012-04-06 20:38 ` [PATCH 5/6] arch/tile: provide kernel support for the tilegx mPIPE shim Chris Metcalf
2012-04-09 13:34 ` Arnd Bergmann
2012-04-09 21:04 ` Chris Metcalf
2012-04-06 20:42 ` [PATCH 6/6] tilegx network driver: initial support Chris Metcalf
2012-04-09 13:49 ` Arnd Bergmann
2012-04-09 21:30 ` Chris Metcalf
2012-04-10 10:42 ` Arnd Bergmann
2012-04-12 23:23 ` Chris Metcalf
2012-04-13 10:34 ` Arnd Bergmann
2012-04-28 22:07 ` Chris Metcalf
2012-04-04 20:39 ` [PATCH v2 0/6] arch/tile: networking support for tilegx Chris Metcalf
2012-04-04 20:39 ` [PATCH v2 1/6] arch/tile: introduce GXIO IORPC framework " Chris Metcalf
2012-04-04 20:58 ` [PATCH v2 3/6] arch/tile: common DMA code for the GXIO IORPC subsystem Chris Metcalf
2012-04-06 17:52 ` [PATCH v2 2/6] arch/tile: support MMIO-based readb/writeb etc Chris Metcalf
2012-04-06 20:38 ` [PATCH v2 4/6] arch/tile: provide kernel support for the tilegx mPIPE shim Chris Metcalf
2012-04-06 20:42 ` [PATCH v2 6/6] tilegx network driver: initial support Chris Metcalf
2012-04-30 14:35 ` Arnd Bergmann
2001-09-17 4:00 ` [PATCH v3] " Chris Metcalf
2012-05-03 5:41 ` David Miller
2012-05-03 15:45 ` Chris Metcalf
2012-05-03 17:07 ` David Miller
2012-05-03 17:25 ` Chris Metcalf
2012-05-03 16:41 ` [PATCH v4] " Chris Metcalf
2012-05-04 6:42 ` David Miller
2012-05-09 10:42 ` [PATCH v5] " Chris Metcalf
2012-05-11 13:54 ` Ben Hutchings [this message]
2012-05-20 4:42 ` [PATCH v6] " Chris Metcalf
2012-05-20 20:55 ` David Miller
2012-05-23 20:42 ` [PATCH v7] " Chris Metcalf
2012-05-24 4:31 ` David Miller
2012-05-25 14:42 ` [PATCH v8] " Chris Metcalf
2012-06-04 20:12 ` [PATCH v9] " Chris Metcalf
2012-06-06 16:41 ` David Miller
2012-06-06 17:31 ` Eric Dumazet
2012-06-06 17:40 ` Eric Dumazet
2012-06-06 18:36 ` Chris Metcalf
2012-06-06 18:54 ` David Miller
2001-09-17 4:00 ` [PATCH v10] " Chris Metcalf
2012-04-06 20:42 ` Chris Metcalf
2012-06-07 20:39 ` David Miller
2012-06-07 20:44 ` Chris Metcalf
2012-06-07 20:47 ` Chris Metcalf
2012-06-07 20:50 ` Ben Hutchings
2012-06-07 20:52 ` Joe Perches
2012-06-07 20:45 ` Chris Metcalf
2012-06-12 0:03 ` David Miller
2012-06-12 13:14 ` Chris Metcalf
2012-06-06 18:10 ` [PATCH v9] " Eric Dumazet
2012-06-06 18:17 ` David Miller
2012-06-06 18:19 ` Ben Hutchings
2012-05-20 16:35 ` [PATCH v5] " Chris Metcalf
2012-04-28 19:41 ` [PATCH v2 5/6] arch/tile: break out the "csum a long" function to <asm/checksum.h> Chris Metcalf
2012-04-29 11:15 ` [PATCH 6/6] tilegx network driver: initial support Arnd Bergmann
2012-04-15 23:06 ` Chris Metcalf
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=1336744445.2874.57.camel@bwh-desktop.uk.solarflarecom.com \
--to=bhutchings@solarflare.com \
--cc=arnd@arndb.de \
--cc=cmetcalf@tilera.com \
--cc=davem@davemloft.net \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).