* [PATCH net-2.6.24] chelsio: sparse warning fixes (old cxgb2)
From: Stephen Hemminger @ 2007-10-08 23:19 UTC (permalink / raw)
To: Jeff Garzik, David S. Miller; +Cc: netdev, Divy Le Ray
Fix problems detected by sparse:
1. whole chunk of MAC code was for defined and never used
2. hook for running ext intr in workqueue wasn't being used
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
--- a/drivers/net/chelsio/Makefile 2007-10-04 09:33:28.000000000 -0700
+++ b/drivers/net/chelsio/Makefile 2007-10-08 16:06:05.000000000 -0700
@@ -4,6 +4,6 @@
obj-$(CONFIG_CHELSIO_T1) += cxgb.o
-cxgb-$(CONFIG_CHELSIO_T1_1G) += mac.o mv88e1xxx.o vsc7326.o
+cxgb-$(CONFIG_CHELSIO_T1_1G) += mv88e1xxx.o vsc7326.o
cxgb-objs := cxgb2.o espi.o tp.o pm3393.o sge.o subr.o \
mv88x201x.o my3126.o $(cxgb-y)
--- a/drivers/net/chelsio/common.h 2007-10-04 09:33:28.000000000 -0700
+++ b/drivers/net/chelsio/common.h 2007-10-08 16:04:50.000000000 -0700
@@ -372,6 +372,7 @@ extern void t1_interrupts_enable(adapter
extern void t1_interrupts_disable(adapter_t *adapter);
extern void t1_interrupts_clear(adapter_t *adapter);
extern int t1_elmer0_ext_intr_handler(adapter_t *adapter);
+extern void t1_elmer0_ext_intr(adapter_t *adapter);
extern int t1_slow_intr_handler(adapter_t *adapter);
extern int t1_link_start(struct cphy *phy, struct cmac *mac, struct link_config *lc);
--- a/drivers/net/chelsio/mac.c 2007-10-08 16:06:32.000000000 -0700
+++ /dev/null 1970-01-01 00:00:00.000000000 +0000
@@ -1,368 +0,0 @@
-/* $Date: 2005/10/22 00:42:59 $ $RCSfile: mac.c,v $ $Revision: 1.32 $ */
-#include "gmac.h"
-#include "regs.h"
-#include "fpga_defs.h"
-
-#define MAC_CSR_INTERFACE_GMII 0x0
-#define MAC_CSR_INTERFACE_TBI 0x1
-#define MAC_CSR_INTERFACE_MII 0x2
-#define MAC_CSR_INTERFACE_RMII 0x3
-
-/* Chelsio's MAC statistics. */
-struct mac_statistics {
-
- /* Transmit */
- u32 TxFramesTransmittedOK;
- u32 TxReserved1;
- u32 TxReserved2;
- u32 TxOctetsTransmittedOK;
- u32 TxFramesWithDeferredXmissions;
- u32 TxLateCollisions;
- u32 TxFramesAbortedDueToXSCollisions;
- u32 TxFramesLostDueToIntMACXmitError;
- u32 TxReserved3;
- u32 TxMulticastFrameXmittedOK;
- u32 TxBroadcastFramesXmittedOK;
- u32 TxFramesWithExcessiveDeferral;
- u32 TxPAUSEMACCtrlFramesTransmitted;
-
- /* Receive */
- u32 RxFramesReceivedOK;
- u32 RxFrameCheckSequenceErrors;
- u32 RxAlignmentErrors;
- u32 RxOctetsReceivedOK;
- u32 RxFramesLostDueToIntMACRcvError;
- u32 RxMulticastFramesReceivedOK;
- u32 RxBroadcastFramesReceivedOK;
- u32 RxInRangeLengthErrors;
- u32 RxTxOutOfRangeLengthField;
- u32 RxFrameTooLongErrors;
- u32 RxPAUSEMACCtrlFramesReceived;
-};
-
-static int static_aPorts[] = {
- FPGA_GMAC_INTERRUPT_PORT0,
- FPGA_GMAC_INTERRUPT_PORT1,
- FPGA_GMAC_INTERRUPT_PORT2,
- FPGA_GMAC_INTERRUPT_PORT3
-};
-
-struct _cmac_instance {
- u32 index;
-};
-
-static int mac_intr_enable(struct cmac *mac)
-{
- u32 mac_intr;
-
- if (t1_is_asic(mac->adapter)) {
- /* ASIC */
-
- /* We don't use the on chip MAC for ASIC products. */
- } else {
- /* FPGA */
-
- /* Set parent gmac interrupt. */
- mac_intr = readl(mac->adapter->regs + A_PL_ENABLE);
- mac_intr |= FPGA_PCIX_INTERRUPT_GMAC;
- writel(mac_intr, mac->adapter->regs + A_PL_ENABLE);
-
- mac_intr = readl(mac->adapter->regs + FPGA_GMAC_ADDR_INTERRUPT_ENABLE);
- mac_intr |= static_aPorts[mac->instance->index];
- writel(mac_intr,
- mac->adapter->regs + FPGA_GMAC_ADDR_INTERRUPT_ENABLE);
- }
-
- return 0;
-}
-
-static int mac_intr_disable(struct cmac *mac)
-{
- u32 mac_intr;
-
- if (t1_is_asic(mac->adapter)) {
- /* ASIC */
-
- /* We don't use the on chip MAC for ASIC products. */
- } else {
- /* FPGA */
-
- /* Set parent gmac interrupt. */
- mac_intr = readl(mac->adapter->regs + A_PL_ENABLE);
- mac_intr &= ~FPGA_PCIX_INTERRUPT_GMAC;
- writel(mac_intr, mac->adapter->regs + A_PL_ENABLE);
-
- mac_intr = readl(mac->adapter->regs + FPGA_GMAC_ADDR_INTERRUPT_ENABLE);
- mac_intr &= ~(static_aPorts[mac->instance->index]);
- writel(mac_intr,
- mac->adapter->regs + FPGA_GMAC_ADDR_INTERRUPT_ENABLE);
- }
-
- return 0;
-}
-
-static int mac_intr_clear(struct cmac *mac)
-{
- u32 mac_intr;
-
- if (t1_is_asic(mac->adapter)) {
- /* ASIC */
-
- /* We don't use the on chip MAC for ASIC products. */
- } else {
- /* FPGA */
-
- /* Set parent gmac interrupt. */
- writel(FPGA_PCIX_INTERRUPT_GMAC,
- mac->adapter->regs + A_PL_CAUSE);
- mac_intr = readl(mac->adapter->regs + FPGA_GMAC_ADDR_INTERRUPT_CAUSE);
- mac_intr |= (static_aPorts[mac->instance->index]);
- writel(mac_intr,
- mac->adapter->regs + FPGA_GMAC_ADDR_INTERRUPT_CAUSE);
- }
-
- return 0;
-}
-
-static int mac_get_address(struct cmac *mac, u8 addr[6])
-{
- u32 data32_lo, data32_hi;
-
- data32_lo = readl(mac->adapter->regs
- + MAC_REG_IDLO(mac->instance->index));
- data32_hi = readl(mac->adapter->regs
- + MAC_REG_IDHI(mac->instance->index));
-
- addr[0] = (u8) ((data32_hi >> 8) & 0xFF);
- addr[1] = (u8) ((data32_hi) & 0xFF);
- addr[2] = (u8) ((data32_lo >> 24) & 0xFF);
- addr[3] = (u8) ((data32_lo >> 16) & 0xFF);
- addr[4] = (u8) ((data32_lo >> 8) & 0xFF);
- addr[5] = (u8) ((data32_lo) & 0xFF);
- return 0;
-}
-
-static int mac_reset(struct cmac *mac)
-{
- u32 data32;
- int mac_in_reset, time_out = 100;
- int idx = mac->instance->index;
-
- data32 = readl(mac->adapter->regs + MAC_REG_CSR(idx));
- writel(data32 | F_MAC_RESET,
- mac->adapter->regs + MAC_REG_CSR(idx));
-
- do {
- data32 = readl(mac->adapter->regs + MAC_REG_CSR(idx));
-
- mac_in_reset = data32 & F_MAC_RESET;
- if (mac_in_reset)
- udelay(1);
- } while (mac_in_reset && --time_out);
-
- if (mac_in_reset) {
- CH_ERR("%s: MAC %d reset timed out\n",
- mac->adapter->name, idx);
- return 2;
- }
-
- return 0;
-}
-
-static int mac_set_rx_mode(struct cmac *mac, struct t1_rx_mode *rm)
-{
- u32 val;
-
- val = readl(mac->adapter->regs
- + MAC_REG_CSR(mac->instance->index));
- val &= ~(F_MAC_PROMISC | F_MAC_MC_ENABLE);
- val |= V_MAC_PROMISC(t1_rx_mode_promisc(rm) != 0);
- val |= V_MAC_MC_ENABLE(t1_rx_mode_allmulti(rm) != 0);
- writel(val,
- mac->adapter->regs + MAC_REG_CSR(mac->instance->index));
-
- return 0;
-}
-
-static int mac_set_speed_duplex_fc(struct cmac *mac, int speed, int duplex,
- int fc)
-{
- u32 data32;
-
- data32 = readl(mac->adapter->regs
- + MAC_REG_CSR(mac->instance->index));
- data32 &= ~(F_MAC_HALF_DUPLEX | V_MAC_SPEED(M_MAC_SPEED) |
- V_INTERFACE(M_INTERFACE) | F_MAC_TX_PAUSE_ENABLE |
- F_MAC_RX_PAUSE_ENABLE);
-
- switch (speed) {
- case SPEED_10:
- case SPEED_100:
- data32 |= V_INTERFACE(MAC_CSR_INTERFACE_MII);
- data32 |= V_MAC_SPEED(speed == SPEED_10 ? 0 : 1);
- break;
- case SPEED_1000:
- data32 |= V_INTERFACE(MAC_CSR_INTERFACE_GMII);
- data32 |= V_MAC_SPEED(2);
- break;
- }
-
- if (duplex >= 0)
- data32 |= V_MAC_HALF_DUPLEX(duplex == DUPLEX_HALF);
-
- if (fc >= 0) {
- data32 |= V_MAC_RX_PAUSE_ENABLE((fc & PAUSE_RX) != 0);
- data32 |= V_MAC_TX_PAUSE_ENABLE((fc & PAUSE_TX) != 0);
- }
-
- writel(data32,
- mac->adapter->regs + MAC_REG_CSR(mac->instance->index));
- return 0;
-}
-
-static int mac_enable(struct cmac *mac, int which)
-{
- u32 val;
-
- val = readl(mac->adapter->regs
- + MAC_REG_CSR(mac->instance->index));
- if (which & MAC_DIRECTION_RX)
- val |= F_MAC_RX_ENABLE;
- if (which & MAC_DIRECTION_TX)
- val |= F_MAC_TX_ENABLE;
- writel(val,
- mac->adapter->regs + MAC_REG_CSR(mac->instance->index));
- return 0;
-}
-
-static int mac_disable(struct cmac *mac, int which)
-{
- u32 val;
-
- val = readl(mac->adapter->regs
- + MAC_REG_CSR(mac->instance->index));
- if (which & MAC_DIRECTION_RX)
- val &= ~F_MAC_RX_ENABLE;
- if (which & MAC_DIRECTION_TX)
- val &= ~F_MAC_TX_ENABLE;
- writel(val,
- mac->adapter->regs + MAC_REG_CSR(mac->instance->index));
- return 0;
-}
-
-#if 0
-static int mac_set_ifs(struct cmac *mac, u32 mode)
-{
- t1_write_reg_4(mac->adapter,
- MAC_REG_IFS(mac->instance->index),
- mode);
- return 0;
-}
-
-static int mac_enable_isl(struct cmac *mac)
-{
- u32 data32 = readl(mac->adapter->regs
- + MAC_REG_CSR(mac->instance->index));
- data32 |= F_MAC_RX_ENABLE | F_MAC_TX_ENABLE;
- t1_write_reg_4(mac->adapter,
- MAC_REG_CSR(mac->instance->index),
- data32);
- return 0;
-}
-#endif
-
-static int mac_set_mtu(struct cmac *mac, int mtu)
-{
- if (mtu > 9600)
- return -EINVAL;
- writel(mtu + ETH_HLEN + VLAN_HLEN,
- mac->adapter->regs + MAC_REG_LARGEFRAMELENGTH(mac->instance->index));
-
- return 0;
-}
-
-static const struct cmac_statistics *mac_update_statistics(struct cmac *mac,
- int flag)
-{
- struct mac_statistics st;
- u32 *p = (u32 *) & st, i;
-
- writel(0,
- mac->adapter->regs + MAC_REG_RMCNT(mac->instance->index));
-
- for (i = 0; i < sizeof(st) / sizeof(u32); i++)
- *p++ = readl(mac->adapter->regs
- + MAC_REG_RMDATA(mac->instance->index));
-
- /* XXX convert stats */
- return &mac->stats;
-}
-
-static void mac_destroy(struct cmac *mac)
-{
- kfree(mac);
-}
-
-static struct cmac_ops chelsio_mac_ops = {
- .destroy = mac_destroy,
- .reset = mac_reset,
- .interrupt_enable = mac_intr_enable,
- .interrupt_disable = mac_intr_disable,
- .interrupt_clear = mac_intr_clear,
- .enable = mac_enable,
- .disable = mac_disable,
- .set_mtu = mac_set_mtu,
- .set_rx_mode = mac_set_rx_mode,
- .set_speed_duplex_fc = mac_set_speed_duplex_fc,
- .macaddress_get = mac_get_address,
- .statistics_update = mac_update_statistics,
-};
-
-static struct cmac *mac_create(adapter_t *adapter, int index)
-{
- struct cmac *mac;
- u32 data32;
-
- if (index >= 4)
- return NULL;
-
- mac = kzalloc(sizeof(*mac) + sizeof(cmac_instance), GFP_KERNEL);
- if (!mac)
- return NULL;
-
- mac->ops = &chelsio_mac_ops;
- mac->instance = (cmac_instance *) (mac + 1);
-
- mac->instance->index = index;
- mac->adapter = adapter;
-
- data32 = readl(adapter->regs + MAC_REG_CSR(mac->instance->index));
- data32 &= ~(F_MAC_RESET | F_MAC_PROMISC | F_MAC_PROMISC |
- F_MAC_LB_ENABLE | F_MAC_RX_ENABLE | F_MAC_TX_ENABLE);
- data32 |= F_MAC_JUMBO_ENABLE;
- writel(data32, adapter->regs + MAC_REG_CSR(mac->instance->index));
-
- /* Initialize the random backoff seed. */
- data32 = 0x55aa + (3 * index);
- writel(data32,
- adapter->regs + MAC_REG_GMRANDBACKOFFSEED(mac->instance->index));
-
- /* Check to see if the mac address needs to be set manually. */
- data32 = readl(adapter->regs + MAC_REG_IDLO(mac->instance->index));
- if (data32 == 0 || data32 == 0xffffffff) {
- /*
- * Add a default MAC address if we can't read one.
- */
- writel(0x43FFFFFF - index,
- adapter->regs + MAC_REG_IDLO(mac->instance->index));
- writel(0x0007,
- adapter->regs + MAC_REG_IDHI(mac->instance->index));
- }
-
- (void) mac_set_mtu(mac, 1500);
- return mac;
-}
-
-const struct gmac t1_chelsio_mac_ops = {
- .create = mac_create
-};
--- a/drivers/net/chelsio/subr.c 2007-10-08 16:04:04.000000000 -0700
+++ b/drivers/net/chelsio/subr.c 2007-10-08 16:08:10.000000000 -0700
@@ -884,7 +884,7 @@ static int asic_slow_intr(adapter_t *ada
if (cause & F_PL_INTR_PCIX)
t1_pci_intr_handler(adapter);
if (cause & F_PL_INTR_EXT)
- t1_elmer0_ext_intr_handler(adapter);
+ t1_elmer0_ext_intr(adapter);
/* Clear the interrupts just processed. */
writel(cause, adapter->regs + A_PL_CAUSE);
^ permalink raw reply
* Re: [PATCH]: Third (final?) release of Sun Neptune driver
From: David Miller @ 2007-10-08 23:08 UTC (permalink / raw)
To: oliver; +Cc: netdev
In-Reply-To: <470A5450.70403@hartkopp.net>
From: Oliver Hartkopp <oliver@hartkopp.net>
Date: Mon, 08 Oct 2007 18:01:20 +0200
> David Miller wrote:
> > +
> > +static int link_status_1g(struct niu *np, int *link_up_p)
...
> Should *link_up_p be set to any value before returning?
...
> > +out:
> > + spin_unlock_irqrestore(&np->lock, flags);
> > +
> > + *link_up_p = link_up;
> > + return 0;
> > +}
> > +
> >
> Although you added a proper return value in link_status_10g() while
> fixing the locking issue, you should think about providing a similar
> return value for link_status_1g() to be constistent here.
Thanks for finding these two issues. I've fixed it in the patch
below.
Luckily, this case was harmless. When we enable a loopback mode,
niu_open() forces the link up by calling netif_carrier_on().
Then, niu_timer() always sees -EINVAL coming back from
niu_link_status() because of these loopback checks, and in such
cases never uses the link_up value because niu_link_status_common()
will not be invoked.
Anyways, thanks again!
commit ec3ec3566a8ee3c8c262606e06299c8212ebd53f
Author: David S. Miller <davem@sunset.davemloft.net>
Date: Mon Oct 8 16:08:17 2007 -0700
[NIU]: Make sure link_up status is set to something in link_status_{1,10}g().
Noticed by Oliver Hartkopp.
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index 2a69cee..7c09243 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -1082,13 +1082,14 @@ static int link_status_10g(struct niu *np, int *link_up_p)
unsigned long flags;
int err, link_up;
- if (np->link_config.loopback_mode != LOOPBACK_DISABLED)
- return -EINVAL;
-
link_up = 0;
spin_lock_irqsave(&np->lock, flags);
+ err = -EINVAL;
+ if (np->link_config.loopback_mode != LOOPBACK_DISABLED)
+ return out;
+
err = mdio_read(np, np->phy_addr, BCM8704_PMA_PMD_DEV_ADDR,
BCM8704_PMD_RCV_SIGDET);
if (err < 0)
@@ -1119,15 +1120,12 @@ static int link_status_10g(struct niu *np, int *link_up_p)
link_up = 1;
np->link_config.active_speed = SPEED_10000;
np->link_config.active_duplex = DUPLEX_FULL;
+ err = 0;
out:
spin_unlock_irqrestore(&np->lock, flags);
*link_up_p = link_up;
- return 0;
-
-out_err:
- spin_unlock_irqrestore(&np->lock, flags);
return err;
}
@@ -1138,15 +1136,16 @@ static int link_status_1g(struct niu *np, int *link_up_p)
u8 current_duplex;
int err, link_up;
- if (np->link_config.loopback_mode != LOOPBACK_DISABLED)
- return -EINVAL;
-
link_up = 0;
current_speed = SPEED_INVALID;
current_duplex = DUPLEX_INVALID;
spin_lock_irqsave(&np->lock, flags);
+ err = -EINVAL;
+ if (np->link_config.loopback_mode != LOOPBACK_DISABLED)
+ goto out;
+
err = mii_read(np, np->phy_addr, MII_BMSR);
if (err < 0)
goto out;
@@ -1172,6 +1171,7 @@ static int link_status_1g(struct niu *np, int *link_up_p)
goto out;
estat = err;
+ link_up = 1;
if (estat & (ESTATUS_1000_TFULL | ESTATUS_1000_THALF)) {
current_speed = SPEED_1000;
if (estat & ESTATUS_1000_TFULL)
@@ -1195,16 +1195,16 @@ static int link_status_1g(struct niu *np, int *link_up_p)
current_speed = SPEED_10;
current_duplex = DUPLEX_HALF;
} else
- goto out;
+ link_up = 0;
}
- link_up = 1;
}
+ err = 0;
out:
spin_unlock_irqrestore(&np->lock, flags);
*link_up_p = link_up;
- return 0;
+ return err;
}
static int niu_link_status(struct niu *np, int *link_up_p)
^ permalink raw reply related
* Re: pktgen question
From: David Miller @ 2007-10-08 22:54 UTC (permalink / raw)
To: swise; +Cc: greearb, rick.jones2, hadi, johnpol, netdev, Robert.Olsson
In-Reply-To: <470AB342.1030209@opengridcomputing.com>
From: Steve Wise <swise@opengridcomputing.com>
Date: Mon, 08 Oct 2007 17:46:26 -0500
> We're talking about just for pktgen...eh?
My bad, I'm happy to review a patch that uses the
skb->destructor in pktgen to achieve this.
^ permalink raw reply
* Re: pktgen question
From: Steve Wise @ 2007-10-08 22:46 UTC (permalink / raw)
To: David Miller; +Cc: greearb, rick.jones2, hadi, johnpol, netdev, Robert.Olsson
In-Reply-To: <20071008.152234.13748626.davem@davemloft.net>
David Miller wrote:
> From: Ben Greear <greearb@candelatech.com>
> Date: Mon, 08 Oct 2007 14:57:13 -0700
>
>> This skb recycling can certainly work and has been done several
>> times before. It never gets into the kernel though.
>
> Because it doesn't work.
>
> A socket can hang onto a receive packet essentially forever.
>
> You cannot therefore rely upon the network stack to give you the
> packet back in some reasonable finite amount of time. This is simply
> the nature of the beast.
>
> Which means that you either:
>
> 1) Starve and stop receiving packets when the recycling ring
> runs out because all of those packets are stuck in socket
> buffers. This is easily DoS'able by users on your system
>
> 2) End up allocating new packets anyways, but then what's the
> point of the recycling ring? It's just a hack that works
> when everything goes as planned, and in real life that is
> rarely the case.
>
> It also makes the driver locking more complicated. Packet
> input occurs in NAPI drivers via netif_receive_skb() which
> would be capable of recycling packets back to the same
> driver in a recycling scheme. But the recycling can occur
> from other contexts too. The netif_receive_skb() caller
> already has atomic access to the receive queue, but those
> other callback cases do not.
>
> That locking issue is just the tip of the iceberg. Once you
> start discussing solutions, all sorts of new issues begin to
> pop up.
>
> SKB recycling, just say no.
>
We're talking about just for pktgen...eh?
^ permalink raw reply
* Re: [PATCH][E1000E] some cleanups
From: Kok, Auke @ 2007-10-08 22:40 UTC (permalink / raw)
To: hadi; +Cc: netdev
In-Reply-To: <1191773723.4394.31.camel@localhost>
jamal wrote:
> Ok, here you go; the explanation is below. This is from net-2.6.24 of
> early this AM. I saw a patch you posted that is derived from Krishna;
> although it hasnt showed up in the tree - i have considered those
> changes and this patch adds a little more optimization in case of
> errors.
>
> I will send you a patch to kill LLTX the sooner this shows up somewhere.
thanks.
My biggest problem with the patch as you sent it that it's a tonload of changes
and no implicit benefit immediately as I can see. I would really have to see the
LLTX change as well to make a wellfounded decision whether it is the right thing
to go and overhaul this part of the driver or not :)
I'm not particularly against the changes per se though.
so please, if needed offlist send me those LLTX changes as well.
Auke
>
> On Wed, 2007-03-10 at 09:18 -0400, jamal wrote:
>
>> The cleanup is to break up the code so it is functionally more readable
>> from a perspective of the 4 distinct parts in ->hard_start_xmit():
>>
>> a) packet formatting (example: vlan, mss, descriptor counting, etc.)
>> b) chip-specific formatting
>> c) enqueueing the packet on a DMA ring
>> d) IO operations to complete packet transmit, tell DMA engine to chew
>> on, tx completion interrupts, set last tx time, etc.
>>
>> Each of those steps sitting in different functions accumulates state
>> that is used in the next steps. cb stores this state because it a
>> scratchpad the driver owns. You could create some other structure and
>> pass it around the iteration, but why waste more bytes.
>
^ permalink raw reply
* [ofa-general] RE: [PATCH 2/3][NET_BATCH] net core use batching
From: Waskiewicz Jr, Peter P @ 2007-10-08 22:36 UTC (permalink / raw)
To: hadi, David Miller
Cc: johnpol, herbert, gaagaan, Robert.Olsson, netdev, rdreier,
mcarlson, randy.dunlap, jagana, general, mchan, tgraf, jeff, sri,
shemminger, kaber
In-Reply-To: <1191882858.4373.103.camel@localhost>
> If net_device_subqueue is visible from both driver and core
> scheduler area (couldnt tell from looking at whats in there
> already), then that'll do it.
Yes, I use the net_device_subqueue structs (the state variable in there)
in the prio and rr qdiscs right now. It's an indexed list at the very
end of struct netdevice.
-PJ Waskiewicz
^ permalink raw reply
* [ofa-general] RE: parallel networking
From: Waskiewicz Jr, Peter P @ 2007-10-08 22:35 UTC (permalink / raw)
To: David Miller, hadi
Cc: johnpol, kumarkr, herbert, jeff, Robert.Olsson, netdev, rdreier,
linux-kernel, randy.dunlap, gaagaan, jagana, general, mchan,
tgraf, mingo, sri, shemminger, kaber
In-Reply-To: <20071008.153353.58431888.davem@davemloft.net>
> Multiply whatever effect you think you might be able to
> measure due to that on your 2 or 4 way system, and multiple
> it up to 64 cpus or so for machines I am using. This is
> where machines are going, and is going to become the norm.
That along with speeds going to 10 GbE with multiple Tx/Rx queues (with
40 and 100 GbE under discussion now), where multiple CPU's hitting the
driver are needed to push line rate without cratering the entire
machine.
-PJ Waskiewicz
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: jamal @ 2007-10-08 22:34 UTC (permalink / raw)
To: David Miller
Cc: johnpol, herbert, gaagaan, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, mcarlson, randy.dunlap, jagana, general,
mchan, tgraf, jeff, sri, shemminger, kaber
In-Reply-To: <20071008.142626.26988698.davem@davemloft.net>
On Mon, 2007-08-10 at 14:26 -0700, David Miller wrote:
> Add xmit_win to struct net_device_subqueue, problem solved.
If net_device_subqueue is visible from both driver and core scheduler
area (couldnt tell from looking at whats in there already), then that'll
do it.
cheers,
jamal
^ permalink raw reply
* Re: parallel networking
From: David Miller @ 2007-10-08 22:33 UTC (permalink / raw)
To: hadi
Cc: jeff, peter.p.waskiewicz.jr, krkumar2, johnpol, herbert, kaber,
shemminger, jagana, Robert.Olsson, rick.jones2, xma, gaagaan,
netdev, rdreier, mingo, mchan, general, kumarkr, tgraf,
randy.dunlap, sri, linux-kernel
In-Reply-To: <1191882618.4373.99.camel@localhost>
From: jamal <hadi@cyberus.ca>
Date: Mon, 08 Oct 2007 18:30:18 -0400
> Very quickly there are no more packets for it to dequeue from the
> qdisc or the driver is stoped and it has to get out of there. If you
> dont have any interupt tied to a specific cpu then you can have many
> cpus enter and leave that region all the time.
With the lock shuttling back and forth between those cpus, which is
what we're trying to avoid.
Multiply whatever effect you think you might be able to measure due to
that on your 2 or 4 way system, and multiple it up to 64 cpus or so
for machines I am using. This is where machines are going, and is
going to become the norm.
^ permalink raw reply
* [ofa-general] RE: [PATCH 2/3][NET_BATCH] net core use batching
From: Waskiewicz Jr, Peter P @ 2007-10-08 22:33 UTC (permalink / raw)
To: hadi
Cc: johnpol, herbert, gaagaan, Robert.Olsson, netdev, rdreier,
mcarlson, kaber, randy.dunlap, jagana, general, mchan, tgraf,
jeff, sri, shemminger, David Miller
In-Reply-To: <1191876530.4373.58.camel@localhost>
> true, that needs some resolution. Heres a hand-waving thought:
> Assuming all packets of a specific map end up in the same
> qdiscn queue, it seems feasible to ask the qdisc scheduler to
> give us enough packages (ive seen people use that terms to
> refer to packets) for each hardware ring's available space.
> With the patches i posted, i do that via
> dev->xmit_win that assumes only one view of the driver; essentially a
> single ring.
> If that is doable, then it is up to the driver to say "i have
> space for 5 in ring[0], 10 in ring[1] 0 in ring[2]" based on
> what scheduling scheme the driver implements - the dev->blist
> can stay the same. Its a handwave, so there may be issues
> there and there could be better ways to handle this.
>
> Note: The other issue that needs resolving that i raised
> earlier was in regards to multiqueue running on multiple cpus
> servicing different rings concurently.
I can see the qdisc being modified to send batches per queue_mapping.
This shouldn't be too difficult, and if we had the xmit_win per queue
(in the subqueue struct like Dave pointed out).
Addressing your note/issue with different rings being services
concurrently: I'd like to remove the QDISC_RUNNING bit from the global
device; with Tx multiqueue, this bit should be set on each queue (if at
all), allowing multiple Tx rings to be loaded simultaneously. The
biggest issue today with the multiqueue implementation is the global
queue_lock. I see it being a hot source of contention in my testing; my
setup is a 8-core machine (dual quad-core procs) with a 10GbE NIC, using
8 Tx and 8 Rx queues. On transmit, when loading all 8 queues, the
enqueue/dequeue are hitting that lock quite a bit for the whole device.
I really think that the queue_lock should join the queue_state, so the
device no longer manages the top-level state (since we're operating
per-queue instead of per-device).
> It's the core that does that, not the driver; the driver
> continues to use ->hard_start_xmit() (albeit modified one).
> The idea is not to have many new interfaces.
I'll look closer at this, since I think I confused myself.
> Isnt multiqueue mainline already?
Well, it's in 2.6.23-rc*. I imagine it won't see much action though
until 2.6.24, since people will be porting drivers during that time.
Plus having the native Rx multiqueue w/NAPI code in 2.6.24 makes sense
to have Tx multiqueue at that time.
> The point behind batching is to reduce the cost of the locks
> by amortizing across the locks. Even better if one can, they
> should get rid of locks. Remind me, why do you need the
> per-queuemap lock? And is it needed from the enqueuing side
> too? Maybe lets start there to help me understand things?
The multiqueue implementation today enforces the number of qdisc bands
(RR or PRIO) to be equal to the number of Tx rings your hardware/driver
is supporting. Therefore, the queue_lock and queue_state in the kernel
directly relate to the qdisc band management. If the queue stops from
the driver, then the qdisc won't try to dequeue from the band. What I'm
working on is to move the lock there too, so I can lock the queue when I
enqueue (protect the band from multiple sources modifying the skb
chain), and lock it when I dequeue. This is purely for concurrency of
adding/popping skb's from the qdisc queues. Right now, we take the
whole global lock to add and remove skb's. This is the next logical
step for separating the queue dependancy on each other. Please let me
know if this doesn't make sense, or if you have any questions at all
about my reasoning. I agree that this is where we should be on the same
page before moving onto anything else in this discussion. :)
> Sure, that is doable if the driver can set a per
> queue_mapping xmit_win and the qdisc can be taught to say
> "give me packets for queue_mapping X"
Yes, I like this idea very much. Do that, modify the qdisc to send in
chunks from a queue, and the problem should be solved.
I will try and find some additional cycles to get my patches completely
working, and send them. It'd be easier I think to see what's going on
if I did that. I'll also try to make them work with the ideas of
xmit_win per queue and batched queue qdisc sends. Stay tuned...
Thanks Jamal,
-PJ Waskiewicz
<peter.p.waskiewicz.jr@intel.com>
^ permalink raw reply
* Re: parallel networking
From: jamal @ 2007-10-08 22:30 UTC (permalink / raw)
To: David Miller
Cc: jeff, peter.p.waskiewicz.jr, krkumar2, johnpol, herbert, kaber,
shemminger, jagana, Robert.Olsson, rick.jones2, xma, gaagaan,
netdev, rdreier, mingo, mchan, general, kumarkr, tgraf,
randy.dunlap, sri, linux-kernel
In-Reply-To: <20071008.141154.107706003.davem@davemloft.net>
On Mon, 2007-08-10 at 14:11 -0700, David Miller wrote:
> The problem is that the packet schedulers want global guarantees
> on packet ordering, not flow centric ones.
>
> That is the issue Jamal is concerned about.
indeed, thank you for giving it better wording.
> The more I think about it, the more inevitable it seems that we really
> might need multiple qdiscs, one for each TX queue, to pull this full
> parallelization off.
>
> But the semantics of that don't smell so nice either. If the user
> attaches a new qdisc to "ethN", does it go to all the TX queues, or
> what?
>
> All of the traffic shaping technology deals with the device as a unary
> object. It doesn't fit to multi-queue at all.
If you let only one CPU at a time access the "xmit path" you solve all
the reordering. If you want to be more fine grained you make the
serialization point as low as possible in the stack - perhaps in the
driver.
But I think even what we have today with only one cpu entering the
dequeue/scheduler region, _for starters_, is not bad actually ;-> What
i am finding (and i can tell you i have been trying hard;->) is that a
sufficiently fast cpu doesnt sit in the dequeue area for "too long" (and
batching reduces the time spent further). Very quickly there are no more
packets for it to dequeue from the qdisc or the driver is stoped and it
has to get out of there. If you dont have any interupt tied to a
specific cpu then you can have many cpus enter and leave that region all
the time.
cheers,
jamal
^ permalink raw reply
* Re: pktgen question
From: David Miller @ 2007-10-08 22:22 UTC (permalink / raw)
To: greearb; +Cc: swise, rick.jones2, hadi, johnpol, netdev, Robert.Olsson
In-Reply-To: <470AA7B9.4070703@candelatech.com>
From: Ben Greear <greearb@candelatech.com>
Date: Mon, 08 Oct 2007 14:57:13 -0700
> This skb recycling can certainly work and has been done several
> times before. It never gets into the kernel though.
Because it doesn't work.
A socket can hang onto a receive packet essentially forever.
You cannot therefore rely upon the network stack to give you the
packet back in some reasonable finite amount of time. This is simply
the nature of the beast.
Which means that you either:
1) Starve and stop receiving packets when the recycling ring
runs out because all of those packets are stuck in socket
buffers. This is easily DoS'able by users on your system
2) End up allocating new packets anyways, but then what's the
point of the recycling ring? It's just a hack that works
when everything goes as planned, and in real life that is
rarely the case.
It also makes the driver locking more complicated. Packet
input occurs in NAPI drivers via netif_receive_skb() which
would be capable of recycling packets back to the same
driver in a recycling scheme. But the recycling can occur
from other contexts too. The netif_receive_skb() caller
already has atomic access to the receive queue, but those
other callback cases do not.
That locking issue is just the tip of the iceberg. Once you
start discussing solutions, all sorts of new issues begin to
pop up.
SKB recycling, just say no.
^ permalink raw reply
* Re: pktgen question
From: Ben Greear @ 2007-10-08 21:57 UTC (permalink / raw)
To: Steve Wise; +Cc: Rick Jones, hadi, Evgeniy Polyakov, netdev, Robert Olsson
In-Reply-To: <470AA48B.4050005@opengridcomputing.com>
Steve Wise wrote:
>
>
> Ben Greear wrote:
>> Rick Jones wrote:
>>>>> Perf-wise, you could clone the skbs up front, then deliver them to
>>>>> the nic in a tight loop. This would mitigate the added overhead
>>>>> introduced by calling skb_clone() in the loop doing transmits...
>>>>
>>>> That only works if you are sending a small number of skbs. You
>>>> can't pre-clone several minutes worth of 10Gbe traffic
>>>> with any normal amount of RAM.
>>>
>>> Does pktgen really need to allocate anything more than some smallish
>>> fraction more than the depth of the driver's transmit queue?
>>
>> If you want to send sustained high rates of traffic, for more than
>> just a trivial amount of time, then you either have to play the current
>> trick with the skb_get(), or you have to allocate a real packet each time
>> (maybe with skb_clone() or similar, but it's still more overhead than
>> the skb_get
>> which only bumps a reference count.)
>>
>> I see no other way, but if you can think of one, please let me know.
>>
>
> You can keep freed skb's that were cloned on a free list, then reuse
> them once freed. You can detect when the driver frees them by adding a
> destroy function to the skb. So what will happen is the set of cloned
> skbs needed will eventually settled down to a constent amount and the
> amount will be based on the latency involved in transmitting a single
> skb. And it should be bounded by the max txq depth. Yes? (or am I all
> wet :)
>
> So you would pay the overhead of cloning only until you hit this steady
> state.
>
> Whatchathink?
This skb recycling can certainly work and has been done several
times before. It never gets into the kernel though.
Also, still a lot more overhead than incrementing the in-use counter
since you will have to re-initialize the pkt (you don't know what
all the underlying devices might have done to it.)
The number needed would be bound by all of the queues involved. There
could be several queues if you are running pktgen on virtual devices
(with potential queues between virtual devs and hardware devs).
Still, I at least would be interested in seeing a patch if you put
one together. You should get buy-in from Robert and/or DaveM if you
want it to have a chance to get into the kernel.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [ofa-general] Re: [PATCH RFC] RDMA/CMA: Allocate PS_TCP ports from the host TCP port space.
From: Steve Wise @ 2007-10-08 21:54 UTC (permalink / raw)
To: David Miller; +Cc: mshefty, rdreier, netdev, linux-kernel, general
In-Reply-To: <20070809.145534.102938208.davem@davemloft.net>
David Miller wrote:
> From: Sean Hefty <mshefty@ichips.intel.com>
> Date: Thu, 09 Aug 2007 14:40:16 -0700
>
>> Steve Wise wrote:
>>> Any more comments?
>> Does anyone have ideas on how to reserve the port space without using a
>> struct socket?
>
> How about we just remove the RDMA stack altogether? I am not at all
> kidding. If you guys can't stay in your sand box and need to cause
> problems for the normal network stack, it's unacceptable. We were
> told all along the if RDMA went into the tree none of this kind of
> stuff would be an issue.
>
> These are exactly the kinds of problems for which people like myself
> were dreading. These subsystems have no buisness using the TCP port
> space of the Linux software stack, absolutely none.
>
> After TCP port reservation, what's next? It seems an at least
> bi-monthly event that the RDMA folks need to put their fingers
> into something else in the normal networking stack. No more.
>
> I will NACK any patch that opens up sockets to eat up ports or
> anything stupid like that.
Hey Dave,
The hack to use a socket and bind it to claim the port was just for
demostrating the idea. The correct solution, IMO, is to enhance the
core low level 4-tuple allocation services to be more generic (eg: not
be tied to a struct sock). Then the host tcp stack and the host rdma
stack can allocate TCP/iWARP ports/4tuples from this common exported
service and share the port space. This allocation service could also be
used by other deep adapters like iscsi adapters if needed.
Will you NAK such a solution if I go implement it and submit for review?
The dual ip subnet solution really sux, and I'm trying one more time
to see if you will entertain the common port space solution, if done
correctly.
Thanks,
Steve.
^ permalink raw reply
* Re: pktgen question
From: Steve Wise @ 2007-10-08 21:43 UTC (permalink / raw)
To: Ben Greear; +Cc: Rick Jones, hadi, Evgeniy Polyakov, netdev, Robert Olsson
In-Reply-To: <46F8007A.6000701@candelatech.com>
Ben Greear wrote:
> Rick Jones wrote:
>>>> Perf-wise, you could clone the skbs up front, then deliver them to
>>>> the nic in a tight loop. This would mitigate the added overhead
>>>> introduced by calling skb_clone() in the loop doing transmits...
>>>
>>> That only works if you are sending a small number of skbs. You can't
>>> pre-clone several minutes worth of 10Gbe traffic
>>> with any normal amount of RAM.
>>
>> Does pktgen really need to allocate anything more than some smallish
>> fraction more than the depth of the driver's transmit queue?
>
> If you want to send sustained high rates of traffic, for more than
> just a trivial amount of time, then you either have to play the current
> trick with the skb_get(), or you have to allocate a real packet each time
> (maybe with skb_clone() or similar, but it's still more overhead than
> the skb_get
> which only bumps a reference count.)
>
> I see no other way, but if you can think of one, please let me know.
>
You can keep freed skb's that were cloned on a free list, then reuse
them once freed. You can detect when the driver frees them by adding a
destroy function to the skb. So what will happen is the set of cloned
skbs needed will eventually settled down to a constent amount and the
amount will be based on the latency involved in transmitting a single
skb. And it should be bounded by the max txq depth. Yes? (or am I all
wet :)
So you would pay the overhead of cloning only until you hit this steady
state.
Whatchathink?
> Thanks,
> Ben
>
^ permalink raw reply
* Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.
From: David Miller @ 2007-10-08 21:28 UTC (permalink / raw)
To: shemminger; +Cc: mchan, eliezert, jeff, netdev, eilong, vladz, gertner
In-Reply-To: <20071008140301.3e69d01d@freepuppy.rosehill>
From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Mon, 8 Oct 2007 14:03:01 -0700
> On Mon, 08 Oct 2007 14:40:22 -0700
> "Michael Chan" <mchan@broadcom.com> wrote:
>
> > On Mon, 2007-10-08 at 12:08 -0700, Stephen Hemminger wrote:
> > > On Mon, 08 Oct 2007 20:34:41 +0200
> > > "Eliezer" <eliezert@broadcom.com> wrote:
> > >
> > > > > * The MACRO's for 64 bit stats look like they could be done with
> > > > > u64 and/or turned into inline's.
> > > >
> > > > The MACRO's modify some of their arguments, plus they need to work on 32
> > > > bit machines (are 64 bit counters always available on 32 bit machines?).
> > > > so using an inline would allow the inline to be more readable but
> > > > calling it would get ugly.
> > > > I'm open to suggestions.
> > > >
> > >
> > > u64 exists on all platforms (including 32 bit).
> > >
> >
> > I think the biggest problem with these 64-bits counters (and 64-bit
> > addresses) is that the hardware treats them as big endian and they get
> > DMA'ed in big endian format. We control the byte swap so that 32-bit
> > quantities will have the correct endianness, but the high and low 32-bit
> > words will be in the wrong spots on little endian machines. That's why
> > we need to separate the high and the low words and convert them back and
> > forth.
> >
> There are types and tools for checking endianness see be64, etc.
It's not a be64 Stephen, Michael is trying to explain this.
They configure the hardware to swap the bytes within a 32-bit word
into cpu endianness, but the chip doesn't swap the 32-bit words within
a 64-bit word properly, which is why the macros are necessary.
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: David Miller @ 2007-10-08 21:26 UTC (permalink / raw)
To: hadi
Cc: johnpol, herbert, gaagaan, Robert.Olsson, netdev, rdreier,
peter.p.waskiewicz.jr, mcarlson, randy.dunlap, jagana, general,
mchan, tgraf, jeff, sri, shemminger, kaber
In-Reply-To: <1191876530.4373.58.camel@localhost>
From: jamal <hadi@cyberus.ca>
Date: Mon, 08 Oct 2007 16:48:50 -0400
> On Mon, 2007-08-10 at 12:46 -0700, Waskiewicz Jr, Peter P wrote:
>
> > I still have concerns how this will work with Tx multiqueue.
> > The way the batching code looks right now, you will probably send a
> > batch of skb's from multiple bands from PRIO or RR to the driver. For
> > non-Tx multiqueue drivers, this is fine. For Tx multiqueue drivers,
> > this isn't fine, since the Tx ring is selected by the value of
> > skb->queue_mapping (set by the qdisc on {prio|rr}_classify()). If the
> > whole batch comes in with different queue_mappings, this could prove to
> > be an interesting issue.
>
> true, that needs some resolution. Heres a hand-waving thought:
> Assuming all packets of a specific map end up in the same qdiscn queue,
> it seems feasible to ask the qdisc scheduler to give us enough packages
> (ive seen people use that terms to refer to packets) for each hardware
> ring's available space. With the patches i posted, i do that via
> dev->xmit_win that assumes only one view of the driver; essentially a
> single ring.
> If that is doable, then it is up to the driver to say
> "i have space for 5 in ring[0], 10 in ring[1] 0 in ring[2]" based on
> what scheduling scheme the driver implements - the dev->blist can stay
> the same. Its a handwave, so there may be issues there and there could
> be better ways to handle this.
Add xmit_win to struct net_device_subqueue, problem solved.
^ permalink raw reply
* Re: Fixed PHY regression
From: David Miller @ 2007-10-08 21:20 UTC (permalink / raw)
To: joakim.tjernlund; +Cc: netdev, jeff
In-Reply-To: <008601c809d7$7f5dc710$04ac10ac@Jocke>
From: "Joakim Tjernlund" <joakim.tjernlund@transmode.se>
Date: Mon, 8 Oct 2007 20:17:33 +0200
>
> David, any chance the second item, Oops while modprobing phy fixed module, in
> http://marc.info/?l=linux-kernel&m=119178673421891&w=2
> will make it into 2.6.23?
>
> The fix is now in your tree,
> http://git.kernel.org/?p=linux/kernel/git/davem/bak-net-2.6.24.git;a=commit;h=bbb4c0c35a4c2aed5e025b668c8dfc99c5b74cff
>
> It has been in Andrews tree for a while, then in Garzik and now it is in your tree.
Jeff, any reason you didn't submit this one for 2.6.23?
^ permalink raw reply
* [ofa-general] Re: parallel networking
From: David Miller @ 2007-10-08 21:11 UTC (permalink / raw)
To: jeff
Cc: randy.dunlap, Robert.Olsson, gaagaan, kumarkr,
peter.p.waskiewicz.jr, shemminger, johnpol, herbert, rdreier,
general, mingo, sri, jagana, hadi, mchan, netdev, linux-kernel,
tgraf, kaber
In-Reply-To: <470A3D24.3050803@garzik.org>
From: Jeff Garzik <jeff@garzik.org>
Date: Mon, 08 Oct 2007 10:22:28 -0400
> In terms of overall parallelization, both for TX as well as RX, my gut
> feeling is that we want to move towards an MSI-X, multi-core friendly
> model where packets are LIKELY to be sent and received by the same set
> of [cpus | cores | packages | nodes] that the [userland] processes
> dealing with the data.
The problem is that the packet schedulers want global guarantees
on packet ordering, not flow centric ones.
That is the issue Jamal is concerned about.
The more I think about it, the more inevitable it seems that we really
might need multiple qdiscs, one for each TX queue, to pull this full
parallelization off.
But the semantics of that don't smell so nice either. If the user
attaches a new qdisc to "ethN", does it go to all the TX queues, or
what?
All of the traffic shaping technology deals with the device as a unary
object. It doesn't fit to multi-queue at all.
^ permalink raw reply
* Re: [PATCH 1/4] [NET_SCHED] explict hold dev tx lock
From: David Miller @ 2007-10-08 21:05 UTC (permalink / raw)
To: hadi
Cc: peter.p.waskiewicz.jr, krkumar2, johnpol, herbert, kaber,
shemminger, jagana, Robert.Olsson, rick.jones2, xma, gaagaan,
netdev, rdreier, mcarlson, jeff, mchan, general, kumarkr, tgraf,
randy.dunlap, sri
In-Reply-To: <1191850490.4352.41.camel@localhost>
From: jamal <hadi@cyberus.ca>
Date: Mon, 08 Oct 2007 09:34:50 -0400
> The brain-block i am having is the parallelization aspect of it.
> Whatever scheme it is - it needs to ensure the scheduler works as
> expected. For example, if it was a strict prio scheduler i would expect
> that whatever goes out is always high priority first and never ever
> allow a low prio packet out at any time theres something high prio
> needing to go out. If i have the two priorities running on two cpus,
> then i cant guarantee that effect.
> IOW, i see the scheduler/qdisc level as not being split across parallel
> cpus. Do i make any sense?
Picture it like N tubes you stick packets into, and the tubes are
processed using DRR.
So packets within a tube won't be reordered, but reordering amongst
tubes is definitely possible.
^ permalink raw reply
* Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.
From: Stephen Hemminger @ 2007-10-08 21:03 UTC (permalink / raw)
To: Michael Chan
Cc: Eliezer, David Miller, jeff@garzik.org, netdev, eilong, vladz,
gertner
In-Reply-To: <1191879622.5277.11.camel@dell>
On Mon, 08 Oct 2007 14:40:22 -0700
"Michael Chan" <mchan@broadcom.com> wrote:
> On Mon, 2007-10-08 at 12:08 -0700, Stephen Hemminger wrote:
> > On Mon, 08 Oct 2007 20:34:41 +0200
> > "Eliezer" <eliezert@broadcom.com> wrote:
> >
> > > > * The MACRO's for 64 bit stats look like they could be done with
> > > > u64 and/or turned into inline's.
> > >
> > > The MACRO's modify some of their arguments, plus they need to work on 32
> > > bit machines (are 64 bit counters always available on 32 bit machines?).
> > > so using an inline would allow the inline to be more readable but
> > > calling it would get ugly.
> > > I'm open to suggestions.
> > >
> >
> > u64 exists on all platforms (including 32 bit).
> >
>
> I think the biggest problem with these 64-bits counters (and 64-bit
> addresses) is that the hardware treats them as big endian and they get
> DMA'ed in big endian format. We control the byte swap so that 32-bit
> quantities will have the correct endianness, but the high and low 32-bit
> words will be in the wrong spots on little endian machines. That's why
> we need to separate the high and the low words and convert them back and
> forth.
>
There are types and tools for checking endianness see be64, etc.
--
Stephen Hemminger <shemminger@linux-foundation.org>
^ permalink raw reply
* RE: [PATCH 2/3][NET_BATCH] net core use batching
From: jamal @ 2007-10-08 20:48 UTC (permalink / raw)
To: Waskiewicz Jr, Peter P
Cc: David Miller, krkumar2, johnpol, herbert, kaber, shemminger,
jagana, Robert.Olsson, rick.jones2, xma, gaagaan, netdev, rdreier,
mcarlson, jeff, mchan, general, tgraf, randy.dunlap, sri
In-Reply-To: <D5C1322C3E673F459512FB59E0DDC32903BC1234@orsmsx414.amr.corp.intel.com>
On Mon, 2007-08-10 at 12:46 -0700, Waskiewicz Jr, Peter P wrote:
> I still have concerns how this will work with Tx multiqueue.
> The way the batching code looks right now, you will probably send a
> batch of skb's from multiple bands from PRIO or RR to the driver. For
> non-Tx multiqueue drivers, this is fine. For Tx multiqueue drivers,
> this isn't fine, since the Tx ring is selected by the value of
> skb->queue_mapping (set by the qdisc on {prio|rr}_classify()). If the
> whole batch comes in with different queue_mappings, this could prove to
> be an interesting issue.
true, that needs some resolution. Heres a hand-waving thought:
Assuming all packets of a specific map end up in the same qdiscn queue,
it seems feasible to ask the qdisc scheduler to give us enough packages
(ive seen people use that terms to refer to packets) for each hardware
ring's available space. With the patches i posted, i do that via
dev->xmit_win that assumes only one view of the driver; essentially a
single ring.
If that is doable, then it is up to the driver to say
"i have space for 5 in ring[0], 10 in ring[1] 0 in ring[2]" based on
what scheduling scheme the driver implements - the dev->blist can stay
the same. Its a handwave, so there may be issues there and there could
be better ways to handle this.
Note: The other issue that needs resolving that i raised earlier was in
regards to multiqueue running on multiple cpus servicing different rings
concurently.
> Now I see in the driver HOWTO you recently sent that the driver
> will be expected to loop over the list and call it's ->hard_start_xmit()
> for each skb.
It's the core that does that, not the driver; the driver continues to
use ->hard_start_xmit() (albeit modified one). The idea is not to have
many new interfaces.
> I think that should be fine for multiqueue, I just wanted
> to see if you had any thoughts on how it should work, any performance
> issues you can see (I can't think of any). Since the batching feature
> and Tx multiqueue are very new features, I'd like to make sure we can
> think of any possible issues with them coexisting before they are both
> mainline.
Isnt multiqueue mainline already?
> Looking ahead for multiqueue, I'm still working on the per-queue
> lock implementation for multiqueue, which I know will not work with
> batching as it's designed today.
The point behind batching is to reduce the cost of the locks by
amortizing across the locks. Even better if one can, they should get rid
of locks. Remind me, why do you need the per-queuemap lock? And is it
needed from the enqueuing side too? Maybe lets start there to help me
understand things?
> I'm still not sure how to handle this,
> because it really would require the batch you send to have the same
> queue_mapping in each skb, so you're grabbing the correct queue_lock.
Sure, that is doable if the driver can set a per queue_mapping xmit_win
and the qdisc can be taught to say "give me packets for queue_mapping X"
> Or, we could have the core grab all the queue locks for each
> skb->queue_mapping represented in the batch. That would block another
> batch though if it had any of those queues in it's next batch before the
> first one completed. Thoughts?
I am not understanding the desire to have locks on a per-queuemap. I
think the single queuelock we have today should suffice. If the intent
is to have concurent cpus running to each hardware ring, then this is
what i questioned earlier whether it was the right thing to do(very top
of email where i mention it as "other issue").
cheers,
jamal
^ permalink raw reply
* Re: [BNX2X RESUBMIT][PATCH 0/8] New driver for Broadcom 10Gb Ethernet, take two.
From: Michael Chan @ 2007-10-08 21:40 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Eliezer, David Miller, jeff@garzik.org, netdev, eilong, vladz,
gertner
In-Reply-To: <20071008120855.758d10e7@freepuppy.rosehill>
On Mon, 2007-10-08 at 12:08 -0700, Stephen Hemminger wrote:
> On Mon, 08 Oct 2007 20:34:41 +0200
> "Eliezer" <eliezert@broadcom.com> wrote:
>
> > > * The MACRO's for 64 bit stats look like they could be done with
> > > u64 and/or turned into inline's.
> >
> > The MACRO's modify some of their arguments, plus they need to work on 32
> > bit machines (are 64 bit counters always available on 32 bit machines?).
> > so using an inline would allow the inline to be more readable but
> > calling it would get ugly.
> > I'm open to suggestions.
> >
>
> u64 exists on all platforms (including 32 bit).
>
I think the biggest problem with these 64-bits counters (and 64-bit
addresses) is that the hardware treats them as big endian and they get
DMA'ed in big endian format. We control the byte swap so that 32-bit
quantities will have the correct endianness, but the high and low 32-bit
words will be in the wrong spots on little endian machines. That's why
we need to separate the high and the low words and convert them back and
forth.
^ permalink raw reply
* [ofa-general] [NET_BATCH] Some perf results
From: jamal @ 2007-10-08 19:53 UTC (permalink / raw)
To: David Miller
Cc: johnpol, peter.p.waskiewicz.jr, kumarkr, herbert, gaagaan,
Robert.Olsson, netdev, rdreier, mcarlson, randy.dunlap, jagana,
general, mchan, tgraf, jeff, sri, shemminger, kaber
[-- Attachment #1: Type: text/plain, Size: 824 bytes --]
Ive attached a small pdf with results. This adds on top of results I
posted yesterday (although i didnt see them reflected on netdev).
1) "batch-ntlst" is the patches posted today that remove the temporary
list in qdisc restart and is derived from this AM net-2.6.24
2) "batch-kern" is result of batching patches posted yesterday that had
the temporary list and is based on net-2.6.24 from yesterday AM
3) "net-2.6.2" is yesterday's AM net-2.6.24 with no changes,
So #1 is not a completely fair comparison with #2 and #3. However,
looking at the logs, the changes that have gone in are unrelated to the
areas i have touched, so i dont expect any effect.
Overall, removing the temporay list from qdisc_restart provides a small
improvement noticeable only at the smaller packet sizes.
In any case, enjoy.
cheers,
jamal
[-- Attachment #2: batch-res2.pdf --]
[-- Type: application/pdf, Size: 12277 bytes --]
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* RE: [PATCH 2/3][NET_BATCH] net core use batching
From: Waskiewicz Jr, Peter P @ 2007-10-08 19:46 UTC (permalink / raw)
To: hadi, David Miller
Cc: krkumar2, johnpol, herbert, kaber, shemminger, jagana,
Robert.Olsson, rick.jones2, xma, gaagaan, netdev, rdreier,
mcarlson, jeff, mchan, general, tgraf, randy.dunlap, sri
In-Reply-To: <1191868010.4335.33.camel@localhost>
> -----Original Message-----
> From: J Hadi Salim [mailto:j.hadi123@gmail.com] On Behalf Of jamal
> Sent: Monday, October 08, 2007 11:27 AM
> To: David Miller
> Cc: krkumar2@in.ibm.com; johnpol@2ka.mipt.ru;
> herbert@gondor.apana.org.au; kaber@trash.net;
> shemminger@linux-foundation.org; jagana@us.ibm.com;
> Robert.Olsson@data.slu.se; rick.jones2@hp.com;
> xma@us.ibm.com; gaagaan@gmail.com; netdev@vger.kernel.org;
> rdreier@cisco.com; Waskiewicz Jr, Peter P;
> mcarlson@broadcom.com; jeff@garzik.org; mchan@broadcom.com;
> general@lists.openfabrics.org; kumarkr@linux.ibm.com;
> tgraf@suug.ch; randy.dunlap@oracle.com; sri@us.ibm.com
> Subject: [PATCH 2/3][NET_BATCH] net core use batching
>
> This patch adds the usage of batching within the core.
>
> cheers,
> jamal
Hey Jamal,
I still have concerns how this will work with Tx multiqueue.
The way the batching code looks right now, you will probably send a
batch of skb's from multiple bands from PRIO or RR to the driver. For
non-Tx multiqueue drivers, this is fine. For Tx multiqueue drivers,
this isn't fine, since the Tx ring is selected by the value of
skb->queue_mapping (set by the qdisc on {prio|rr}_classify()). If the
whole batch comes in with different queue_mappings, this could prove to
be an interesting issue.
Now I see in the driver HOWTO you recently sent that the driver
will be expected to loop over the list and call it's ->hard_start_xmit()
for each skb. I think that should be fine for multiqueue, I just wanted
to see if you had any thoughts on how it should work, any performance
issues you can see (I can't think of any). Since the batching feature
and Tx multiqueue are very new features, I'd like to make sure we can
think of any possible issues with them coexisting before they are both
mainline.
Looking ahead for multiqueue, I'm still working on the per-queue
lock implementation for multiqueue, which I know will not work with
batching as it's designed today. I'm still not sure how to handle this,
because it really would require the batch you send to have the same
queue_mapping in each skb, so you're grabbing the correct queue_lock.
Or, we could have the core grab all the queue locks for each
skb->queue_mapping represented in the batch. That would block another
batch though if it had any of those queues in it's next batch before the
first one completed. Thoughts?
Thanks Jamal,
-PJ Waskiewicz
<peter.p.waskiewicz.jr@intel.com>
^ 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