* [PATCH 1/7] sky2: use upper/lower 32 bits
From: Stephen Hemminger @ 2009-08-19 1:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20090819011704.685802801@vyatta.com>
[-- Attachment #1: sky2-upper32.patch --]
[-- Type: text/plain, Size: 1948 bytes --]
Use the existing macros to show where DMA address is being broken
apart. This is cosmetic only.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/drivers/net/sky2.c 2009-08-18 09:30:17.292586888 -0700
+++ b/drivers/net/sky2.c 2009-08-18 09:32:29.143195273 -0700
@@ -984,12 +984,12 @@ static void sky2_qset(struct sky2_hw *hw
* hardware and driver list elements
*/
static void sky2_prefetch_init(struct sky2_hw *hw, u32 qaddr,
- u64 addr, u32 last)
+ dma_addr_t addr, u32 last)
{
sky2_write32(hw, Y2_QADDR(qaddr, PREF_UNIT_CTRL), PREF_UNIT_RST_SET);
sky2_write32(hw, Y2_QADDR(qaddr, PREF_UNIT_CTRL), PREF_UNIT_RST_CLR);
- sky2_write32(hw, Y2_QADDR(qaddr, PREF_UNIT_ADDR_HI), addr >> 32);
- sky2_write32(hw, Y2_QADDR(qaddr, PREF_UNIT_ADDR_LO), (u32) addr);
+ sky2_write32(hw, Y2_QADDR(qaddr, PREF_UNIT_ADDR_HI), upper_32_bits(addr));
+ sky2_write32(hw, Y2_QADDR(qaddr, PREF_UNIT_ADDR_LO), lower_32_bits(addr));
sky2_write16(hw, Y2_QADDR(qaddr, PREF_UNIT_LAST_IDX), last);
sky2_write32(hw, Y2_QADDR(qaddr, PREF_UNIT_CTRL), PREF_UNIT_OP_ON);
@@ -1057,7 +1057,7 @@ static void sky2_rx_add(struct sky2_port
}
le = sky2_next_rx(sky2);
- le->addr = cpu_to_le32((u32) map);
+ le->addr = cpu_to_le32(lower_32_bits(map));
le->length = cpu_to_le16(len);
le->opcode = op | HW_OWNER;
}
@@ -1662,7 +1662,7 @@ static int sky2_xmit_frame(struct sk_buf
}
le = get_tx_le(sky2, &slot);
- le->addr = cpu_to_le32((u32) mapping);
+ le->addr = cpu_to_le32(lower_32_bits(mapping));
le->length = cpu_to_le16(len);
le->ctrl = ctrl;
le->opcode = mss ? (OP_LARGESEND | HW_OWNER) : (OP_PACKET | HW_OWNER);
@@ -1689,7 +1689,7 @@ static int sky2_xmit_frame(struct sk_buf
}
le = get_tx_le(sky2, &slot);
- le->addr = cpu_to_le32((u32) mapping);
+ le->addr = cpu_to_le32(lower_32_bits(mapping));
le->length = cpu_to_le16(frag->size);
le->ctrl = ctrl;
le->opcode = OP_BUFFER | HW_OWNER;
--
^ permalink raw reply
* [PATCH 6/7] sky2: no recycling
From: Stephen Hemminger @ 2009-08-19 1:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20090819011704.685802801@vyatta.com>
[-- Attachment #1: sky2-no-recycle.patch --]
[-- Type: text/plain, Size: 2058 bytes --]
Recycling turns out to be a bad idea! For most use cases, the
packet can not be reused: TCP packets are cloned. Even for the ideal
case of forwarding, it hurts performance because of CPU ping/pong.
On a multi-core system forwarding of 64 byte packets is worse
much worse: recycling = 24% forwarded vs no recycling = 42% forwarded
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/drivers/net/sky2.c 2009-08-18 17:50:31.591887510 -0700
+++ b/drivers/net/sky2.c 2009-08-18 17:53:04.506927814 -0700
@@ -1177,7 +1177,6 @@ static void sky2_rx_clean(struct sky2_po
re->skb = NULL;
}
}
- skb_queue_purge(&sky2->rx_recycle);
}
/* Basic MII support */
@@ -1269,10 +1268,8 @@ static struct sk_buff *sky2_rx_alloc(str
struct sk_buff *skb;
int i;
- skb = __skb_dequeue(&sky2->rx_recycle);
- if (!skb)
- skb = netdev_alloc_skb(sky2->netdev, sky2->rx_data_size
- + sky2_rx_pad(sky2->hw));
+ skb = netdev_alloc_skb(sky2->netdev,
+ sky2->rx_data_size + sky2_rx_pad(sky2->hw));
if (!skb)
goto nomem;
@@ -1364,8 +1361,6 @@ static int sky2_rx_start(struct sky2_por
sky2->rx_data_size = size;
- skb_queue_head_init(&sky2->rx_recycle);
-
/* Fill Rx ring */
for (i = 0; i < sky2->rx_pending; i++) {
re = sky2->rx_ring + i;
@@ -1776,12 +1771,7 @@ static void sky2_tx_complete(struct sky2
dev->stats.tx_packets++;
dev->stats.tx_bytes += skb->len;
- if (skb_queue_len(&sky2->rx_recycle) < sky2->rx_pending
- && skb_recycle_check(skb, sky2->rx_data_size
- + sky2_rx_pad(sky2->hw)))
- __skb_queue_head(&sky2->rx_recycle, skb);
- else
- dev_kfree_skb_any(skb);
+ dev_kfree_skb_any(skb);
sky2->tx_next = RING_NEXT(idx, sky2->tx_ring_size);
}
--- a/drivers/net/sky2.h 2009-08-18 17:50:34.706665948 -0700
+++ b/drivers/net/sky2.h 2009-08-18 17:51:02.596652122 -0700
@@ -2032,7 +2032,6 @@ struct sky2_port {
u16 rx_pending;
u16 rx_data_size;
u16 rx_nfrags;
- struct sk_buff_head rx_recycle;
#ifdef SKY2_VLAN_TAG_USED
u16 rx_tag;
--
^ permalink raw reply
* [PATCH 2/7] sky2: transmit ring 64 bit conservation
From: Stephen Hemminger @ 2009-08-19 1:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20090819011704.685802801@vyatta.com>
[-- Attachment #1: sky2-64.patch --]
[-- Type: text/plain, Size: 2091 bytes --]
This patch saves elements on transmit ring by only updating the upper
64 bit address when it changes. With many workloads skb's are located
in same region, so it saves space.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/drivers/net/sky2.c 2009-08-18 09:32:29.143195273 -0700
+++ b/drivers/net/sky2.c 2009-08-18 09:33:15.490426779 -0700
@@ -1016,6 +1016,7 @@ static void tx_init(struct sky2_port *sk
le = get_tx_le(sky2, &sky2->tx_prod);
le->addr = 0;
le->opcode = OP_ADDR64 | HW_OWNER;
+ sky2->tx_last_upper = 0;
}
static inline struct tx_ring_info *tx_le_re(struct sky2_port *sky2,
@@ -1573,8 +1574,9 @@ static int sky2_xmit_frame(struct sk_buf
struct sky2_tx_le *le = NULL;
struct tx_ring_info *re;
unsigned i, len;
- u16 slot;
dma_addr_t mapping;
+ u32 upper;
+ u16 slot;
u16 mss;
u8 ctrl;
@@ -1593,9 +1595,11 @@ static int sky2_xmit_frame(struct sk_buf
dev->name, slot, skb->len);
/* Send high bits if needed */
- if (sizeof(dma_addr_t) > sizeof(u32)) {
+ upper = upper_32_bits(mapping);
+ if (upper != sky2->tx_last_upper) {
le = get_tx_le(sky2, &slot);
- le->addr = cpu_to_le32(upper_32_bits(mapping));
+ le->addr = cpu_to_le32(upper);
+ sky2->tx_last_upper = upper;
le->opcode = OP_ADDR64 | HW_OWNER;
}
@@ -1681,10 +1685,11 @@ static int sky2_xmit_frame(struct sk_buf
if (pci_dma_mapping_error(hw->pdev, mapping))
goto mapping_unwind;
- if (sizeof(dma_addr_t) > sizeof(u32)) {
+ upper = upper_32_bits(mapping);
+ if (upper != sky2->tx_last_upper) {
le = get_tx_le(sky2, &slot);
- le->addr = cpu_to_le32(upper_32_bits(mapping));
- le->ctrl = 0;
+ le->addr = cpu_to_le32(upper);
+ sky2->tx_last_upper = upper;
le->opcode = OP_ADDR64 | HW_OWNER;
}
--- a/drivers/net/sky2.h 2009-08-18 09:29:59.224176503 -0700
+++ b/drivers/net/sky2.h 2009-08-18 09:33:15.491426771 -0700
@@ -2017,6 +2017,7 @@ struct sky2_port {
u16 tx_pending;
u16 tx_last_mss;
+ u32 tx_last_upper;
u32 tx_tcpsum;
struct rx_ring_info *rx_ring ____cacheline_aligned_in_smp;
--
^ permalink raw reply
* [PATCH 4/7] sky2: dynamic size transmit ring
From: Stephen Hemminger @ 2009-08-19 1:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20090819011704.685802801@vyatta.com>
[-- Attachment #1: sky2-tx-size.patch --]
[-- Type: text/plain, Size: 6748 bytes --]
Allocate and size transmit ring based on parameters. Saves excess
space and allows configuring larger rings for testing.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/drivers/net/sky2.c 2009-08-18 09:34:30.664368374 -0700
+++ b/drivers/net/sky2.c 2009-08-18 09:34:35.094550982 -0700
@@ -64,10 +64,12 @@
#define RX_MAX_PENDING (RX_LE_SIZE/6 - 2)
#define RX_DEF_PENDING RX_MAX_PENDING
-#define TX_RING_SIZE 512
-#define TX_DEF_PENDING 128
-#define MAX_SKB_TX_LE (4 + (sizeof(dma_addr_t)/sizeof(u32))*MAX_SKB_FRAGS)
+/* This is the worst case number of transmit list elements for a single skb:
+ VLAN + TSO + CKSUM + Data + skb_frags * DMA */
+#define MAX_SKB_TX_LE (4 + (sizeof(dma_addr_t)/sizeof(u32))*MAX_SKB_FRAGS)
#define TX_MIN_PENDING (MAX_SKB_TX_LE+1)
+#define TX_MAX_PENDING 4096
+#define TX_DEF_PENDING 127
#define STATUS_RING_SIZE 2048 /* 2 ports * (TX + 2*RX) */
#define STATUS_LE_BYTES (STATUS_RING_SIZE*sizeof(struct sky2_status_le))
@@ -1000,7 +1002,7 @@ static inline struct sky2_tx_le *get_tx_
{
struct sky2_tx_le *le = sky2->tx_le + *slot;
- *slot = RING_NEXT(*slot, TX_RING_SIZE);
+ *slot = RING_NEXT(*slot, sky2->tx_ring_size);
le->ctrl = 0;
return le;
}
@@ -1433,13 +1435,13 @@ static int sky2_up(struct net_device *de
/* must be power of 2 */
sky2->tx_le = pci_alloc_consistent(hw->pdev,
- TX_RING_SIZE *
+ sky2->tx_ring_size *
sizeof(struct sky2_tx_le),
&sky2->tx_le_map);
if (!sky2->tx_le)
goto err_out;
- sky2->tx_ring = kcalloc(TX_RING_SIZE, sizeof(struct tx_ring_info),
+ sky2->tx_ring = kcalloc(sky2->tx_ring_size, sizeof(struct tx_ring_info),
GFP_KERNEL);
if (!sky2->tx_ring)
goto err_out;
@@ -1491,7 +1493,7 @@ static int sky2_up(struct net_device *de
sky2_write16(hw, Q_ADDR(txqaddr[port], Q_AL), ECU_TXFF_LEV);
sky2_prefetch_init(hw, txqaddr[port], sky2->tx_le_map,
- TX_RING_SIZE - 1);
+ sky2->tx_ring_size - 1);
#ifdef SKY2_VLAN_TAG_USED
sky2_set_vlan_mode(hw, port, sky2->vlgrp != NULL);
@@ -1520,7 +1522,7 @@ err_out:
}
if (sky2->tx_le) {
pci_free_consistent(hw->pdev,
- TX_RING_SIZE * sizeof(struct sky2_tx_le),
+ sky2->tx_ring_size * sizeof(struct sky2_tx_le),
sky2->tx_le, sky2->tx_le_map);
sky2->tx_le = NULL;
}
@@ -1533,15 +1535,15 @@ err_out:
}
/* Modular subtraction in ring */
-static inline int tx_dist(unsigned tail, unsigned head)
+static inline int tx_inuse(const struct sky2_port *sky2)
{
- return (head - tail) & (TX_RING_SIZE - 1);
+ return (sky2->tx_prod - sky2->tx_cons) & (sky2->tx_ring_size - 1);
}
/* Number of list elements available for next tx */
static inline int tx_avail(const struct sky2_port *sky2)
{
- return sky2->tx_pending - tx_dist(sky2->tx_cons, sky2->tx_prod);
+ return sky2->tx_pending - tx_inuse(sky2);
}
/* Estimate of number of transmit list elements required */
@@ -1717,7 +1719,7 @@ static int sky2_xmit_frame(struct sk_buf
return NETDEV_TX_OK;
mapping_unwind:
- for (i = sky2->tx_prod; i != slot; i = RING_NEXT(i, TX_RING_SIZE)) {
+ for (i = sky2->tx_prod; i != slot; i = RING_NEXT(i, sky2->tx_ring_size)) {
le = sky2->tx_le + i;
re = sky2->tx_ring + i;
@@ -1760,10 +1762,10 @@ static void sky2_tx_complete(struct sky2
struct pci_dev *pdev = sky2->hw->pdev;
unsigned idx;
- BUG_ON(done >= TX_RING_SIZE);
+ BUG_ON(done >= sky2->tx_ring_size);
for (idx = sky2->tx_cons; idx != done;
- idx = RING_NEXT(idx, TX_RING_SIZE)) {
+ idx = RING_NEXT(idx, sky2->tx_ring_size)) {
struct sky2_tx_le *le = sky2->tx_le + idx;
struct tx_ring_info *re = sky2->tx_ring + idx;
@@ -1799,7 +1801,7 @@ static void sky2_tx_complete(struct sky2
else
dev_kfree_skb_any(skb);
- sky2->tx_next = RING_NEXT(idx, TX_RING_SIZE);
+ sky2->tx_next = RING_NEXT(idx, sky2->tx_ring_size);
}
}
@@ -1907,7 +1909,7 @@ static int sky2_down(struct net_device *
kfree(sky2->rx_ring);
pci_free_consistent(hw->pdev,
- TX_RING_SIZE * sizeof(struct sky2_tx_le),
+ sky2->tx_ring_size * sizeof(struct sky2_tx_le),
sky2->tx_le, sky2->tx_le_map);
kfree(sky2->tx_ring);
@@ -2517,7 +2519,6 @@ static int sky2_status_intr(struct sky2_
case OP_TXINDEXLE:
/* TX index reports status for both ports */
- BUILD_BUG_ON(TX_RING_SIZE > 0x1000);
sky2_tx_done(hw->dev[0], status & 0xfff);
if (hw->dev[1])
sky2_tx_done(hw->dev[1],
@@ -3669,7 +3670,7 @@ static int sky2_set_coalesce(struct net_
ecmd->rx_coalesce_usecs_irq > tmax)
return -EINVAL;
- if (ecmd->tx_max_coalesced_frames >= TX_RING_SIZE-1)
+ if (ecmd->tx_max_coalesced_frames >= sky2->tx_ring_size-1)
return -EINVAL;
if (ecmd->rx_max_coalesced_frames > RX_MAX_PENDING)
return -EINVAL;
@@ -3713,7 +3714,7 @@ static void sky2_get_ringparam(struct ne
ering->rx_max_pending = RX_MAX_PENDING;
ering->rx_mini_max_pending = 0;
ering->rx_jumbo_max_pending = 0;
- ering->tx_max_pending = TX_RING_SIZE - 1;
+ ering->tx_max_pending = TX_MAX_PENDING;
ering->rx_pending = sky2->rx_pending;
ering->rx_mini_pending = 0;
@@ -3728,14 +3729,15 @@ static int sky2_set_ringparam(struct net
if (ering->rx_pending > RX_MAX_PENDING ||
ering->rx_pending < 8 ||
- ering->tx_pending < MAX_SKB_TX_LE ||
- ering->tx_pending > TX_RING_SIZE - 1)
+ ering->tx_pending < TX_MIN_PENDING ||
+ ering->tx_pending > TX_MAX_PENDING)
return -EINVAL;
sky2_detach(dev);
sky2->rx_pending = ering->rx_pending;
sky2->tx_pending = ering->tx_pending;
+ sky2->tx_ring_size = roundup_pow_of_two(sky2->tx_pending+1);
return sky2_reattach(dev);
}
@@ -4105,8 +4107,8 @@ static int sky2_debug_show(struct seq_fi
/* Dump contents of tx ring */
sop = 1;
- for (idx = sky2->tx_next; idx != sky2->tx_prod && idx < TX_RING_SIZE;
- idx = RING_NEXT(idx, TX_RING_SIZE)) {
+ for (idx = sky2->tx_next; idx != sky2->tx_prod && idx < sky2->tx_ring_size;
+ idx = RING_NEXT(idx, sky2->tx_ring_size)) {
const struct sky2_tx_le *le = sky2->tx_le + idx;
u32 a = le32_to_cpu(le->addr);
@@ -4315,7 +4317,9 @@ static __devinit struct net_device *sky2
sky2->wol = wol;
spin_lock_init(&sky2->phy_lock);
+
sky2->tx_pending = TX_DEF_PENDING;
+ sky2->tx_ring_size = roundup_pow_of_two(TX_DEF_PENDING+1);
sky2->rx_pending = RX_DEF_PENDING;
hw->dev[port] = dev;
--- a/drivers/net/sky2.h 2009-08-18 09:33:15.491426771 -0700
+++ b/drivers/net/sky2.h 2009-08-18 09:34:35.095551088 -0700
@@ -2011,6 +2011,7 @@ struct sky2_port {
struct tx_ring_info *tx_ring;
struct sky2_tx_le *tx_le;
+ u16 tx_ring_size;
u16 tx_cons; /* next le to check */
u16 tx_prod; /* next le to use */
u16 tx_next; /* debug only */
--
^ permalink raw reply
* [PATCH 3/7] sky2: simplify list element error
From: Stephen Hemminger @ 2009-08-19 1:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20090819011704.685802801@vyatta.com>
[-- Attachment #1: sky2-le-error.patch --]
[-- Type: text/plain, Size: 2017 bytes --]
The code for list element error (which should only happen on hardware
errors) should be cleaner and safer. Gets rid of unused ring_size
argument, which makes next patch easier.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/drivers/net/sky2.c 2009-08-18 09:33:15.490426779 -0700
+++ b/drivers/net/sky2.c 2009-08-18 09:34:30.664368374 -0700
@@ -2661,19 +2661,15 @@ static void sky2_mac_intr(struct sky2_hw
}
/* This should never happen it is a bug. */
-static void sky2_le_error(struct sky2_hw *hw, unsigned port,
- u16 q, unsigned ring_size)
+static void sky2_le_error(struct sky2_hw *hw, unsigned port, u16 q)
{
struct net_device *dev = hw->dev[port];
- struct sky2_port *sky2 = netdev_priv(dev);
- unsigned idx;
- const u64 *le = (q == Q_R1 || q == Q_R2)
- ? (u64 *) sky2->rx_le : (u64 *) sky2->tx_le;
-
- idx = sky2_read16(hw, Y2_QADDR(q, PREF_UNIT_GET_IDX));
- printk(KERN_ERR PFX "%s: descriptor error q=%#x get=%u [%llx] put=%u\n",
- dev->name, (unsigned) q, idx, (unsigned long long) le[idx],
- (unsigned) sky2_read16(hw, Y2_QADDR(q, PREF_UNIT_PUT_IDX)));
+ u16 idx = sky2_read16(hw, Y2_QADDR(q, PREF_UNIT_GET_IDX));
+
+ dev_err(&hw->pdev->dev, PFX
+ "%s: descriptor error q=%#x get=%u put=%u\n",
+ dev->name, (unsigned) q, (unsigned) idx,
+ (unsigned) sky2_read16(hw, Y2_QADDR(q, PREF_UNIT_PUT_IDX)));
sky2_write32(hw, Q_ADDR(q, Q_CSR), BMU_CLR_IRQ_CHK);
}
@@ -2759,16 +2755,16 @@ static void sky2_err_intr(struct sky2_hw
sky2_mac_intr(hw, 1);
if (status & Y2_IS_CHK_RX1)
- sky2_le_error(hw, 0, Q_R1, RX_LE_SIZE);
+ sky2_le_error(hw, 0, Q_R1);
if (status & Y2_IS_CHK_RX2)
- sky2_le_error(hw, 1, Q_R2, RX_LE_SIZE);
+ sky2_le_error(hw, 1, Q_R2);
if (status & Y2_IS_CHK_TXA1)
- sky2_le_error(hw, 0, Q_XA1, TX_RING_SIZE);
+ sky2_le_error(hw, 0, Q_XA1);
if (status & Y2_IS_CHK_TXA2)
- sky2_le_error(hw, 1, Q_XA2, TX_RING_SIZE);
+ sky2_le_error(hw, 1, Q_XA2);
}
static int sky2_poll(struct napi_struct *napi, int work_limit)
--
^ permalink raw reply
* [PATCH 5/7] sky2: optimize transmit completion
From: Stephen Hemminger @ 2009-08-19 1:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20090819011704.685802801@vyatta.com>
[-- Attachment #1: sky2-tx-re.patch --]
[-- Type: text/plain, Size: 5100 bytes --]
Don't reference the list element in hardware transmit ring on transmit
completion. The list element is updated by hardware, therefore
it causes a cache miss. Do book keeping in software structure.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/drivers/net/sky2.c 2009-08-18 10:50:35.096988776 -0700
+++ b/drivers/net/sky2.c 2009-08-18 11:27:05.272209365 -0700
@@ -1001,8 +1001,11 @@ static void sky2_prefetch_init(struct sk
static inline struct sky2_tx_le *get_tx_le(struct sky2_port *sky2, u16 *slot)
{
struct sky2_tx_le *le = sky2->tx_le + *slot;
+ struct tx_ring_info *re = sky2->tx_ring + *slot;
*slot = RING_NEXT(*slot, sky2->tx_ring_size);
+ re->flags = 0;
+ re->skb = NULL;
le->ctrl = 0;
return le;
}
@@ -1021,12 +1024,6 @@ static void tx_init(struct sky2_port *sk
sky2->tx_last_upper = 0;
}
-static inline struct tx_ring_info *tx_le_re(struct sky2_port *sky2,
- struct sky2_tx_le *le)
-{
- return sky2->tx_ring + (le - sky2->tx_le);
-}
-
/* Update chip's next pointer */
static inline void sky2_put_idx(struct sky2_hw *hw, unsigned q, u16 idx)
{
@@ -1563,6 +1560,19 @@ static unsigned tx_le_req(const struct s
return count;
}
+static void sky2_tx_unmap(struct pci_dev *pdev,
+ const struct tx_ring_info *re)
+{
+ if (re->flags & TX_MAP_SINGLE)
+ pci_unmap_single(pdev, pci_unmap_addr(re, mapaddr),
+ pci_unmap_len(re, maplen),
+ PCI_DMA_TODEVICE);
+ else if (re->flags & TX_MAP_PAGE)
+ pci_unmap_page(pdev, pci_unmap_addr(re, mapaddr),
+ pci_unmap_len(re, maplen),
+ PCI_DMA_TODEVICE);
+}
+
/*
* Put one packet in ring for transmit.
* A single packet can generate multiple list elements, and
@@ -1667,16 +1677,17 @@ static int sky2_xmit_frame(struct sk_buf
}
}
+ re = sky2->tx_ring + slot;
+ re->flags = TX_MAP_SINGLE;
+ pci_unmap_addr_set(re, mapaddr, mapping);
+ pci_unmap_len_set(re, maplen, len);
+
le = get_tx_le(sky2, &slot);
le->addr = cpu_to_le32(lower_32_bits(mapping));
le->length = cpu_to_le16(len);
le->ctrl = ctrl;
le->opcode = mss ? (OP_LARGESEND | HW_OWNER) : (OP_PACKET | HW_OWNER);
- re = tx_le_re(sky2, le);
- re->skb = skb;
- pci_unmap_addr_set(re, mapaddr, mapping);
- pci_unmap_len_set(re, maplen, len);
for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
@@ -1695,18 +1706,19 @@ static int sky2_xmit_frame(struct sk_buf
le->opcode = OP_ADDR64 | HW_OWNER;
}
+ re = sky2->tx_ring + slot;
+ re->flags = TX_MAP_PAGE;
+ pci_unmap_addr_set(re, mapaddr, mapping);
+ pci_unmap_len_set(re, maplen, frag->size);
+
le = get_tx_le(sky2, &slot);
le->addr = cpu_to_le32(lower_32_bits(mapping));
le->length = cpu_to_le16(frag->size);
le->ctrl = ctrl;
le->opcode = OP_BUFFER | HW_OWNER;
-
- re = tx_le_re(sky2, le);
- re->skb = skb;
- pci_unmap_addr_set(re, mapaddr, mapping);
- pci_unmap_len_set(re, maplen, frag->size);
}
+ re->skb = skb;
le->ctrl |= EOP;
sky2->tx_prod = slot;
@@ -1720,23 +1732,9 @@ static int sky2_xmit_frame(struct sk_buf
mapping_unwind:
for (i = sky2->tx_prod; i != slot; i = RING_NEXT(i, sky2->tx_ring_size)) {
- le = sky2->tx_le + i;
re = sky2->tx_ring + i;
- switch(le->opcode & ~HW_OWNER) {
- case OP_LARGESEND:
- case OP_PACKET:
- pci_unmap_single(hw->pdev,
- pci_unmap_addr(re, mapaddr),
- pci_unmap_len(re, maplen),
- PCI_DMA_TODEVICE);
- break;
- case OP_BUFFER:
- pci_unmap_page(hw->pdev, pci_unmap_addr(re, mapaddr),
- pci_unmap_len(re, maplen),
- PCI_DMA_TODEVICE);
- break;
- }
+ sky2_tx_unmap(hw->pdev, re);
}
mapping_error:
@@ -1759,34 +1757,18 @@ mapping_error:
static void sky2_tx_complete(struct sky2_port *sky2, u16 done)
{
struct net_device *dev = sky2->netdev;
- struct pci_dev *pdev = sky2->hw->pdev;
unsigned idx;
BUG_ON(done >= sky2->tx_ring_size);
for (idx = sky2->tx_cons; idx != done;
idx = RING_NEXT(idx, sky2->tx_ring_size)) {
- struct sky2_tx_le *le = sky2->tx_le + idx;
struct tx_ring_info *re = sky2->tx_ring + idx;
+ struct sk_buff *skb = re->skb;
- switch(le->opcode & ~HW_OWNER) {
- case OP_LARGESEND:
- case OP_PACKET:
- pci_unmap_single(pdev,
- pci_unmap_addr(re, mapaddr),
- pci_unmap_len(re, maplen),
- PCI_DMA_TODEVICE);
- break;
- case OP_BUFFER:
- pci_unmap_page(pdev, pci_unmap_addr(re, mapaddr),
- pci_unmap_len(re, maplen),
- PCI_DMA_TODEVICE);
- break;
- }
-
- if (le->ctrl & EOP) {
- struct sk_buff *skb = re->skb;
+ sky2_tx_unmap(sky2->hw->pdev, re);
+ if (skb) {
if (unlikely(netif_msg_tx_done(sky2)))
printk(KERN_DEBUG "%s: tx done %u\n",
dev->name, idx);
--- a/drivers/net/sky2.h 2009-08-18 10:50:35.110991954 -0700
+++ b/drivers/net/sky2.h 2009-08-18 11:09:06.181966221 -0700
@@ -1984,6 +1984,9 @@ struct sky2_status_le {
struct tx_ring_info {
struct sk_buff *skb;
+ unsigned long flags;
+#define TX_MAP_SINGLE 0x0001
+#define TX_MAP_PAGE 000002
DECLARE_PCI_UNMAP_ADDR(mapaddr);
DECLARE_PCI_UNMAP_LEN(maplen);
};
--
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Ira W. Snyder @ 2009-08-19 0:44 UTC (permalink / raw)
To: Avi Kivity
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <4A8B25F5.7050806@redhat.com>
On Wed, Aug 19, 2009 at 01:06:45AM +0300, Avi Kivity wrote:
> On 08/19/2009 12:26 AM, Avi Kivity wrote:
>>>
>>> Off the top of my head, I would think that transporting userspace
>>> addresses in the ring (for copy_(to|from)_user()) vs. physical addresses
>>> (for DMAEngine) might be a problem. Pinning userspace pages into memory
>>> for DMA is a bit of a pain, though it is possible.
>>
>>
>> Oh, the ring doesn't transport userspace addresses. It transports
>> guest addresses, and it's up to vhost to do something with them.
>>
>> Currently vhost supports two translation modes:
>>
>> 1. virtio address == host virtual address (using copy_to_user)
>> 2. virtio address == offsetted host virtual address (using copy_to_user)
>>
>> The latter mode is used for kvm guests (with multiple offsets,
>> skipping some details).
>>
>> I think you need to add a third mode, virtio address == host physical
>> address (using dma engine). Once you do that, and wire up the
>> signalling, things should work.
>
>
> You don't need in fact a third mode. You can mmap the x86 address space
> into your ppc userspace and use the second mode. All you need then is
> the dma engine glue and byte swapping.
>
Hmm, I'll have to think about that.
The ppc is a 32-bit processor, so it has 4GB of address space for
everything, including PCI, SDRAM, flash memory, and all other
peripherals.
This is exactly like 32bit x86, where you cannot have a PCI card that
exposes a 4GB PCI BAR. The system would have no address space left for
its own SDRAM.
On my x86 computers, I only have 1GB of physical RAM, and so the ppc's
have plenty of room in their address spaces to map the entire x86 RAM
into their own address space. That is exactly what I do now. Accesses to
ppc physical address 0x80000000 "magically" hit x86 physical address
0x0.
Ira
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Ira W. Snyder @ 2009-08-19 0:38 UTC (permalink / raw)
To: Avi Kivity
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <4A8B1C7F.4060008@redhat.com>
On Wed, Aug 19, 2009 at 12:26:23AM +0300, Avi Kivity wrote:
> On 08/18/2009 11:59 PM, Ira W. Snyder wrote:
>> On a non shared-memory system (where the guest's RAM is not just a chunk
>> of userspace RAM in the host system), virtio's management model seems to
>> fall apart. Feature negotiation doesn't work as one would expect.
>>
>
> In your case, virtio-net on the main board accesses PCI config space
> registers to perform the feature negotiation; software on your PCI cards
> needs to trap these config space accesses and respond to them according
> to virtio ABI.
>
Is this "real PCI" (physical hardware) or "fake PCI" (software PCI
emulation) that you are describing?
The host (x86, PCI master) must use "real PCI" to actually configure the
boards, enable bus mastering, etc. Just like any other PCI device, such
as a network card.
On the guests (ppc, PCI agents) I cannot add/change PCI functions (the
last .[0-9] in the PCI address) nor can I change PCI BAR's once the
board has started. I'm pretty sure that would violate the PCI spec,
since the PCI master would need to re-scan the bus, and re-assign
addresses, which is a task for the BIOS.
> (There's no real guest on your setup, right? just a kernel running on
> and x86 system and other kernels running on the PCI cards?)
>
Yes, the x86 (PCI master) runs Linux (booted via PXELinux). The ppc's
(PCI agents) also run Linux (booted via U-Boot). They are independent
Linux systems, with a physical PCI interconnect.
The x86 has CONFIG_PCI=y, however the ppc's have CONFIG_PCI=n. Linux's
PCI stack does bad things as a PCI agent. It always assumes it is a PCI
master.
It is possible for me to enable CONFIG_PCI=y on the ppc's by removing
the PCI bus from their list of devices provided by OpenFirmware. They
can not access PCI via normal methods. PCI drivers cannot work on the
ppc's, because Linux assumes it is a PCI master.
To the best of my knowledge, I cannot trap configuration space accesses
on the PCI agents. I haven't needed that for anything I've done thus
far.
>> This does appear to be solved by vbus, though I haven't written a
>> vbus-over-PCI implementation, so I cannot be completely sure.
>>
>
> Even if virtio-pci doesn't work out for some reason (though it should),
> you can write your own virtio transport and implement its config space
> however you like.
>
This is what I did with virtio-over-PCI. The way virtio-net negotiates
features makes this work non-intuitively.
>> I'm not at all clear on how to get feature negotiation to work on a
>> system like mine. From my study of lguest and kvm (see below) it looks
>> like userspace will need to be involved, via a miscdevice.
>>
>
> I don't see why. Is the kernel on the PCI cards in full control of all
> accesses?
>
I'm not sure what you mean by this. Could you be more specific? This is
a normal, unmodified vanilla Linux kernel running on the PCI agents.
>> Ok. I thought I should at least express my concerns while we're
>> discussing this, rather than being too late after finding the time to
>> study the driver.
>>
>> Off the top of my head, I would think that transporting userspace
>> addresses in the ring (for copy_(to|from)_user()) vs. physical addresses
>> (for DMAEngine) might be a problem. Pinning userspace pages into memory
>> for DMA is a bit of a pain, though it is possible.
>>
>
> Oh, the ring doesn't transport userspace addresses. It transports guest
> addresses, and it's up to vhost to do something with them.
>
> Currently vhost supports two translation modes:
>
> 1. virtio address == host virtual address (using copy_to_user)
> 2. virtio address == offsetted host virtual address (using copy_to_user)
>
> The latter mode is used for kvm guests (with multiple offsets, skipping
> some details).
>
> I think you need to add a third mode, virtio address == host physical
> address (using dma engine). Once you do that, and wire up the
> signalling, things should work.
>
Ok.
In my virtio-over-PCI patch, I hooked two virtio-net's together. I wrote
an algorithm to pair the tx/rx queues together. Since virtio-net
pre-fills its rx queues with buffers, I was able to use the DMA engine
to copy from the tx queue into the pre-allocated memory in the rx queue.
I have an intuitive idea about how I think vhost-net works in this case.
>> There is also the problem of different endianness between host and guest
>> in virtio-net. The struct virtio_net_hdr (include/linux/virtio_net.h)
>> defines fields in host byte order. Which totally breaks if the guest has
>> a different endianness. This is a virtio-net problem though, and is not
>> transport specific.
>>
>
> Yeah. You'll need to add byteswaps.
>
I wonder if Rusty would accept a new feature:
VIRTIO_F_NET_LITTLE_ENDIAN, which would allow the virtio-net driver to
use LE for all of it's multi-byte fields.
I don't think the transport should have to care about the endianness.
>> I've browsed over both the kvm and lguest code, and it looks like they
>> each re-invent a mechanism for transporting interrupts between the host
>> and guest, using eventfd. They both do this by implementing a
>> miscdevice, which is basically their management interface.
>>
>> See drivers/lguest/lguest_user.c (see write() and LHREQ_EVENTFD) and
>> kvm-kmod-devel-88/x86/kvm_main.c (see kvm_vm_ioctl(), called via
>> kvm_dev_ioctl()) for how they hook up eventfd's.
>>
>> I can now imagine how two userspace programs (host and guest) could work
>> together to implement a management interface, including hotplug of
>> devices, etc. Of course, this would basically reinvent the vbus
>> management interface into a specific driver.
>>
>
> You don't need anything in the guest userspace (virtio-net) side.
>
>> I think this is partly what Greg is trying to abstract out into generic
>> code. I haven't studied the actual data transport mechanisms in vbus,
>> though I have studied virtio's transport mechanism. I think a generic
>> management interface for virtio might be a good thing to consider,
>> because it seems there are at least two implementations already: kvm and
>> lguest.
>>
>
> Management code in the kernel doesn't really help unless you plan to
> manage things with echo and cat.
>
True. It's slowpath setup, so I don't care how fast it is. For reasons
outside my control, the x86 (PCI master) is running a RHEL5 system. This
means glibc-2.5, which doesn't have eventfd support, AFAIK. I could try
and push for an upgrade. This obviously makes cat/echo really nice, it
doesn't depend on glibc, only the kernel version.
I don't give much weight to the above, because I can use the eventfd
syscalls directly, without glibc support. It is just more painful.
Ira
^ permalink raw reply
* Re: [PATCH] revert TCP retransmission backoff on ICMP destination unreachable
From: Damian Lukowski @ 2009-08-18 23:56 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Netdev, David Miller
In-Reply-To: <Pine.LNX.4.64.0908190012400.13065@melkinkari.cs.Helsinki.FI>
> On Tue, 18 Aug 2009, Damian Lukowski wrote:
>
>>> On Fri, 14 Aug 2009, Damian Lukowski wrote:
>>>
>>>>> Longer than 80 columns, and use an inline function instead
>>>>> of a macro in order to get proper type checking.
>>>>> [...]
>>>>> Do not break up the function local variables with spurious new lines
>>>>> like this, please.
>>>>> [...]
>>>>> The indentation and tabbing is messed up in all of the code you are
>>>>> adding, please fix it up to be consistent with the surrounding code
>>>>> and the rest of the TCP stack.
>>>>>
>>>>> Do not use C++ style // comments.
>>>> Better?
>>> Please, include the changelog message on resubmits too next time.
>> I'm sorry, which message do you mean? I used plain diff without GIT or
>> anything.
>
> In the original mail you had some description about the patch, I was
> thinking of quoting something out of it but it was missing so I didn't
> bother copying. But more importantly, would Dave Miller want to apply the
> patch of yours, he would need that description and it saves work for him
> to have the description in every resubmit "duplicated" (one usually just
> gets a boomerang mail asking for the description which is similarly
> wasting his time).
Ok, I will fully quote previous messages now. Just wanted to avoid
redundancy. Anyway, as long as the draft is not accepted as RFC, the
patch will violate current standards, so I doubt, Dave would like to
apply it soon. :)
>
>>>> + icsk->icsk_backoff--;
>>>> + icsk->icsk_rto >>= 1;
>>> Are you absolute sure that we don't go to zero here when phase of the
>>> moon is just right? I'd put a max(..., 1) guard there.
>> Well, the retransmission timer doubles icsk_rto whenever it increases
>> icsk_backoff, so we should reach the original icsk_rto, when
>> icsk_backoff becomes zero.
>
> This is a false assumption. Take a look into the formula, there's a
> cap how far icsk_rto goes but icsk_backoff keeps increasing. In fact,
> there would probably be a need to set some other lower bound than 1,
> TCP_RTO_MIN would probably be more appropriate for this.
*sigh* I should have seen that. I would like to preserve the initial rto
value and not work with RTO_MIN. First, I have thought about storing the
initial rto in another state variable. But what, if we simply
recalculate the value?:
###
icsk->icsk_backoff--;
- icsk->icsk_rto >>= 1;
+ tcp_set_rto(sk);
+ icsk->icsk_rto <<= icsk->icsk_backoff;
+ tcp_bound_rto(sk);
###
>>>> + BUG_ON(!skb_r);
>>>> +
>>>> + if (sock_owned_by_user(sk)) {
>>>> + /* Deferring retransmission clocked by ICMP
>>>> + * due to locked socket. */
>>>> + inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
>>>> + min(icsk->icsk_rto, TCP_RESOURCE_PROBE_INTERVAL),
>>> ??? (besides having indent wrong). What makes you think HZ/2 is right
>>> value if icsk_rto is large?
>> To be honest, I have taken over the code from my predecessor and didn't
>> question this part.
>
> But you did Signoff that regardless, so you should know ;-).
Only because checkpatch.pl didn't like it otherwise. :)
>
>> When thinking about it, I would remove this block
>> completely. Will bad things happen, if I mess with the timer, when the
>> socket is locked?
>
> I've no idea without looking into all the details but I think this needs
> to be moved slightly later anyway into the imminent expiry branch and the
> timeout value needs to be copied from tcp_write_timer instead of using
> that bogus formula.
>
>>>> + TCP_RTO_MAX);
>>> Perhaps you lack here something to exit?
>>>
>>>> + }
>>>> +
>>>> + if (tcp_time_stamp - TCP_SKB_CB(skb_r)->when >
>>>> + inet_csk(sk)->icsk_rto) {
>>> icsk->icsk_rto !?!
>>>
>>>> + /* RTO revert clocked out retransmission. */
>>>> + tcp_retransmit_skb(sk, skb_r);
>>> This is plain wrong, tcp_sock counters will get messed up if
>>> TCPCB_SACKED_RETRANS is already set while calling tcp_retransmit_skb. As
>>> a sidenote, it would be probably useful to move the check + clear of
>>> that bit before doing the retransmission to some common place one day.
>> What, if I call tcp_retransmit_timer() manually instead? Will there
>> still be problems with SACK?
>
> SACK is fine then, and I took a quick look into the other stuff which
> seemed to be quite ok too but it was just a quick glance, but then you
> certainly must delay the RTO if sock_owned_by_user(sk) is true like
> tcp_write_timer does. Tcp_retransmit_timer would also deal with the
> missing step 5.5 btw. I'm not sure if you like all the things that will
> happen in the tcp_enter_loss though, especially the undo_marker clearing
> might not please you, or is at best, sub-optimal in a certain scenario.
>
>>>> + inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
>>>> + icsk->icsk_rto, TCP_RTO_MAX);
>>> RFC 2988, step 5.5 missing? Have you verified this patch for real?
>> Thats the whole point of the draft. If consecutive retransmissions
>> trigger fitting ICMPs, the RTO value is not doubled, but remains
>> constant.
>
> No, either the draft is wrong or the implementation. 5.5 step should be
> applied. Now you end up decreasing RTO per icmp which causes imminent
> retransmission instead of keeping it "constant". Effectively, step 4, 5, 6
> (halves like you do), 7 -> R, R is 5.4-5.6 and 5.5 increases RTO again
> because the timer _expired_ here even though we did short-circuit it
> instead of setting a zero timer and using the normal procedure (which in
> itself is ok approach, no need to force timer to expire).
>
>> So you can have a retransmission schedule of
>> t, t+r, t+2r, t+3r, t+4r, t+5r ...
>> instead of
>> t, t+r, t+3r, t+7r, t+15r, t+31r ...
>
> Sure, but that's not what happens, instead you will get this series:
>
> t, t+r, (t+r)/2, (t+r)/4, ...
>
> ...disclaimer: it might be off-by-something somewhere but I hope you get
> my point anyway.
It seems to me, that we focus on different things. From the TCP code's
point of view, RFC2988 step 5.5 is of course executed, because timeouts
still trigger tcp_retransmit_timer(), which doubles the RTO at the end.
But from the network's point of view, there is no doubling, but constant
intervals between retransmissions.
The following happens with patched TCP (not considering RTO_MAX):
t: Connectivity breaks, rto is r
t+r: First RTO fires, SND.UNA is retransmitted by
tcp_retransmit_timer() and rto := 2rto
Now it is rto == 2r
t+r+epsilon: ICMP unreach arrives from a router.
The patch sets rto := rto/2
Now it is rto == r
t+2r: Second RTO fires, SND.UNA is retransmitted by
tcp_retransmit_timer() and rto := 2rto
Now it is rto == 2r
Assume, that this retransmission did not trigger
any ICMP message.
t+4r: Third RTO fires, SND.UNA is retransmitted by
tcp_retransmit_timer() and rto := 2rto
Now it is rto == 4r
Assume, that this retransmission did not trigger
any ICMP message.
t+8r: Fourth RTO fires, SND.UNA is retransmitted by
tcp_retransmit_timer() and rto := 2rto
Now it is rto == 8r
t+8r+epsilon: ICMP unreach arrives from a router.
The patch sets rto := rto/2
Now it is rto == 4r
t+12r: Fifth RTO fires ...
>> I can show you trace plots of patched an unpatched TCP, if you like.
>> Should I attach files in the mails, or better post hyperlinks to them?
>
> Sure, but they must not be from a trivial case but the rto must
> "flunctuate"
How should they fluctuate? RTO is doubled by tcp_retransmit_timer() no
matter what happens, and it is halved by the the patch, if an
appropriate ICMP packet has been received.
> and the expirys must be using the "imminent" branch for it
> to count for me as a proof that you did it right.
I'm not sure what you mean.
Still, hyperlinks or attachments?
>
>>> How about:
>>>
>>> u32 remaining;
>>>
>>> remaining = icsk->icsk_rto - min(icsk->icsk_rto,
>>> tcp_time_stamp - TCP_SKB_CB(skb_r)->when));
>>> if (!remaining) {
>>> tcp_retransmit_skb(...);
>>> remaining = icsk->icsk_rto;
>>> }
>>> inet_csk_reset_xmit_timer(..., remaining);
>> Seems to be ok, but isn't tcp_retransmit_timer(...) better?
>
> Yeah, that's mostly fine I guess (at least in meaning of "safe").
>
>>>> - if (icsk->icsk_retransmits >= sysctl_tcp_retries1) {
>>>> + if (retrans_overstepped(sk, sysctl_tcp_retries1)) {
>>> ??? Could you justify these changes and explain their relation to the
>>> draft and icmp messages? If not, please drop them and try with proper
>>> description and description for them in a separate patch. I fail to see
>>> what you're trying to achieve here. You just ended up redefining the
>>> sysctl meaning too I think...
>> Well, you're quite right. RFCs specify timeouts in dimensions of time
>> (seconds, minutes, etc.), but Linux specifies timeouts in form of
>> numbers of retransmissions. One can do this, as long as the
>> retransmission schedule (exponential backoff in RFC case) is known. But
>> with this patch, the schedule can be completely different. In the
>> "worst" case, the time intervals between consecutive retransmission
>> probes are constant, leading to a TCP timeout after few seconds, instead
>> of several minutes.
>> What I did here, is to calculate the TCP timeout as if the schedule were
>> still an exponential backoff, given the allowed number of
>> retransmissions. It is possible, that TCP will now retransmit many more
>> times than tcp_retries indicates, but the duration until the TCP
>> connection times out, is more or less equivalent to unpatched TCP with
>> same tcp_retries values.
>> So, whenever some control block uses count values, but means time
>> values, I have to replace the control block appropriately; and that's
>> what retrans_overstepped() is supposed to do.
>
> I can understand your point, however, this is independent thing which
> should not be mixed with the other part of the change. So in future
> provide them as a two patch series. ...And expect some to not like this
> kind of change.
Ok.
^ permalink raw reply
* Re: pull request: wireless-2.6 2009-08-18
From: David Miller @ 2009-08-18 23:29 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20090818151436.GA2768@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Tue, 18 Aug 2009 11:14:37 -0400
> Dave,
>
> Two more for 2.6.31...
>
> The one from Johannes is actually already in net-next-2.6, but it has
> since been reported to cause a possible irq lock inversion dependency in
> 2.6.31-rc6, as described here:
>
> http://marc.info/?l=linux-wireless&m=125052198303537&w=2
>
> The one from me fixes a minor bounds checking problem in orinoco, as
> reported by Dan Carpenter. It is simple and obvious.
Pulled, thanks John.
^ permalink raw reply
* (unknown)
From: BISHOP @ 2009-08-18 23:27 UTC (permalink / raw)
DO YOU NEED A LOAN CONTACT MR BISHOP WILSON FOR A LOAN. AT bishoploancompany@gmail.com
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Ira W. Snyder @ 2009-08-18 23:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Gregory Haskins, kvm, netdev, linux-kernel, alacrityvm-devel,
Avi Kivity, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <20090818205748.GC20393@redhat.com>
On Tue, Aug 18, 2009 at 11:57:48PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 18, 2009 at 08:53:29AM -0700, Ira W. Snyder wrote:
> > I think Greg is referring to something like my virtio-over-PCI patch.
> > I'm pretty sure that vhost is completely useless for my situation. I'd
> > like to see vhost work for my use, so I'll try to explain what I'm
> > doing.
> >
> > I've got a system where I have about 20 computers connected via PCI. The
> > PCI master is a normal x86 system, and the PCI agents are PowerPC
> > systems. The PCI agents act just like any other PCI card, except they
> > are running Linux, and have their own RAM and peripherals.
> >
> > I wrote a custom driver which imitated a network interface and a serial
> > port. I tried to push it towards mainline, and DavidM rejected it, with
> > the argument, "use virtio, don't add another virtualization layer to the
> > kernel." I think he has a decent argument, so I wrote virtio-over-PCI.
> >
> > Now, there are some things about virtio that don't work over PCI.
> > Mainly, memory is not truly shared. It is extremely slow to access
> > memory that is "far away", meaning "across the PCI bus." This can be
> > worked around by using a DMA controller to transfer all data, along with
> > an intelligent scheme to perform only writes across the bus. If you're
> > careful, reads are never needed.
> >
> > So, in my system, copy_(to|from)_user() is completely wrong.
> > There is no userspace, only a physical system.
>
> Can guests do DMA to random host memory? Or is there some kind of IOMMU
> and DMA API involved? If the later, then note that you'll still need
> some kind of driver for your device. The question we need to ask
> ourselves then is whether this driver can reuse bits from vhost.
>
Mostly. All of my systems are 32 bit (both x86 and ppc). From the view
of the ppc (and DMAEngine), I can view the first 1GB of host memory.
This limited view is due to address space limitations on the ppc. The
view of PCI memory must live somewhere in the ppc address space, along
with the ppc's SDRAM, flash, and other peripherals. Since this is a
32bit processor, I only have 4GB of address space to work with.
The PCI address space could be up to 4GB in size. If I tried to allow
the ppc boards to view all 4GB of PCI address space, then they would
have no address space left for their onboard SDRAM, etc.
Hopefully that makes sense.
I use dma_set_mask(dev, DMA_BIT_MASK(30) on the host system to ensure
that when dma_map_sg() is called, it returns addresses that can be
accessed directly by the device.
The DMAEngine can access any local (ppc) memory without any restriction.
I have used the Linux DMAEngine API (include/linux/dmaengine.h) to
handle all data transfer across the PCI bus. The Intel I/OAT (and many
others) use the same API.
> > In fact, because normal x86 computers
> > do not have DMA controllers, the host system doesn't actually handle any
> > data transfer!
>
> Is it true that PPC has to initiate all DMA then? How do you
> manage not to do DMA reads then?
>
Yes, the ppc initiates all DMA. It handles all data transfer (both reads
and writes) across the PCI bus, for speed reasons. A CPU cannot create
burst transactions on the PCI bus. This is the reason that most (all?)
network cards (as a familiar example) use DMA to transfer packet
contents into RAM.
Sorry if I made a confusing statement ("no reads are necessary")
earlier. What I meant to say was: If you are very careful, it is not
necessary for the CPU to do any reads over the PCI bus to maintain
state. Writes are the only necessary CPU-initiated transaction.
I implemented this in my virtio-over-PCI patch, copying as much as
possible from the virtio vring structure. The descriptors in the rings
are only changed by one "side" of the connection, therefore they can be
cached as they are written (via the CPU) across the PCI bus, with the
knowledge that both sides will have a consistent view.
I'm sorry, this is hard to explain via email. It is much easier in a
room with a whiteboard. :)
> > I used virtio-net in both the guest and host systems in my example
> > virtio-over-PCI patch, and succeeded in getting them to communicate.
> > However, the lack of any setup interface means that the devices must be
> > hardcoded into both drivers, when the decision could be up to userspace.
> > I think this is a problem that vbus could solve.
>
> What you describe (passing setup from host to guest) seems like
> a feature that guest devices need to support. It seems unlikely that
> vbus, being a transport layer, can address this.
>
I think I explained this poorly as well.
Virtio needs two things to function:
1) a set of descriptor rings (1 or more)
2) a way to kick each ring.
With the amount of space available in the ppc's PCI BAR's (which point
at a small chunk of SDRAM), I could potentially make ~6 virtqueues + 6
kick interrupts available.
Right now, my virtio-over-PCI driver hardcoded the first and second
virtqueues to be for virtio-net only, and nothing else.
What if the user wanted 2 virtio-console and 2 virtio-net? They'd have
to change the driver, because virtio doesn't have much of a management
interface. Vbus does have a management interface: you create devices via
configfs. The vbus-connector on the guest notices new devices, and
triggers hotplug events on the guest.
As far as I understand it, vbus is a bus model, not just a transport
layer.
> >
> > For my own selfish reasons (I don't want to maintain an out-of-tree
> > driver) I'd like to see *something* useful in mainline Linux. I'm happy
> > to answer questions about my setup, just ask.
> >
> > Ira
>
> Thanks Ira, I'll think about it.
> A couple of questions:
> - Could you please describe what kind of communication needs to happen?
> - I'm not familiar with DMA engine in question. I'm guessing it's the
> usual thing: in/out buffers need to be kernel memory, interface is
> asynchronous, small limited number of outstanding requests? Is there a
> userspace interface for it and if yes how does it work?
>
The DMA engine can handle transferring from any two physical addresses,
as seen from the ppc address map. The things of interest are:
1) ppc sdram
2) host sdram (first 1GB only, explained above)
The Linux DMAEngine API allows you to do sync or async requests with
callbacks, and an unlimited number of outstanding requests (until you
exhaust memory).
The interface is in-kernel only. See include/linux/dmaengine.h for the
details, but the most important part is dma_async_memcpy_buf_to_buf(),
which will copy between two kernel virtual addresses.
It is trivial to code up an implementation which will transfer between
physical addresses instead, which I found much more convenient in my
code. I'm happy to provide the function if/when needed.
Ira
^ permalink raw reply
* [PATCH -next] net: fix ks8851 build errors
From: Randy Dunlap @ 2009-08-18 22:27 UTC (permalink / raw)
To: Stephen Rothwell, netdev, akpm; +Cc: linux-next, LKML, davem
In-Reply-To: <20090818184215.fdf1c7ae.sfr@canb.auug.org.au>
From: Randy Dunlap <randy.dunlap@oracle.com>
Fix build errors due to missing Kconfig selects:
ks8851.c:(.text+0x7d2ee): undefined reference to `crc32_le'
ks8851.c:(.text+0x7d2f5): undefined reference to `bitrev32'
Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
drivers/net/Kconfig | 2 ++
1 file changed, 2 insertions(+)
--- linux-next-20090818.orig/drivers/net/Kconfig
+++ linux-next-20090818/drivers/net/Kconfig
@@ -1733,6 +1733,8 @@ config KS8851
tristate "Micrel KS8851 SPI"
depends on SPI
select MII
+ select CRC32
+ select BITREVERSE
help
SPI driver for Micrel KS8851 SPI attached network chip.
---
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/
^ permalink raw reply
* Re: [PATCH] revert TCP retransmission backoff on ICMP destination unreachable
From: Ilpo Järvinen @ 2009-08-18 22:07 UTC (permalink / raw)
To: Damian Lukowski; +Cc: David Miller, Netdev
In-Reply-To: <4A8AE771.1070308@tvk.rwth-aachen.de>
On Tue, 18 Aug 2009, Damian Lukowski wrote:
> > On Fri, 14 Aug 2009, Damian Lukowski wrote:
> >
> >>> Longer than 80 columns, and use an inline function instead
> >>> of a macro in order to get proper type checking.
> >>> [...]
> >>> Do not break up the function local variables with spurious new lines
> >>> like this, please.
> >>> [...]
> >>> The indentation and tabbing is messed up in all of the code you are
> >>> adding, please fix it up to be consistent with the surrounding code
> >>> and the rest of the TCP stack.
> >>>
> >>> Do not use C++ style // comments.
> >>
> >> Better?
> >
> > Please, include the changelog message on resubmits too next time.
>
> I'm sorry, which message do you mean? I used plain diff without GIT or
> anything.
In the original mail you had some description about the patch, I was
thinking of quoting something out of it but it was missing so I didn't
bother copying. But more importantly, would Dave Miller want to apply the
patch of yours, he would need that description and it saves work for him
to have the description in every resubmit "duplicated" (one usually just
gets a boomerang mail asking for the description which is similarly
wasting his time).
> >> + icsk->icsk_backoff--;
> >> + icsk->icsk_rto >>= 1;
> >
> > Are you absolute sure that we don't go to zero here when phase of the
> > moon is just right? I'd put a max(..., 1) guard there.
>
> Well, the retransmission timer doubles icsk_rto whenever it increases
> icsk_backoff, so we should reach the original icsk_rto, when
> icsk_backoff becomes zero.
This is a false assumption. Take a look into the formula, there's a
cap how far icsk_rto goes but icsk_backoff keeps increasing. In fact,
there would probably be a need to set some other lower bound than 1,
TCP_RTO_MIN would probably be more appropriate for this.
> >> + BUG_ON(!skb_r);
> >> +
> >> + if (sock_owned_by_user(sk)) {
> >> + /* Deferring retransmission clocked by ICMP
> >> + * due to locked socket. */
> >> + inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
> >> + min(icsk->icsk_rto, TCP_RESOURCE_PROBE_INTERVAL),
> >
> > ??? (besides having indent wrong). What makes you think HZ/2 is right
> > value if icsk_rto is large?
>
> To be honest, I have taken over the code from my predecessor and didn't
> question this part.
But you did Signoff that regardless, so you should know ;-).
> When thinking about it, I would remove this block
> completely. Will bad things happen, if I mess with the timer, when the
> socket is locked?
I've no idea without looking into all the details but I think this needs
to be moved slightly later anyway into the imminent expiry branch and the
timeout value needs to be copied from tcp_write_timer instead of using
that bogus formula.
> >> + TCP_RTO_MAX);
> >
> > Perhaps you lack here something to exit?
> >
> >> + }
> >> +
> >> + if (tcp_time_stamp - TCP_SKB_CB(skb_r)->when >
> >> + inet_csk(sk)->icsk_rto) {
> >
> > icsk->icsk_rto !?!
> >
> >> + /* RTO revert clocked out retransmission. */
> >> + tcp_retransmit_skb(sk, skb_r);
> >
> > This is plain wrong, tcp_sock counters will get messed up if
> > TCPCB_SACKED_RETRANS is already set while calling tcp_retransmit_skb. As
> > a sidenote, it would be probably useful to move the check + clear of
> > that bit before doing the retransmission to some common place one day.
>
> What, if I call tcp_retransmit_timer() manually instead? Will there
> still be problems with SACK?
SACK is fine then, and I took a quick look into the other stuff which
seemed to be quite ok too but it was just a quick glance, but then you
certainly must delay the RTO if sock_owned_by_user(sk) is true like
tcp_write_timer does. Tcp_retransmit_timer would also deal with the
missing step 5.5 btw. I'm not sure if you like all the things that will
happen in the tcp_enter_loss though, especially the undo_marker clearing
might not please you, or is at best, sub-optimal in a certain scenario.
> >> + inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
> >> + icsk->icsk_rto, TCP_RTO_MAX);
> >
> > RFC 2988, step 5.5 missing? Have you verified this patch for real?
>
> Thats the whole point of the draft. If consecutive retransmissions
> trigger fitting ICMPs, the RTO value is not doubled, but remains
> constant.
No, either the draft is wrong or the implementation. 5.5 step should be
applied. Now you end up decreasing RTO per icmp which causes imminent
retransmission instead of keeping it "constant". Effectively, step 4, 5, 6
(halves like you do), 7 -> R, R is 5.4-5.6 and 5.5 increases RTO again
because the timer _expired_ here even though we did short-circuit it
instead of setting a zero timer and using the normal procedure (which in
itself is ok approach, no need to force timer to expire).
> So you can have a retransmission schedule of
> t, t+r, t+2r, t+3r, t+4r, t+5r ...
> instead of
> t, t+r, t+3r, t+7r, t+15r, t+31r ...
Sure, but that's not what happens, instead you will get this series:
t, t+r, (t+r)/2, (t+r)/4, ...
...disclaimer: it might be off-by-something somewhere but I hope you get
my point anyway.
> I can show you trace plots of patched an unpatched TCP, if you like.
> Should I attach files in the mails, or better post hyperlinks to them?
Sure, but they must not be from a trivial case but the rto must
"flunctuate" and the expirys must be using the "imminent" branch for it
to count for me as a proof that you did it right.
> > How about:
> >
> > u32 remaining;
> >
> > remaining = icsk->icsk_rto - min(icsk->icsk_rto,
> > tcp_time_stamp - TCP_SKB_CB(skb_r)->when));
> > if (!remaining) {
> > tcp_retransmit_skb(...);
> > remaining = icsk->icsk_rto;
> > }
> > inet_csk_reset_xmit_timer(..., remaining);
>
> Seems to be ok, but isn't tcp_retransmit_timer(...) better?
Yeah, that's mostly fine I guess (at least in meaning of "safe").
> >> - if (icsk->icsk_retransmits >= sysctl_tcp_retries1) {
> >> + if (retrans_overstepped(sk, sysctl_tcp_retries1)) {
> >
> > ??? Could you justify these changes and explain their relation to the
> > draft and icmp messages? If not, please drop them and try with proper
> > description and description for them in a separate patch. I fail to see
> > what you're trying to achieve here. You just ended up redefining the
> > sysctl meaning too I think...
>
> Well, you're quite right. RFCs specify timeouts in dimensions of time
> (seconds, minutes, etc.), but Linux specifies timeouts in form of
> numbers of retransmissions. One can do this, as long as the
> retransmission schedule (exponential backoff in RFC case) is known. But
> with this patch, the schedule can be completely different. In the
> "worst" case, the time intervals between consecutive retransmission
> probes are constant, leading to a TCP timeout after few seconds, instead
> of several minutes.
> What I did here, is to calculate the TCP timeout as if the schedule were
> still an exponential backoff, given the allowed number of
> retransmissions. It is possible, that TCP will now retransmit many more
> times than tcp_retries indicates, but the duration until the TCP
> connection times out, is more or less equivalent to unpatched TCP with
> same tcp_retries values.
> So, whenever some control block uses count values, but means time
> values, I have to replace the control block appropriately; and that's
> what retrans_overstepped() is supposed to do.
I can understand your point, however, this is independent thing which
should not be mixed with the other part of the change. So in future
provide them as a two patch series. ...And expect some to not like this
kind of change.
--
i.
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Avi Kivity @ 2009-08-18 22:06 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <4A8B1C7F.4060008@redhat.com>
On 08/19/2009 12:26 AM, Avi Kivity wrote:
>>
>> Off the top of my head, I would think that transporting userspace
>> addresses in the ring (for copy_(to|from)_user()) vs. physical addresses
>> (for DMAEngine) might be a problem. Pinning userspace pages into memory
>> for DMA is a bit of a pain, though it is possible.
>
>
> Oh, the ring doesn't transport userspace addresses. It transports
> guest addresses, and it's up to vhost to do something with them.
>
> Currently vhost supports two translation modes:
>
> 1. virtio address == host virtual address (using copy_to_user)
> 2. virtio address == offsetted host virtual address (using copy_to_user)
>
> The latter mode is used for kvm guests (with multiple offsets,
> skipping some details).
>
> I think you need to add a third mode, virtio address == host physical
> address (using dma engine). Once you do that, and wire up the
> signalling, things should work.
You don't need in fact a third mode. You can mmap the x86 address space
into your ppc userspace and use the second mode. All you need then is
the dma engine glue and byte swapping.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [RFC net-next PATCH 0/4] qlge: Performance changes for qlge.
From: Ron Mercer @ 2009-08-18 21:26 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <20090817.175700.207432952.davem@davemloft.net>
Dave,
Thanks for the quick feedback. I will re-spin per my comments below.
>
> > 1) Do TX completions in send path (with cleaner timer).
>
> You should really do them in NAPI context.
>
> When you do them from hardware interrupt context, they all
> get rescheduled into a softirq for the real SKB freeing
> work anyways.
>
> So by doing it in NAPI poll, you're avoiding some needless
> overhead.
>
> BTW, it's insanely confusing that there is a function called
> qlge_msix_tx_isr() that of all things does RX work :-/
>
I tried to do the patch series as a logical progression but might have
made it more confusing. Patch 1 moves TX completion processing to the
hardware interrupt context (as you pointed out). Patch 2 moves it from
interrupt context to the send path as many drivers do.
It wasn't my intention to do the processinging in the ISR. Sorry about
the confusion.
> > 2) Change RSS queue count to match MSIx vector count instead
> > of CPU count. Some platforms didn't offer enough vectors
> > for our previous approach.
>
> Ideally you want "max(num_msix_vectors, num_cpus)" because
> if you hook up more MSIX vectors than you have cpus it's just
> extra overhead and depending upon the descrepency between the
> two counts it might unevenly distribute traffic work amongst
> the cpus.
I think you mean "min(num_msix_vectors, num_cpus)". That is what I'm
trying to do in the patch. I will clean it up and improve comments before I
resubmit.
>
> > 3) Change large RX buffer logic to use either multiple pages
> > or chunks of pages based on MTU and system page size.
> >
> > Examples:
> >
> > 64k Pages with 1500 MTU. The RX buffers size would be
> > 2048 bytes and there would be 32 per page.
> >
> > 4k pages with 9000 MTU. The RX buffer size would be 16k,
> > or 4 pages per buffer.
>
> This is wasteful, does the card have a mechnism by which it
> can dynamically carve up pages depending upon the actual
> frame size?
>
> If anything, make sure that skb->truesize gets set to something
> reasonable, or else TCP is going to reallocate SKBs when the
> receive queue limits are hit.
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Avi Kivity @ 2009-08-18 21:26 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <20090818205919.GA1168@ovro.caltech.edu>
On 08/18/2009 11:59 PM, Ira W. Snyder wrote:
> On a non shared-memory system (where the guest's RAM is not just a chunk
> of userspace RAM in the host system), virtio's management model seems to
> fall apart. Feature negotiation doesn't work as one would expect.
>
In your case, virtio-net on the main board accesses PCI config space
registers to perform the feature negotiation; software on your PCI cards
needs to trap these config space accesses and respond to them according
to virtio ABI.
(There's no real guest on your setup, right? just a kernel running on
and x86 system and other kernels running on the PCI cards?)
> This does appear to be solved by vbus, though I haven't written a
> vbus-over-PCI implementation, so I cannot be completely sure.
>
Even if virtio-pci doesn't work out for some reason (though it should),
you can write your own virtio transport and implement its config space
however you like.
> I'm not at all clear on how to get feature negotiation to work on a
> system like mine. From my study of lguest and kvm (see below) it looks
> like userspace will need to be involved, via a miscdevice.
>
I don't see why. Is the kernel on the PCI cards in full control of all
accesses?
> Ok. I thought I should at least express my concerns while we're
> discussing this, rather than being too late after finding the time to
> study the driver.
>
> Off the top of my head, I would think that transporting userspace
> addresses in the ring (for copy_(to|from)_user()) vs. physical addresses
> (for DMAEngine) might be a problem. Pinning userspace pages into memory
> for DMA is a bit of a pain, though it is possible.
>
Oh, the ring doesn't transport userspace addresses. It transports guest
addresses, and it's up to vhost to do something with them.
Currently vhost supports two translation modes:
1. virtio address == host virtual address (using copy_to_user)
2. virtio address == offsetted host virtual address (using copy_to_user)
The latter mode is used for kvm guests (with multiple offsets, skipping
some details).
I think you need to add a third mode, virtio address == host physical
address (using dma engine). Once you do that, and wire up the
signalling, things should work.
> There is also the problem of different endianness between host and guest
> in virtio-net. The struct virtio_net_hdr (include/linux/virtio_net.h)
> defines fields in host byte order. Which totally breaks if the guest has
> a different endianness. This is a virtio-net problem though, and is not
> transport specific.
>
Yeah. You'll need to add byteswaps.
> I've browsed over both the kvm and lguest code, and it looks like they
> each re-invent a mechanism for transporting interrupts between the host
> and guest, using eventfd. They both do this by implementing a
> miscdevice, which is basically their management interface.
>
> See drivers/lguest/lguest_user.c (see write() and LHREQ_EVENTFD) and
> kvm-kmod-devel-88/x86/kvm_main.c (see kvm_vm_ioctl(), called via
> kvm_dev_ioctl()) for how they hook up eventfd's.
>
> I can now imagine how two userspace programs (host and guest) could work
> together to implement a management interface, including hotplug of
> devices, etc. Of course, this would basically reinvent the vbus
> management interface into a specific driver.
>
You don't need anything in the guest userspace (virtio-net) side.
> I think this is partly what Greg is trying to abstract out into generic
> code. I haven't studied the actual data transport mechanisms in vbus,
> though I have studied virtio's transport mechanism. I think a generic
> management interface for virtio might be a good thing to consider,
> because it seems there are at least two implementations already: kvm and
> lguest.
>
Management code in the kernel doesn't really help unless you plan to
manage things with echo and cat.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [PATCH] Revert netlink ABI change to gnet_stats_basic
From: Arnd Bergmann @ 2009-08-18 21:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Michael Spang, David S. Miller, LKML, Linux Netdev List
In-Reply-To: <4A885FD1.4030105@gmail.com>
On Sunday 16 August 2009 19:36:49 Eric Dumazet wrote:
> In 5e140dfc1fe87eae27846f193086724806b33c7d "net: reorder struct Qdisc
> for better SMP performance" the definition of struct gnet_stats_basic
> changed incompatibly, as copies of this struct are shipped to
> userland via netlink.
>
> Restoring old behavior is not welcome, for performance reason.
>
> Fix is to use a private structure for kernel, and
> teach gnet_stats_copy_basic() to convert from kernel to user land,
> using legacy structure (struct gnet_stats_basic)
Are you sure that the packed structure is actually an advantage
for performance? On architectures that do not have unaligned stores
in hardware, the compiler will generate highly inefficient code
for accessing packed variables, in order to avoid alignment exceptions.
It would need some more measurements, but I'd guess that
struct gnet_stats_basic_packed
{
__u64 bytes __attribute__ ((packed, aligned(4)));
__u32 packets;
};
would give significantly better code on those architectures than
your version, while still giving you the 12-byte size.
Arnd <><
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Arnd Bergmann @ 2009-08-18 21:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Ira W. Snyder, Avi Kivity, Gregory Haskins, kvm, netdev,
linux-kernel, alacrityvm-devel, Anthony Liguori, Ingo Molnar,
Gregory Haskins
In-Reply-To: <20090818203522.GA20393@redhat.com>
On Tuesday 18 August 2009 20:35:22 Michael S. Tsirkin wrote:
> On Tue, Aug 18, 2009 at 10:27:52AM -0700, Ira W. Snyder wrote:
> > Also, in my case I'd like to boot Linux with my rootfs over NFS. Is
> > vhost-net capable of this?
> >
> > I've had Arnd, BenH, and Grant Likely (and others, privately) contact me
> > about devices they are working with that would benefit from something
> > like virtio-over-PCI. I'd like to see vhost-net be merged with the
> > capability to support my use case. There are plenty of others that would
> > benefit, not just myself.
yes.
> > I'm not sure vhost-net is being written with this kind of future use in
> > mind. I'd hate to see it get merged, and then have to change the ABI to
> > support physical-device-to-device usage. It would be better to keep
> > future use in mind now, rather than try and hack it in later.
>
> I still need to think your usage over. I am not so sure this fits what
> vhost is trying to do. If not, possibly it's better to just have a
> separate driver for your device.
I now think we need both. virtio-over-PCI does it the right way for its
purpose and can be rather generic. It could certainly be extended to
support virtio-net on both sides (host and guest) of KVM, but I think
it better fits the use where a kernel wants to communicate with some
other machine where you normally wouldn't think of using qemu.
Vhost-net OTOH is great in the way that it serves as an easy way to
move the virtio-net code from qemu into the kernel, without changing
its behaviour. It should even straightforward to do live-migration between
hosts with and without it, something that would be much harder with
the virtio-over-PCI logic. Also, its internal state is local to the
process owning its file descriptor, which makes it much easier to
manage permissions and cleanup of its resources.
Arnd <><
^ permalink raw reply
* Re: [PATCH] net: add Xilinx emac lite device driver
From: Stephen Hemminger @ 2009-08-18 21:04 UTC (permalink / raw)
To: John Linn
Cc: netdev, linuxppc-dev, jgarzik, davem, John Linn, Grant Likely,
Josh Boyer, John Williams, Michal Simek, Sadanand M
In-Reply-To: <20090818153047.48E42ED8051@mail66-dub.bigfish.com>
On Tue, 18 Aug 2009 09:30:41 -0600
John Linn <john.linn@xilinx.com> wrote:
> +/**
> + * xemaclite_enable_interrupts - Enable the interrupts for the EmacLite device
> + * @drvdata: Pointer to the Emaclite device private data
> + *
> + * This function enables the Tx and Rx interrupts for the Emaclite device along
> + * with the Global Interrupt Enable.
> + */
> +static void xemaclite_enable_interrupts(struct net_local *drvdata)
Docbook format is really a not necessary on local functions that
are only used in the driver. It is fine if you want to use it, as
long as the file isn't processed by kernel make docs but
the docbook is intended for automatic generation of kernel API manuals.
--
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Ira W. Snyder @ 2009-08-18 20:59 UTC (permalink / raw)
To: Avi Kivity
Cc: Michael S. Tsirkin, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <4A8AF880.6080704@redhat.com>
On Tue, Aug 18, 2009 at 09:52:48PM +0300, Avi Kivity wrote:
> On 08/18/2009 09:27 PM, Ira W. Snyder wrote:
>>> I think in this case you want one side to be virtio-net (I'm guessing
>>> the x86) and the other side vhost-net (the ppc boards with the dma
>>> engine). virtio-net on x86 would communicate with userspace on the ppc
>>> board to negotiate features and get a mac address, the fast path would
>>> be between virtio-net and vhost-net (which would use the dma engine to
>>> push and pull data).
>>>
>>>
>>
>> Ah, that seems backwards, but it should work after vhost-net learns how
>> to use the DMAEngine API.
>>
>> I haven't studied vhost-net very carefully yet. As soon as I saw the
>> copy_(to|from)_user() I stopped reading, because it seemed useless for
>> my case. I'll look again and try to find where vhost-net supports
>> setting MAC addresses and other features.
>>
>
> It doesn't; all it does is pump the rings, leaving everything else to
> userspace.
>
Ok.
On a non shared-memory system (where the guest's RAM is not just a chunk
of userspace RAM in the host system), virtio's management model seems to
fall apart. Feature negotiation doesn't work as one would expect.
This does appear to be solved by vbus, though I haven't written a
vbus-over-PCI implementation, so I cannot be completely sure.
I'm not at all clear on how to get feature negotiation to work on a
system like mine. From my study of lguest and kvm (see below) it looks
like userspace will need to be involved, via a miscdevice.
>> Also, in my case I'd like to boot Linux with my rootfs over NFS. Is
>> vhost-net capable of this?
>>
>
> It's just another network interface. You'd need an initramfs though to
> contain the needed userspace.
>
Ok. I'm using an initramfs already, so adding some more userspace to it
isn't a problem.
>> I've had Arnd, BenH, and Grant Likely (and others, privately) contact me
>> about devices they are working with that would benefit from something
>> like virtio-over-PCI. I'd like to see vhost-net be merged with the
>> capability to support my use case. There are plenty of others that would
>> benefit, not just myself.
>>
>> I'm not sure vhost-net is being written with this kind of future use in
>> mind. I'd hate to see it get merged, and then have to change the ABI to
>> support physical-device-to-device usage. It would be better to keep
>> future use in mind now, rather than try and hack it in later.
>>
>
> Please review and comment then. I'm fairly confident there won't be any
> ABI issues since vhost-net does so little outside pumping the rings.
>
Ok. I thought I should at least express my concerns while we're
discussing this, rather than being too late after finding the time to
study the driver.
Off the top of my head, I would think that transporting userspace
addresses in the ring (for copy_(to|from)_user()) vs. physical addresses
(for DMAEngine) might be a problem. Pinning userspace pages into memory
for DMA is a bit of a pain, though it is possible.
There is also the problem of different endianness between host and guest
in virtio-net. The struct virtio_net_hdr (include/linux/virtio_net.h)
defines fields in host byte order. Which totally breaks if the guest has
a different endianness. This is a virtio-net problem though, and is not
transport specific.
> Note the signalling paths go through eventfd: when vhost-net wants the
> other side to look at its ring, it tickles an eventfd which is supposed
> to trigger an interrupt on the other side. Conversely, when another
> eventfd is signalled, vhost-net will look at the ring and process any
> data there. You'll need to wire your signalling to those eventfds,
> either in userspace or in the kernel.
>
Ok. I've never used eventfd before, so that'll take yet more studying.
I've browsed over both the kvm and lguest code, and it looks like they
each re-invent a mechanism for transporting interrupts between the host
and guest, using eventfd. They both do this by implementing a
miscdevice, which is basically their management interface.
See drivers/lguest/lguest_user.c (see write() and LHREQ_EVENTFD) and
kvm-kmod-devel-88/x86/kvm_main.c (see kvm_vm_ioctl(), called via
kvm_dev_ioctl()) for how they hook up eventfd's.
I can now imagine how two userspace programs (host and guest) could work
together to implement a management interface, including hotplug of
devices, etc. Of course, this would basically reinvent the vbus
management interface into a specific driver.
I think this is partly what Greg is trying to abstract out into generic
code. I haven't studied the actual data transport mechanisms in vbus,
though I have studied virtio's transport mechanism. I think a generic
management interface for virtio might be a good thing to consider,
because it seems there are at least two implementations already: kvm and
lguest.
Thanks for answering my questions. It helps to talk with someone more
familiar with the issues than I am.
Ira
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Michael S. Tsirkin @ 2009-08-18 20:57 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Gregory Haskins, kvm, netdev, linux-kernel, alacrityvm-devel,
Avi Kivity, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <20090818155329.GD31060@ovro.caltech.edu>
On Tue, Aug 18, 2009 at 08:53:29AM -0700, Ira W. Snyder wrote:
> I think Greg is referring to something like my virtio-over-PCI patch.
> I'm pretty sure that vhost is completely useless for my situation. I'd
> like to see vhost work for my use, so I'll try to explain what I'm
> doing.
>
> I've got a system where I have about 20 computers connected via PCI. The
> PCI master is a normal x86 system, and the PCI agents are PowerPC
> systems. The PCI agents act just like any other PCI card, except they
> are running Linux, and have their own RAM and peripherals.
>
> I wrote a custom driver which imitated a network interface and a serial
> port. I tried to push it towards mainline, and DavidM rejected it, with
> the argument, "use virtio, don't add another virtualization layer to the
> kernel." I think he has a decent argument, so I wrote virtio-over-PCI.
>
> Now, there are some things about virtio that don't work over PCI.
> Mainly, memory is not truly shared. It is extremely slow to access
> memory that is "far away", meaning "across the PCI bus." This can be
> worked around by using a DMA controller to transfer all data, along with
> an intelligent scheme to perform only writes across the bus. If you're
> careful, reads are never needed.
>
> So, in my system, copy_(to|from)_user() is completely wrong.
> There is no userspace, only a physical system.
Can guests do DMA to random host memory? Or is there some kind of IOMMU
and DMA API involved? If the later, then note that you'll still need
some kind of driver for your device. The question we need to ask
ourselves then is whether this driver can reuse bits from vhost.
> In fact, because normal x86 computers
> do not have DMA controllers, the host system doesn't actually handle any
> data transfer!
Is it true that PPC has to initiate all DMA then? How do you
manage not to do DMA reads then?
> I used virtio-net in both the guest and host systems in my example
> virtio-over-PCI patch, and succeeded in getting them to communicate.
> However, the lack of any setup interface means that the devices must be
> hardcoded into both drivers, when the decision could be up to userspace.
> I think this is a problem that vbus could solve.
What you describe (passing setup from host to guest) seems like
a feature that guest devices need to support. It seems unlikely that
vbus, being a transport layer, can address this.
>
> For my own selfish reasons (I don't want to maintain an out-of-tree
> driver) I'd like to see *something* useful in mainline Linux. I'm happy
> to answer questions about my setup, just ask.
>
> Ira
Thanks Ira, I'll think about it.
A couple of questions:
- Could you please describe what kind of communication needs to happen?
- I'm not familiar with DMA engine in question. I'm guessing it's the
usual thing: in/out buffers need to be kernel memory, interface is
asynchronous, small limited number of outstanding requests? Is there a
userspace interface for it and if yes how does it work?
--
MST
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Michael S. Tsirkin @ 2009-08-18 20:39 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Avi Kivity, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <20090818172752.GC17631@ovro.caltech.edu>
On Tue, Aug 18, 2009 at 10:27:52AM -0700, Ira W. Snyder wrote:
> On Tue, Aug 18, 2009 at 07:51:21PM +0300, Avi Kivity wrote:
> > On 08/18/2009 06:53 PM, Ira W. Snyder wrote:
> >> So, in my system, copy_(to|from)_user() is completely wrong. There is no
> >> userspace, only a physical system. In fact, because normal x86 computers
> >> do not have DMA controllers, the host system doesn't actually handle any
> >> data transfer!
> >>
> >
> > In fact, modern x86s do have dma engines these days (google for Intel
> > I/OAT), and one of our plans for vhost-net is to allow their use for
> > packets above a certain size. So a patch allowing vhost-net to
> > optionally use a dma engine is a good thing.
> >
>
> Yes, I'm aware that very modern x86 PCs have general purpose DMA
> engines, even though I don't have any capable hardware. However, I think
> it is better to support using any PC (with or without DMA engine, any
> architecture) as the PCI master, and just handle the DMA all from the
> PCI agent, which is known to have DMA?
>
> >> I used virtio-net in both the guest and host systems in my example
> >> virtio-over-PCI patch, and succeeded in getting them to communicate.
> >> However, the lack of any setup interface means that the devices must be
> >> hardcoded into both drivers, when the decision could be up to userspace.
> >> I think this is a problem that vbus could solve.
> >>
> >
> > Exposing a knob to userspace is not an insurmountable problem; vhost-net
> > already allows changing the memory layout, for example.
> >
>
> Let me explain the most obvious problem I ran into: setting the MAC
> addresses used in virtio.
>
> On the host (PCI master), I want eth0 (virtio-net) to get a random MAC
> address.
>
> On the guest (PCI agent), I want eth0 (virtio-net) to get a specific MAC
> address, aa:bb:cc:dd:ee:ff.
>
> The virtio feature negotiation code handles this, by seeing the
> VIRTIO_NET_F_MAC feature in it's configuration space. If BOTH drivers do
> not have VIRTIO_NET_F_MAC set, then NEITHER will use the specified MAC
> address. This is because the feature negotiation code only accepts a
> feature if it is offered by both sides of the connection.
>
> In this case, I must have the guest generate a random MAC address and
> have the host put aa:bb:cc:dd:ee:ff into the guest's configuration
> space. This basically means hardcoding the MAC addresses in the Linux
> drivers, which is a big no-no.
>
> What would I expose to userspace to make this situation manageable?
>
> Thanks for the response,
> Ira
This calls for some kind of change in guest virtio. vhost being a host
kernel only feature, does not deal with this problem. But assuming
virtio in guest supports this somehow, vhost will not interfere: you do
the setup in qemu userspace anyway, vhost will happily use a network
device however you chose to set it up.
--
MST
^ permalink raw reply
* Re: [Alacrityvm-devel] [PATCH v3 3/6] vbus: add a "vbus-proxy" bus model for vbus_driver objects
From: Michael S. Tsirkin @ 2009-08-18 20:35 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Avi Kivity, Gregory Haskins, kvm, netdev, linux-kernel,
alacrityvm-devel, Anthony Liguori, Ingo Molnar, Gregory Haskins
In-Reply-To: <20090818182735.GD17631@ovro.caltech.edu>
On Tue, Aug 18, 2009 at 11:27:35AM -0700, Ira W. Snyder wrote:
> I haven't studied vhost-net very carefully yet. As soon as I saw the
> copy_(to|from)_user() I stopped reading, because it seemed useless for
> my case. I'll look again and try to find where vhost-net supports
> setting MAC addresses and other features.
vhost net doesn't do this at all. You bind raw socket to a network
device, and program that with usual userspace interfaces.
> Also, in my case I'd like to boot Linux with my rootfs over NFS. Is
> vhost-net capable of this?
>
> I've had Arnd, BenH, and Grant Likely (and others, privately) contact me
> about devices they are working with that would benefit from something
> like virtio-over-PCI. I'd like to see vhost-net be merged with the
> capability to support my use case. There are plenty of others that would
> benefit, not just myself.
>
> I'm not sure vhost-net is being written with this kind of future use in
> mind. I'd hate to see it get merged, and then have to change the ABI to
> support physical-device-to-device usage. It would be better to keep
> future use in mind now, rather than try and hack it in later.
I still need to think your usage over. I am not so sure this fits what
vhost is trying to do. If not, possibly it's better to just have a
separate driver for your device.
> Thanks for the comments.
> Ira
^ permalink raw reply
* [PATCH 2/3] Expose may_setuid() in user.h and add may_setgid() (v2)
From: Dan Smith @ 2009-08-18 19:57 UTC (permalink / raw)
To: orenl; +Cc: containers, netdev, Serge Hallyn
In-Reply-To: <1250625435-16299-1-git-send-email-danms@us.ibm.com>
Make these helpers available to others.
Changes in v2:
- Avoid checking the groupinfo in ctx->realcred against the current in
may_setgid()
Cc: Serge Hallyn <serue@us.ibm.com>
Signed-off-by: Dan Smith <danms@us.ibm.com>
---
include/linux/user.h | 9 +++++++++
kernel/user.c | 13 ++++++++++++-
2 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/include/linux/user.h b/include/linux/user.h
index 68daf84..c231e9c 100644
--- a/include/linux/user.h
+++ b/include/linux/user.h
@@ -1 +1,10 @@
+#ifndef _LINUX_USER_H
+#define _LINUX_USER_H
+
#include <asm/user.h>
+#include <linux/sched.h>
+
+extern int may_setuid(struct user_namespace *ns, uid_t uid);
+extern int may_setgid(gid_t gid);
+
+#endif
diff --git a/kernel/user.c b/kernel/user.c
index a535ed6..a78fde7 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -604,7 +604,7 @@ int checkpoint_user(struct ckpt_ctx *ctx, void *ptr)
return do_checkpoint_user(ctx, (struct user_struct *) ptr);
}
-static int may_setuid(struct user_namespace *ns, uid_t uid)
+int may_setuid(struct user_namespace *ns, uid_t uid)
{
/*
* this next check will one day become
@@ -631,6 +631,17 @@ static int may_setuid(struct user_namespace *ns, uid_t uid)
return 0;
}
+int may_setgid(gid_t gid)
+{
+ if (capable(CAP_SETGID))
+ return 1;
+
+ if (in_egroup_p(gid))
+ return 1;
+
+ return 0;
+}
+
static struct user_struct *do_restore_user(struct ckpt_ctx *ctx)
{
struct user_struct *u;
--
1.6.2.5
^ permalink raw reply related
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