* Re: [PATCH v6 3/4] net: moxa: synchronize DMA memory
From: David Miller @ 2014-08-26 0:26 UTC (permalink / raw)
To: jonas.jensen
Cc: netdev, linux-arm-kernel, linux-kernel, f.fainelli, eric.dumazet,
mirqus
In-Reply-To: <1408976552-15659-1-git-send-email-jonas.jensen@gmail.com>
From: Jonas Jensen <jonas.jensen@gmail.com>
Date: Mon, 25 Aug 2014 16:22:32 +0200
> DMA memory should be synchronized before data is passed
> to/from controller.
>
> Add dma_sync_single_for_cpu(.., DMA_FROM_DEVICE) to RX path
> and dma_sync_single_for_device(.., DMA_TO_DEVICE) to TX path.
>
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH v6 2/4] net: moxa: replace build_skb() with netdev_alloc_skb_ip_align() / memcpy()
From: David Miller @ 2014-08-26 0:26 UTC (permalink / raw)
To: jonas.jensen
Cc: netdev, linux-arm-kernel, linux-kernel, f.fainelli, eric.dumazet,
mirqus
In-Reply-To: <1408976542-15624-1-git-send-email-jonas.jensen@gmail.com>
From: Jonas Jensen <jonas.jensen@gmail.com>
Date: Mon, 25 Aug 2014 16:22:22 +0200
> build_skb() is used to make skbs out of existing RX ring memory
> which is bad because the RX ring is allocated only once, on probe.
> Memory corruption occur because said memory is reclaimed, i.e.
> __kfree_skb() (and eventually put_page()).
>
> Replace build_skb() with netdev_alloc_skb_ip_align() and use memcpy().
>
> Remove SKB_DATA_ALIGN() from RX buffer size while we're at it.
>
> Addresses https://bugzilla.kernel.org/show_bug.cgi?id=69041
>
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH v6 1/4] net: moxa: clear DESC1 on ndo_start_xmit()
From: David Miller @ 2014-08-26 0:25 UTC (permalink / raw)
To: jonas.jensen
Cc: netdev, linux-arm-kernel, linux-kernel, f.fainelli, eric.dumazet,
mirqus
In-Reply-To: <1408976531-15589-1-git-send-email-jonas.jensen@gmail.com>
From: Jonas Jensen <jonas.jensen@gmail.com>
Date: Mon, 25 Aug 2014 16:22:11 +0200
> TX buffer length is not cleared on ndo_start_xmit().
> Failing to do so can bug/hang the controller and
> cause TX interrupts to stop altogether.
>
> Remove the readl() and compute a new value for DESC1.
>
> Addresses https://bugzilla.kernel.org/show_bug.cgi?id=69031
>
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net v2] net: fix checksum features handling in netif_skb_features()
From: David Miller @ 2014-08-26 0:23 UTC (permalink / raw)
To: mkubecek; +Cc: netdev, linux-kernel
In-Reply-To: <20140825131622.6233CA0BB0@unicorn.suse.cz>
From: Michal Kubecek <mkubecek@suse.cz>
Date: Mon, 25 Aug 2014 15:16:22 +0200 (CEST)
> This is follow-up to
>
> da08143b8520 ("vlan: more careful checksum features handling")
>
> which introduced more careful feature intersection in vlan code,
> taking into account that HW_CSUM should be considered superset
> of IP_CSUM/IPV6_CSUM. The same is needed in netif_skb_features()
> in order to avoid offloading mismatch warning when vlan is
> created on top of a bond consisting of slaves supporting IP/IPv6
> checksumming but not vlan Tx offloading.
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [Patch net-next 2/2] net: make skb an optional parameter for__skb_flow_dissect()
From: David Miller @ 2014-08-26 0:22 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev
In-Reply-To: <1409011427-18438-2-git-send-email-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 25 Aug 2014 17:03:47 -0700
> Fixes: commit 690e36e726d00d2 (net: Allow raw buffers to be passed into the flow dissector)
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied.
^ permalink raw reply
* Re: [Patch net-next 1/2] net: fix comments for __skb_flow_get_ports()
From: David Miller @ 2014-08-26 0:21 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev
In-Reply-To: <1409011427-18438-1-git-send-email-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 25 Aug 2014 17:03:46 -0700
> Fixes: commit 690e36e726d00d2 (net: Allow raw buffers to be passed into the flow dissector)
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied.
^ permalink raw reply
* [Patch net-next 2/2] net: make skb an optional parameter for__skb_flow_dissect()
From: Cong Wang @ 2014-08-26 0:03 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David S. Miller
In-Reply-To: <1409011427-18438-1-git-send-email-xiyou.wangcong@gmail.com>
Fixes: commit 690e36e726d00d2 (net: Allow raw buffers to be passed into the flow dissector)
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
include/net/flow_keys.h | 4 ++--
net/core/flow_dissector.c | 18 +++++++++++++++---
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 4040f63..9a03f73 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -28,10 +28,10 @@ struct flow_keys {
};
bool __skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow,
- void *data, int hlen);
+ void *data, __be16 proto, int nhoff, int hlen);
static inline bool skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow)
{
- return __skb_flow_dissect(skb, flow, NULL, 0);
+ return __skb_flow_dissect(skb, flow, NULL, 0, 0, 0);
}
__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
void *data, int hlen_proto);
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index ae8f0db..12f48ca 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -59,14 +59,26 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
}
EXPORT_SYMBOL(__skb_flow_get_ports);
-bool __skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow, void *data, int hlen)
+/**
+ * __skb_flow_dissect - extract the flow_keys struct and return it
+ * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
+ * @data: raw buffer pointer to the packet, if NULL use skb->data
+ * @proto: protocol for which to get the flow, if @data is NULL use skb->protocol
+ * @nhoff: network header offset, if @data is NULL use skb_network_offset(skb)
+ * @hlen: packet header length, if @data is NULL use skb_headlen(skb)
+ *
+ * The function will try to retrieve the struct flow_keys from either the skbuff
+ * or a raw buffer specified by the rest parameters
+ */
+bool __skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow,
+ void *data, __be16 proto, int nhoff, int hlen)
{
- int nhoff = skb_network_offset(skb);
u8 ip_proto;
- __be16 proto = skb->protocol;
if (!data) {
data = skb->data;
+ proto = skb->protocol;
+ nhoff = skb_network_offset(skb);
hlen = skb_headlen(skb);
}
--
1.8.3.1
^ permalink raw reply related
* [Patch net-next 1/2] net: fix comments for __skb_flow_get_ports()
From: Cong Wang @ 2014-08-26 0:03 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David S. Miller
Fixes: commit 690e36e726d00d2 (net: Allow raw buffers to be passed into the flow dissector)
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/core/flow_dissector.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 8ffcc97..ae8f0db 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -26,10 +26,12 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
}
/**
- * skb_flow_get_ports - extract the upper layer ports and return them
- * @skb: buffer to extract the ports from
+ * __skb_flow_get_ports - extract the upper layer ports and return them
+ * @skb: sk_buff to extract the ports from
* @thoff: transport header offset
* @ip_proto: protocol for which to get port offset
+ * @data: raw buffer pointer to the packet, if NULL use skb->data
+ * @hlen: packet header length, if @data is NULL use skb_headlen(skb)
*
* The function will try to retrieve the ports at offset thoff + poff where poff
* is the protocol port offset returned from proto_ports_offset
--
1.8.3.1
^ permalink raw reply related
* Re: using rhashtable in inethash
From: Thomas Graf @ 2014-08-25 23:42 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Paul E. McKenney, John Fastabend
In-Reply-To: <1409009735.3173.6.camel@edumazet-glaptop2.roam.corp.google.com>
On 08/25/14 at 04:35pm, Eric Dumazet wrote:
> Thomas agreed to work on all this. If not I can do this myself.
I already got started and will share the code as soon as I have
something semi functional.
^ permalink raw reply
* Re: using rhashtable in inethash
From: David Miller @ 2014-08-25 23:36 UTC (permalink / raw)
To: tgraf; +Cc: eric.dumazet, netdev, paulmck
In-Reply-To: <20140825233346.GF30140@casper.infradead.org>
From: Thomas Graf <tgraf@suug.ch>
Date: Tue, 26 Aug 2014 00:33:46 +0100
> The reason I added added the sync is because I could not see how
> else to prevent the sock_put() in netlink_release() to release
> socket memoray, specifically the embedded rhash_head, that is
> possibly being accessed in a RCU protected reader traversing the
> bucket. Such a reader would not hold a reference to the socket.
> I may be missing something though and I'm happy to change this for
> something better.
Ok, this is going to be a tree-wide issue where we try to use
rhashtable with sockets of any type.
Perhaps we're overdue for RCU freeing of sockets, but that added
noticable latency last time I tried it.
^ permalink raw reply
* Re: using rhashtable in inethash
From: Eric Dumazet @ 2014-08-25 23:35 UTC (permalink / raw)
To: David Miller; +Cc: tgraf, netdev, Paul E. McKenney, John Fastabend
In-Reply-To: <20140825.161224.1108200625385309828.davem@davemloft.net>
On Mon, 2014-08-25 at 16:12 -0700, David Miller wrote:
> During the Networking Workshop I mentioned converting the inet hash
> tables over to rhashtable so that we don't allocate this insanely
> large hash table at boot time which goes largely unused.
>
> I took a quick look at this last night and the only thing we really
> need is the addition of a set of rhashtable interfaces which use
> NULLs lists, as the inet hashtables currently require.
>
> Also, I noticed in the netlink changes this really expensive
> synchronize_net() added to netlink_release(), is that _really_
> necessary?
>
> That's really expensive and my impression was that such a sync is only
> needed during hash table resizing, not when getting rid of objects
> that we in an rhashtable.
>
> Thomas?
After the workshop we had a discussion with Paul McKenney, Thomas, and
John where we discussed all this.
An other issue we raised was that the grow should happen in process
context, while inserts and deletes should happen from softirq.
We also mentioned the need of a per bucket spinlock, and keep the mutex
only to protect the resizes. (Or an array of spinlocks as used by TCP)
Paul gave a lot of ideas and this seems feasible.
Thomas agreed to work on all this. If not I can do this myself.
(Note that conntrack also uses the same nulls rcu hash tables)
^ permalink raw reply
* [PATCH 2/2] ixgbe: support skb->xmit_more in netdev_ops->ndo_start_xmit()
From: David Miller @ 2014-08-25 23:35 UTC (permalink / raw)
To: netdev
Cc: therbert, jhs, hannes, edumazet, jeffrey.t.kirsher, rusty,
dborkman, brouer
From: Daniel Borkmann <dborkman@redhat.com>
This implements the deferred tail pointer flush API for the ixgbe
driver. Similar version also proposed longer time ago by Alexander Duyck.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 87bd53f..ba9ceaa 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6958,9 +6958,10 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring,
tx_ring->next_to_use = i;
- /* notify HW of packet */
- ixgbe_write_tail(tx_ring, i);
-
+ if (!skb->xmit_more) {
+ /* notify HW of packet */
+ ixgbe_write_tail(tx_ring, i);
+ }
return;
dma_error:
dev_err(tx_ring->dev, "TX DMA map failed\n");
--
1.7.11.7
^ permalink raw reply related
* [PATCH 1/2] net: Remove ndo_xmit_flush netdev operation, use signalling instead.
From: David Miller @ 2014-08-25 23:35 UTC (permalink / raw)
To: netdev
Cc: therbert, jhs, hannes, edumazet, jeffrey.t.kirsher, rusty,
dborkman, brouer
As reported by Jesper Dangaard Brouer, for high packet rates the
overhead of having another indirect call in the TX path is
non-trivial.
There is the indirect call itself, and then there is all of the
reloading of the state to refetch the tail pointer value and
then write the device register.
Move to a more passive scheme, which requires very light modifications
to the device drivers.
The signal is a new skb->xmit_more value, if it is non-zero it means
that more SKBs are pending to be transmitted on the same queue as the
current SKB. And therefore, the driver may elide the tail pointer
update.
Right now skb->xmit_more is always zero.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
drivers/net/ethernet/intel/igb/igb_main.c | 36 +++++++++++--------------------
drivers/net/virtio_net.c | 12 +++--------
include/linux/netdevice.h | 25 ++-------------------
include/linux/skbuff.h | 2 ++
4 files changed, 19 insertions(+), 56 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b9c020a..89c29b4 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -136,7 +136,6 @@ static void igb_update_phy_info(unsigned long);
static void igb_watchdog(unsigned long);
static void igb_watchdog_task(struct work_struct *);
static netdev_tx_t igb_xmit_frame(struct sk_buff *skb, struct net_device *);
-static void igb_xmit_flush(struct net_device *netdev, u16 queue);
static struct rtnl_link_stats64 *igb_get_stats64(struct net_device *dev,
struct rtnl_link_stats64 *stats);
static int igb_change_mtu(struct net_device *, int);
@@ -2076,7 +2075,6 @@ static const struct net_device_ops igb_netdev_ops = {
.ndo_open = igb_open,
.ndo_stop = igb_close,
.ndo_start_xmit = igb_xmit_frame,
- .ndo_xmit_flush = igb_xmit_flush,
.ndo_get_stats64 = igb_get_stats64,
.ndo_set_rx_mode = igb_set_rx_mode,
.ndo_set_mac_address = igb_set_mac,
@@ -4917,6 +4915,14 @@ static void igb_tx_map(struct igb_ring *tx_ring,
tx_ring->next_to_use = i;
+ if (!skb->xmit_more) {
+ writel(i, tx_ring->tail);
+
+ /* we need this if more than one processor can write to our tail
+ * at a time, it synchronizes IO on IA64/Altix systems
+ */
+ mmiowb();
+ }
return;
dma_error:
@@ -5052,20 +5058,17 @@ out_drop:
return NETDEV_TX_OK;
}
-static struct igb_ring *__igb_tx_queue_mapping(struct igb_adapter *adapter, unsigned int r_idx)
+static inline struct igb_ring *igb_tx_queue_mapping(struct igb_adapter *adapter,
+ struct sk_buff *skb)
{
+ unsigned int r_idx = skb->queue_mapping;
+
if (r_idx >= adapter->num_tx_queues)
r_idx = r_idx % adapter->num_tx_queues;
return adapter->tx_ring[r_idx];
}
-static inline struct igb_ring *igb_tx_queue_mapping(struct igb_adapter *adapter,
- struct sk_buff *skb)
-{
- return __igb_tx_queue_mapping(adapter, skb->queue_mapping);
-}
-
static netdev_tx_t igb_xmit_frame(struct sk_buff *skb,
struct net_device *netdev)
{
@@ -5094,21 +5097,6 @@ static netdev_tx_t igb_xmit_frame(struct sk_buff *skb,
return igb_xmit_frame_ring(skb, igb_tx_queue_mapping(adapter, skb));
}
-static void igb_xmit_flush(struct net_device *netdev, u16 queue)
-{
- struct igb_adapter *adapter = netdev_priv(netdev);
- struct igb_ring *tx_ring;
-
- tx_ring = __igb_tx_queue_mapping(adapter, queue);
-
- writel(tx_ring->next_to_use, tx_ring->tail);
-
- /* we need this if more than one processor can write to our tail
- * at a time, it synchronizes IO on IA64/Altix systems
- */
- mmiowb();
-}
-
/**
* igb_tx_timeout - Respond to a Tx Hang
* @netdev: network interface device structure
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6242108..f0c2824 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -953,15 +953,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
}
}
- return NETDEV_TX_OK;
-}
+ if (!skb->xmit_more)
+ virtqueue_kick(sq->vq);
-static void xmit_flush(struct net_device *dev, u16 qnum)
-{
- struct virtnet_info *vi = netdev_priv(dev);
- struct send_queue *sq = &vi->sq[qnum];
-
- virtqueue_kick(sq->vq);
+ return NETDEV_TX_OK;
}
/*
@@ -1393,7 +1388,6 @@ static const struct net_device_ops virtnet_netdev = {
.ndo_open = virtnet_open,
.ndo_stop = virtnet_close,
.ndo_start_xmit = start_xmit,
- .ndo_xmit_flush = xmit_flush,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address = virtnet_set_mac_address,
.ndo_set_rx_mode = virtnet_set_rx_mode,
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 220c509..039b237 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -782,19 +782,6 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
* (can also return NETDEV_TX_LOCKED iff NETIF_F_LLTX)
* Required can not be NULL.
*
- * void (*ndo_xmit_flush)(struct net_device *dev, u16 queue);
- * A driver implements this function when it wishes to support
- * deferred TX queue flushing. The idea is that the expensive
- * operation to trigger TX queue processing can be done after
- * N calls to ndo_start_xmit rather than being done every single
- * time. In this regime ndo_start_xmit will be called one or more
- * times, and then a final ndo_xmit_flush call will be made to
- * have the driver tell the device about the new pending TX queue
- * entries. The kernel keeps track of which queues need flushing
- * by monitoring skb->queue_mapping of the packets it submits to
- * ndo_start_xmit. This is the queue value that will be passed
- * to ndo_xmit_flush.
- *
* u16 (*ndo_select_queue)(struct net_device *dev, struct sk_buff *skb,
* void *accel_priv, select_queue_fallback_t fallback);
* Called to decide which queue to when device supports multiple
@@ -1018,7 +1005,6 @@ struct net_device_ops {
int (*ndo_stop)(struct net_device *dev);
netdev_tx_t (*ndo_start_xmit) (struct sk_buff *skb,
struct net_device *dev);
- void (*ndo_xmit_flush)(struct net_device *dev, u16 queue);
u16 (*ndo_select_queue)(struct net_device *dev,
struct sk_buff *skb,
void *accel_priv,
@@ -3447,15 +3433,8 @@ int __init dev_proc_init(void);
static inline netdev_tx_t __netdev_start_xmit(const struct net_device_ops *ops,
struct sk_buff *skb, struct net_device *dev)
{
- netdev_tx_t ret;
- u16 q;
-
- q = skb->queue_mapping;
- ret = ops->ndo_start_xmit(skb, dev);
- if (dev_xmit_complete(ret) && ops->ndo_xmit_flush)
- ops->ndo_xmit_flush(dev, q);
-
- return ret;
+ skb->xmit_more = 0;
+ return ops->ndo_start_xmit(skb, dev);
}
static inline netdev_tx_t netdev_start_xmit(struct sk_buff *skb, struct net_device *dev)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 18ddf96..9b3802a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -452,6 +452,7 @@ static inline u32 skb_mstamp_us_delta(const struct skb_mstamp *t1,
* @tc_verd: traffic control verdict
* @hash: the packet hash
* @queue_mapping: Queue mapping for multiqueue devices
+ * @xmit_more: More SKBs are pending for this queue
* @ndisc_nodetype: router type (from link layer)
* @ooo_okay: allow the mapping of a socket to a queue to be changed
* @l4_hash: indicate hash is a canonical 4-tuple hash over transport
@@ -558,6 +559,7 @@ struct sk_buff {
__u16 queue_mapping;
kmemcheck_bitfield_begin(flags2);
+ __u8 xmit_more:1;
#ifdef CONFIG_IPV6_NDISC_NODETYPE
__u8 ndisc_nodetype:2;
#endif
--
1.7.11.7
^ permalink raw reply related
* [PATCH 0/2] Get rid of ndo_xmit_flush
From: David Miller @ 2014-08-25 23:34 UTC (permalink / raw)
To: netdev
Cc: therbert, jhs, hannes, edumazet, jeffrey.t.kirsher, rusty,
dborkman, brouer
Given Jesper's performance numbers, it's not the way to go.
Instead, go with a signalling scheme via new boolean skb->xmit_more.
This has several advantages:
1) Nearly trivial driver support, just protect the tail pointer
update with the skb->xmit_more check.
2) No extra indirect calls in the non-deferral cases.
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: using rhashtable in inethash
From: Thomas Graf @ 2014-08-25 23:33 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev, paulmck
In-Reply-To: <20140825.161224.1108200625385309828.davem@davemloft.net>
On 08/25/14 at 04:12pm, David Miller wrote:
> During the Networking Workshop I mentioned converting the inet hash
> tables over to rhashtable so that we don't allocate this insanely
> large hash table at boot time which goes largely unused.
> I took a quick look at this last night and the only thing we really
> need is the addition of a set of rhashtable interfaces which use
> NULLs lists, as the inet hashtables currently require.
Eric brought this up as well last week. I see no problem using
a non-NULL token as the default to identify the end of the list.
We had a quick sitdown with Paul E. McKenney and came up with
some ideas that should allow doing that even with resizes taking
place by using the hash of the entry as the token.
We also discussed the possibility to do the resizing outside of
the insert/remove context and move it to a worker thread using
per bucket locks. We think that we may have found something that
might work and I will give that a shot. It will allow to use a
resizing rhashtable with the insert/remove being in atomic
context which I believe is needed for the inet cache.
> Also, I noticed in the netlink changes this really expensive
> synchronize_net() added to netlink_release(), is that _really_
> necessary?
>
>
> That's really expensive and my impression was that such a sync is only
> needed during hash table resizing, not when getting rid of objects
> that we in an rhashtable.
The reason I added added the sync is because I could not see how
else to prevent the sock_put() in netlink_release() to release
socket memoray, specifically the embedded rhash_head, that is
possibly being accessed in a RCU protected reader traversing the
bucket. Such a reader would not hold a reference to the socket.
I may be missing something though and I'm happy to change this for
something better.
^ permalink raw reply
* Re: [RFC PATCH net-next 1/3] ixgbe: support netdev_ops->ndo_xmit_flush()
From: David Miller @ 2014-08-25 23:31 UTC (permalink / raw)
To: brouer; +Cc: dborkman, netdev
In-Reply-To: <20140825.153248.1202565040217198094.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Mon, 25 Aug 2014 15:32:48 -0700 (PDT)
> Jesper, just for fun, could you revert all of the xmit flush stuff and
> test this patch instead?
This doesn't work properly, sorry. We need to explicitly set xmit_more
to zero before ->ndo_start_xmit() calls, because the initial zero'ing
of that value isn't propagated in copy_skb_header() (nor do we want to
add that).
But I'm beyond convinced now that ->ndo_xmit_flush() is not the way to
do this.
I'm about to post a set of patches which will go into net-next which:
1) Converts the tree to skb->xmit_more
2) Adds Daniel's IXGBE conversion, adjusted for xmit_more.
^ permalink raw reply
* Re: [PATCH 0/3] Basic deferred TX queue flushing infrastructure.
From: Vlad Yasevich @ 2014-08-25 23:24 UTC (permalink / raw)
To: David Miller, cwang
Cc: netdev, therbert, jhs, hannes, edumazet, jeffrey.t.kirsher, rusty
In-Reply-To: <20140825.153146.2165451041039058085.davem@davemloft.net>
On 08/25/2014 06:31 PM, David Miller wrote:
> From: Cong Wang <cwang@twopensource.com>
> Date: Mon, 25 Aug 2014 15:21:00 -0700
>
>> When I tried to unify the list management of SKB's, I was surprised to see
>> there are still some places relying on skb->next and skb->prev to be
>> the head of the skb struct, since nowadays we have list API's, they still
>> play some magic on these pointers (sctp and tipc IIRC). This is why I
>> gave up, maybe it's time to revise this again.
>
> I think SCTP should be OK, and yes I do remember that protocol being one of
> the last subsystems making such SKB list pointer assumptions.
>
> It was using list_*() operations on sk_buff objects or something like that.
All I see that's left is __skb_unlink, __skb_queue_tail and skb_queue_splice_tail_init()
I think you've convert all of them a while ago.
-vlad
>
>> Talking about skb->next, fortunately we do gso segmentation after
>> going out of qdisc queues, otherwise it's scary to play with these
>> pointers at same time. I think all queues of SKB's are either using
>> just ->next or both ->prev and ->next.
>
> It occurs to me that perhaps the thing to do is to pass sk_buff ** to
> dev_hard_start_xmit().
>
> If it really is important to free the original GSO skb after the
> segmented parts, we can run that as part of the destructor of the
> final segment.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] net: Allow raw buffers to be passed into the flow dissector.
From: David Miller @ 2014-08-25 23:14 UTC (permalink / raw)
To: cwang
Cc: amirv, alexander.h.duyck, netdev, ogerlitz, yevgenyp, idos,
eric.dumazet
In-Reply-To: <CAHA+R7NTcRiOrP=M5=i8NkTHYZ3-RT8w-J5FPNsDh88c7yYKvQ@mail.gmail.com>
From: Cong Wang <cwang@twopensource.com>
Date: Mon, 25 Aug 2014 16:13:01 -0700
> On Sat, Aug 23, 2014 at 12:18 PM, David Miller <davem@davemloft.net> wrote:
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 5f362c1..660c649 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -34,29 +34,40 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
>> * The function will try to retrieve the ports at offset thoff + poff where poff
>> * is the protocol port offset returned from proto_ports_offset
>> */
>> -__be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto)
>> +__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
>> + void *data, int hlen)
>
>
> You forgot to update the comments above __skb_flow_get_ports(),
> not a big deal, I will send a patch if you don't. ;-)
Please do, thanks.
> Not sure if I read your patch correctly, I think our goal is to make
> skb parameter
> optional so that callers can pass NULL if they don't have a skb struct?
> __skb_flow_dissect() still refers skb->protocol at least.
The idea is that explicit *data and hlen can be provided in absense of
an SKB.
I see the skb->protocol reference, I guess we'll need to provide an
explicit protocol argument as well, good catch!
^ permalink raw reply
* Re: [PATCH] net: Allow raw buffers to be passed into the flow dissector.
From: Cong Wang @ 2014-08-25 23:13 UTC (permalink / raw)
To: David Miller
Cc: Amir Vadai, Alexander Duyck, netdev, Or Gerlitz, Yevgeny Petrilin,
Ido Shamay, Eric Dumazet
In-Reply-To: <20140823.121855.1259915921868754968.davem@davemloft.net>
On Sat, Aug 23, 2014 at 12:18 PM, David Miller <davem@davemloft.net> wrote:
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 5f362c1..660c649 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -34,29 +34,40 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *i
> * The function will try to retrieve the ports at offset thoff + poff where poff
> * is the protocol port offset returned from proto_ports_offset
> */
> -__be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto)
> +__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
> + void *data, int hlen)
You forgot to update the comments above __skb_flow_get_ports(),
not a big deal, I will send a patch if you don't. ;-)
Not sure if I read your patch correctly, I think our goal is to make
skb parameter
optional so that callers can pass NULL if they don't have a skb struct?
__skb_flow_dissect() still refers skb->protocol at least.
Thanks for the patch anyway.
^ permalink raw reply
* using rhashtable in inethash
From: David Miller @ 2014-08-25 23:12 UTC (permalink / raw)
To: tgraf; +Cc: eric.dumazet, netdev
During the Networking Workshop I mentioned converting the inet hash
tables over to rhashtable so that we don't allocate this insanely
large hash table at boot time which goes largely unused.
I took a quick look at this last night and the only thing we really
need is the addition of a set of rhashtable interfaces which use
NULLs lists, as the inet hashtables currently require.
Also, I noticed in the netlink changes this really expensive
synchronize_net() added to netlink_release(), is that _really_
necessary?
That's really expensive and my impression was that such a sync is only
needed during hash table resizing, not when getting rid of objects
that we in an rhashtable.
Thomas?
^ permalink raw reply
* Re: [PATCH iproute2] nstat: 64bit support on 32bit arches
From: Eric Dumazet @ 2014-08-25 22:52 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20140825144808.25459645@urahara>
On Mon, 2014-08-25 at 14:48 -0700, Stephen Hemminger wrote:
>
> Maybe time to convert to using uint64_t and the printf formats in inttypes.h
Yes, that would be a cleanup, but end result is quite ugly if you ask
me.
^ permalink raw reply
* Re: [RFC PATCH net-next 1/3] ixgbe: support netdev_ops->ndo_xmit_flush()
From: Alexander Duyck @ 2014-08-25 22:51 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Daniel Borkmann; +Cc: davem, netdev
In-Reply-To: <20140825140721.162a6c91@redhat.com>
On 08/25/2014 05:07 AM, Jesper Dangaard Brouer wrote:
> On Sun, 24 Aug 2014 15:42:16 +0200
> Daniel Borkmann <dborkman@redhat.com> wrote:
>
>> This implements the deferred tail pointer flush API for the ixgbe
>> driver. Similar version also proposed longer time ago by Alexander Duyck.
>
> I've run some benchmarks with this patch only, which actually shows a
> performance regression.
>
> Using trafgen with QDISC_BYPASS and mmap mode, via cmdline:
> trafgen --cpp --dev eth5 --conf udp_example01.trafgen -V --cpus 1
>
> BASELINE(no-patch): trafgen QDISC_BYPASS and mmap:
> - tx:1562539 pps
>
> (This patch only): ixgbe use of .ndo_xmit_flush.
> - tx:1532299 pps
>
> Regression: -30240 pps
> * In nanosec: (1/1562539*10^9)-(1/1532299*10^9) = -12.63 ns
>
>
> As DaveM points out, me might not need the mmiowb().
> Result when not performing the mmiowb():
> - tx:1548352 pps
>
> Still a small regression: -14187 pps
> * In nanosec: (1/1562539*10^9)-(1/1548352*10^9) = -5.86 ns
>
>
> I was not expecting this "slowdown", with this rather simple use of the
> new ndo_xmit_flush API. Can anyone explain why this is happening?
One possibility is that we are now doing less stuff between the time we
write tail and when we grab the qdisc lock (locked transactions are
stalled by MMIO) so that we are spending more time stuck waiting for the
write to complete and doing nothing.
Then of course there are always the funny oddball quirks such as the
code changes might have changed the alignment of a loop resulting in Tx
cleanup more expensive than it was before.
Thanks,
Alex
^ permalink raw reply
* Re: [patch net-next RFC 10/12] openvswitch: add support for datapath hardware offload
From: Thomas Graf @ 2014-08-25 22:50 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: ryazanov.s.a-Re5JQEeQqe8AvxtiuMwx3w, ronye-VPRAkNaXOzVWk0Htik3J/w,
jasowang-H+wXaHxf7aLQT0dZR+AlfA,
john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w,
Neil.Jerram-QnUH15yq9NYqDJ6do+/SaQ,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, Andy Gospodarek,
dev-yBygre7rU0TnMu66kgdUjQ, nbd-p3rKhJxN3npAfugRpC6u6w,
f.fainelli-Re5JQEeQqe8AvxtiuMwx3w, John Fastabend,
jeffrey.t.kirsher-ral2JQCrhuEAvxtiuMwx3w, ogerlitz,
ben-/+tVBieCtBitmTQ+vhA3Yw, buytenh-OLH4Qvv75CYX/NnBR394Jw,
Jiri Pirko, roopa-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR,
aviadr-VPRAkNaXOzVWk0Htik3J/w,
nicolas.dichtel-pdR9zngts4EAvxtiuMwx3w,
vyasevic-H+wXaHxf7aLQT0dZR+AlfA, Neil Horman, netdev,
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ, dborkman,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, David Miller
In-Reply-To: <53FB6122.2040901-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>
On 08/25/14 at 12:15pm, Jamal Hadi Salim wrote:
> On 08/25/14 10:17, Thomas Graf wrote:
> >On 08/25/14 at 09:53am, Jamal Hadi Salim wrote:
>
> >fdb_add() *is* flow based. At least in my understanding, the whole
> >point here is to extend the idea of fdb_add() and make it understand
> >L2-L4 in a more generic way for the most common protocols.
> >
> >The reason fdb_add() is not reused is because it is Netlink specific
> >and only suitable for User -> HW offload. Kernel -> HW offload is
> >technically possible but not clean.
> >
>
> I dont think we have a problem handling any of this today.
Yes we do. It's restricted to L2 and we can't extend it easily
because it is based on NDA_*. The use of Netlink makes in-kernel
usage a pain. To me this is the sole reason for not using fdb_add()
in the first place. It seems absolutely clear though that fdb_add()
should be removed after the more generic ndo is in place providing
a superset of what fdb_add() can do today.
> This is where our (shall i say strong) disagreement is.
> I think you will find it non-trivial to show me how you can
> actually take the simple L2 bridge and map it to a "flow".
> Since your starting point is "everything can be represented via a flow
> and some table" - we are at a crosspath.
OK, let me do the convertion for you:
NDA_DST unused
NDA_LLADDR sw_flow_key.eth.dst
NDA_CACHEINFO unused
NDA_PROBES unused
NDA_VLAN sw_flow_key.eth.tci
NDA_PORT unused
NDA_VNI sw_flow_key.tun_key.tun_id
NDA_IFINDEX sw_flow_key.phys.in_port
NDA_MASTER unused
> The tc filter API seems to be doing just that.
> You have different types of classifiers - the h/w may not be able
> to support some classifier types - but that is a capability discovery
> challenge.
Agreed but tc is only one out of many possible existing interfaces
we have. macvtap (given we want to extend beyond L2), routing,
OVS, bridge and eventually even things like a team device can and
should make use of offloads.
> I am saying two things:
> 1) There are a few "fundamental" interfaces; L2 and L3 being some.
> Add crypto offload and a few i mentioned in my presentation. We
Can you share that preso? I was not present.
> know how to do those. example; there is nothing i cant do with
> the rtmsg that is L3. or the fdb/port/vlan filter for L2.
> This flow thing should stay out of those.
Let me remind you about the name of the structure behind all L3
forwarding decisions:
struct flowi4 {
[...]
}
Adding a route means adding a flow. Can we please stop the flow
bashing? The concept of a flow is very generic, well known and already
very present in the kernel.
The sw_flow_key proposed comes close to flowi4. Some fields are
different. They can eventually get merged. The strict IPv4/IPv6
separation is what makes it non obvious and probably why Jiri chose
the OVS representation. If you say rtmsg is complete then that clearly
is not the case. In particular VTEP fields, ARP, and TCP flags are
clearly missing for many uses.
Again, I'm not saying flow is the ultimate answer to everything. It
is not. But a lot of hardware out there is aware of flows in combination
with some form of action execution. Non flow based hardware can have
their own classifier.
> 2) The flow thing should allow a variety of classifiers to be
> handled. Again capability discovery would take care of differences.
So you want the flow to represent something that is not a flow. Again,
this comes back to the conversation in the other email. If this is
all about having a single ndo I'm sure we can find common grounds on
that.
^ permalink raw reply
* Re: [RFC PATCH net-next] ipv6: stop sending PTB packets for MTU < 1280
From: Hannes Frederic Sowa @ 2014-08-25 22:47 UTC (permalink / raw)
To: Hagen Paul Pfeifer; +Cc: netdev, Fernando Gont
In-Reply-To: <1409005545-24910-2-git-send-email-hagen@jauu.net>
Hi Hagen,
On Di, 2014-08-26 at 00:25 +0200, Hagen Paul Pfeifer wrote:
> Reduce the attack vector and stop generating ICMPv6 packet to big for
> packets smaller then the minimal required IPv6 MTU.
>
> See
> http://tools.ietf.org/html/draft-gont-6man-deprecate-atomfrag-generation-00
I wonder if we should wait until this gets RFC status?
I very much welcome this decision! I already raised this problem some
time ago:
http://lists.openwall.net/netdev/2013/12/31/17
I wonder if we should add a mode alike ipv4 ip_no_pmtu_disc mode for
ipv6:
ip_no_pmtu_disc - INTEGER
Disable Path MTU Discovery. If enabled in mode 1 and a
fragmentation-required ICMP is received, the PMTU to this
destination will be set to min_pmtu (see below). You will need
to raise min_pmtu to the smallest interface MTU on your system
manually if you want to avoid locally generated fragments.
In mode 2 incoming Path MTU Discovery messages will be
discarded. Outgoing frames are handled the same as in mode 1,
implicitly setting IP_PMTUDISC_DONT on every created socket.
Mode 3 is a hardend pmtu discover mode. The kernel will only
accept fragmentation-needed errors if the underlying protocol
can verify them besides a plain socket lookup. Current
protocols for which pmtu events will be honored are TCP, SCTP
and DCCP as they verify e.g. the sequence number or the
association. This mode should not be enabled globally but is
only intended to secure e.g. name servers in namespaces where
TCP path mtu must still work but path MTU information of other
protocols should be discarded. If enabled globally this mode
could break other protocols.
Possible values: 0-3
Default: FALSE
Not sure yet...
> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
> ---
> net/ipv6/route.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f74b041..84ebacd 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1154,12 +1154,9 @@ static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
> struct net *net = dev_net(dst->dev);
>
> rt6->rt6i_flags |= RTF_MODIFIED;
> - if (mtu < IPV6_MIN_MTU) {
> - u32 features = dst_metric(dst, RTAX_FEATURES);
> + if (mtu < IPV6_MIN_MTU)
> mtu = IPV6_MIN_MTU;
> - features |= RTAX_FEATURE_ALLFRAG;
> - dst_metric_set(dst, RTAX_FEATURES, features);
> - }
> +
> dst_metric_set(dst, RTAX_MTU, mtu);
> rt6_update_expires(rt6, net->ipv6.sysctl.ip6_rt_mtu_expires);
> }
This patch is a starter, yes. We can now get rid of the dst_allfrag
function altogether.
Thanks,
Hannes
^ permalink raw reply
* RE: [PATCH 0/3] Basic deferred TX queue flushing infrastructure.
From: Jon Maloy @ 2014-08-25 22:45 UTC (permalink / raw)
To: David Miller, cwang@twopensource.com
Cc: netdev@vger.kernel.org, therbert@google.com, jhs@mojatatu.com,
hannes@stressinduktion.org, edumazet@google.com,
jeffrey.t.kirsher@intel.com, rusty@rustcorp.com.au
In-Reply-To: <20140825.153146.2165451041039058085.davem@davemloft.net>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of David Miller
> Sent: August-25-14 6:32 PM
> To: cwang@twopensource.com
> Cc: netdev@vger.kernel.org; therbert@google.com; jhs@mojatatu.com;
> hannes@stressinduktion.org; edumazet@google.com;
> jeffrey.t.kirsher@intel.com; rusty@rustcorp.com.au
> Subject: Re: [PATCH 0/3] Basic deferred TX queue flushing infrastructure.
>
> From: Cong Wang <cwang@twopensource.com>
> Date: Mon, 25 Aug 2014 15:21:00 -0700
>
> > When I tried to unify the list management of SKB's, I was surprised to
> > see there are still some places relying on skb->next and skb->prev to
> > be the head of the skb struct, since nowadays we have list API's, they
> > still play some magic on these pointers (sctp and tipc IIRC). This is
I am not aware of any such assumptions in TIPC. For me you can go ahead
with this.
Regards
///jon
> > why I gave up, maybe it's time to revise this again.
>
> I think SCTP should be OK, and yes I do remember that protocol being one of
> the last subsystems making such SKB list pointer assumptions.
>
> It was using list_*() operations on sk_buff objects or something like that.
>
> > Talking about skb->next, fortunately we do gso segmentation after
> > going out of qdisc queues, otherwise it's scary to play with these
> > pointers at same time. I think all queues of SKB's are either using
> > just ->next or both ->prev and ->next.
>
> It occurs to me that perhaps the thing to do is to pass sk_buff ** to
> dev_hard_start_xmit().
>
> If it really is important to free the original GSO skb after the segmented parts,
> we can run that as part of the destructor of the final segment.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in the body
> of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
^ 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;
as well as URLs for NNTP newsgroup(s).