* Re: XDP performance regression due to CONFIG_RETPOLINE Spectre V2
From: Tushar Dave @ 2018-04-13 17:12 UTC (permalink / raw)
To: Christoph Hellwig, Jesper Dangaard Brouer
Cc: xdp-newbies@vger.kernel.org, netdev@vger.kernel.org,
David Woodhouse, William Tu, Björn Töpel,
Karlsson, Magnus, Alexander Duyck, Arnaldo Carvalho de Melo
In-Reply-To: <20180412145653.GA7172@lst.de>
On 04/12/2018 07:56 AM, Christoph Hellwig wrote:
> On Thu, Apr 12, 2018 at 04:51:23PM +0200, Christoph Hellwig wrote:
>> On Thu, Apr 12, 2018 at 03:50:29PM +0200, Jesper Dangaard Brouer wrote:
>>> ---------------
>>> Implement support for keeping the DMA mapping through the XDP return
>>> call, to remove RX map/unmap calls. Implement bulking for XDP
>>> ndo_xdp_xmit and XDP return frame API. Bulking allows to perform DMA
>>> bulking via scatter-gatter DMA calls, XDP TX need it for DMA
>>> map+unmap. The driver RX DMA-sync (to CPU) per packet calls are harder
>>> to mitigate (via bulk technique). Ask DMA maintainer for a common
>>> case direct call for swiotlb DMA sync call ;-)
>>
>> Why do you even end up in swiotlb code? Once you bounce buffer your
>> performance is toast anyway..
>
> I guess that is because x86 selects it as the default as soon as
> we have more than 4G memory. That should be solveable fairly easily
> with the per-device dma ops, though.\
I guess there is nothing we need to do!
On x86, in case of no intel iommu or iommu is disabled, you end up in
swiotlb for DMA API calls when system has 4G memory.
However, AFAICT, for 64bit DMA capable devices swiotlb DMA APIs do not
use bounce buffer until and unless you have swiotlb=force specified in
kernel commandline.
e.g. here is the snip:
dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
unsigned long offset, size_t size,
enum dma_data_direction dir,
unsigned long attrs)
{
phys_addr_t map, phys = page_to_phys(page) + offset;
dma_addr_t dev_addr = phys_to_dma(dev, phys);
BUG_ON(dir == DMA_NONE);
/*
* If the address happens to be in the device's DMA window,
* we can safely return the device addr and not worry about bounce
* buffering it.
*/
if (dma_capable(dev, dev_addr, size) && swiotlb_force !=
SWIOTLB_FORCE)
return dev_addr;
-Tushar
^ permalink raw reply
* imaging solutions
From: Ross @ 2018-04-13 14:51 UTC (permalink / raw)
To: netdev
Hi,
Not sure if you received my email from last week.
We offer following image editing services:
images cutting out, clipping path, masking
jewelry photos retouching
beauty photos retouching
also wedding photos etc
If you want to test our quality of work.
You may send us one photo with instruction and we will work on it.
Hope to hear from you soon.
Regards,
Ross
The Studio Manager
^ permalink raw reply
* Re: XDP performance regression due to CONFIG_RETPOLINE Spectre V2
From: Christoph Hellwig @ 2018-04-13 17:26 UTC (permalink / raw)
To: Tushar Dave
Cc: Christoph Hellwig, Jesper Dangaard Brouer,
xdp-newbies@vger.kernel.org, netdev@vger.kernel.org,
David Woodhouse, William Tu, Björn Töpel,
Karlsson, Magnus, Alexander Duyck, Arnaldo Carvalho de Melo
In-Reply-To: <fbcb4cc8-53fb-82cd-bd9d-76c5fdd47918@oracle.com>
On Fri, Apr 13, 2018 at 10:12:41AM -0700, Tushar Dave wrote:
> I guess there is nothing we need to do!
>
> On x86, in case of no intel iommu or iommu is disabled, you end up in
> swiotlb for DMA API calls when system has 4G memory.
> However, AFAICT, for 64bit DMA capable devices swiotlb DMA APIs do not
> use bounce buffer until and unless you have swiotlb=force specified in
> kernel commandline.
Sure. But that means very sync_*_to_device and sync_*_to_cpu now
involves an indirect call to do exactly nothing, which in the workload
Jesper is looking at is causing a huge performance degradation due to
retpolines.
^ permalink raw reply
* Re: [PATCH net-next 1/3] net: ethernet: ave: add multiple clocks and resets support as required property
From: Rob Herring @ 2018-04-13 17:26 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: David Miller, netdev, Andrew Lunn, Florian Fainelli, Mark Rutland,
linux-arm-kernel, linux-kernel, devicetree, Masahiro Yamada,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <1523255925-6469-2-git-send-email-hayashi.kunihiko@socionext.com>
On Mon, Apr 09, 2018 at 03:38:43PM +0900, Kunihiko Hayashi wrote:
> When the link is becoming up for Pro4 SoC, the kernel is stalled
> due to some missing clocks and resets.
>
> The AVE block for Pro4 is connected to the GIO bus in the SoC.
> Without its clock/reset, the access to the AVE register makes the
> system stall.
>
> In the same way, another MAC clock for Giga-bit Connection and
> the PHY clock are also required for Pro4 to activate the Giga-bit feature
> and to recognize the PHY.
>
> To satisfy these requirements, this patch adds support for multiple clocks
> and resets, and adds the clock-names and reset-names to the binding because
> we need to distinguish clock/reset for the AVE main block and the others.
>
> Also, make the resets a required property. Currently, "reset is
> optional" relies on that the bootloader or firmware has deasserted
> the reset before booting the kernel. Drivers should work without
> such expectation.
>
> Fixes: 4c270b55a5af ("net: ethernet: socionext: add AVE ethernet driver")
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
> .../bindings/net/socionext,uniphier-ave4.txt | 13 ++-
Reviewed-by: Rob Herring <robh@kernel.org>
> drivers/net/ethernet/socionext/sni_ave.c | 108 ++++++++++++++++-----
> 2 files changed, 96 insertions(+), 25 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next 2/3] dt-bindings: net: ave: add syscon-phy-mode property to configure phy-mode setting
From: Rob Herring @ 2018-04-13 17:27 UTC (permalink / raw)
To: Kunihiko Hayashi
Cc: David Miller, netdev, Andrew Lunn, Florian Fainelli, Mark Rutland,
linux-arm-kernel, linux-kernel, devicetree, Masahiro Yamada,
Masami Hiramatsu, Jassi Brar
In-Reply-To: <1523255925-6469-3-git-send-email-hayashi.kunihiko@socionext.com>
On Mon, Apr 09, 2018 at 03:38:44PM +0900, Kunihiko Hayashi wrote:
> Add "socionext,syscon-phy-mode" property to specify system controller that
> configures the settings about phy-mode.
>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
> Documentation/devicetree/bindings/net/socionext,uniphier-ave4.txt | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH net] team: avoid adding twice the same option to the event list
From: David Miller @ 2018-04-13 18:07 UTC (permalink / raw)
To: pabeni; +Cc: netdev, jiri
In-Reply-To: <0e295e62358a68b22c646adece4272a9bd0473f8.1523620752.git.pabeni@redhat.com>
From: Paolo Abeni <pabeni@redhat.com>
Date: Fri, 13 Apr 2018 13:59:25 +0200
> When parsing the options provided by the user space,
> team_nl_cmd_options_set() insert them in a temporary list to send
> multiple events with a single message.
> While each option's attribute is correctly validated, the code does
> not check for duplicate entries before inserting into the event
> list.
>
> Exploiting the above, the syzbot was able to trigger the following
> splat:
...
> This changeset addresses the avoiding list_add() if the current
> option is already present in the event list.
>
> Reported-and-tested-by: syzbot+4d4af685432dc0e56c91@syzkaller.appspotmail.com
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Fixes: 2fcdb2c9e659 ("team: allow to send multiple set events in one message")
Looks good to me.
It's too bad that the tmp list entries don't get marked as they are
added, or get unlinked by the list processor. Either scheme would
make the "already added" test a lot simpler.
Jiri, please review before I apply this.
Thanks.
^ permalink raw reply
* ethtool 4.16 released
From: John W. Linville @ 2018-04-13 18:02 UTC (permalink / raw)
To: netdev
ethtool version 4.16 has been released.
Home page: https://www.kernel.org/pub/software/network/ethtool/
Download link:
https://www.kernel.org/pub/software/network/ethtool/ethtool-4.16.tar.xz
Release notes:
* Feature: add support for extra RSS contexts and RSS steering filters
* Feature: Document RSS context control and RSS filters
* Fix: don't fall back to grxfhindir when context was specified
* Fix: correct display of VF when showing vf/queue filters
* Fix: show VF and queue in the help for -N
* Fix: correct VF index values for the ring_cookie parameter
* Feature: Add SFF 8636 date code parsing support
John
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* [PATCH v2 net 0/3] sfc: ARFS fixes
From: Edward Cree @ 2018-04-13 18:16 UTC (permalink / raw)
To: linux-net-drivers, David Miller; +Cc: netdev
Three issues introduced by my recent asynchronous filter handling changes:
1. The old filter_rfs_insert would replace a matching filter of equal
priority; we need to pass the appropriate argument to filter_insert to
make it do the same.
2. We're lying to the kernel with our return value from ndo_rx_flow_steer,
so we need to lie consistently when calling rps_may_expire_flow. This
is only a partial fix, as the lie still prevents us from steering
multiple flows with the same ID to different queues; a proper fix that
stops us lying at all will hopefully follow later.
3. It's possible to cause the kernel to hammer ndo_rx_flow_steer very
hard, so make sure we don't build up too huge a backlog of workitems.
Possibly it would be better to fix #3 on the kernel side; I have a patch
which I think does that but it's not a regression in 4.17 so isn't 'net'
material.
There's also the issue that we come up in the bad configuration that
triggers #3 by default, but that too is a problem for another time.
Edward Cree (3):
sfc: insert ARFS filters with replace_equal=true
sfc: pass the correctly bogus filter_id to rps_may_expire_flow()
sfc: limit ARFS workitems in flight per channel
drivers/net/ethernet/sfc/ef10.c | 3 +-
drivers/net/ethernet/sfc/farch.c | 2 +-
drivers/net/ethernet/sfc/net_driver.h | 25 +++++++++++++++
drivers/net/ethernet/sfc/rx.c | 60 ++++++++++++++++++-----------------
4 files changed, 58 insertions(+), 32 deletions(-)
^ permalink raw reply
* [PATCH] PCI: Add PCIe to pcie_print_link_status() messages
From: Jakub Kicinski @ 2018-04-13 18:16 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: oss-drivers, Tal Gilboa, Tariq Toukan, Jacob Keller,
Ganesh Goudar, Jeff Kirsher, intel-wired-lan, netdev,
linux-kernel, linux-pci, Jakub Kicinski
Currently the pcie_print_link_status() will print PCIe bandwidth
and link width information but does not mention it is pertaining
to the PCIe. Since this and related functions are used exclusively
by networking drivers today users may get confused into thinking
that it's the NIC bandwidth that is being talked about. Insert a
"PCIe" into the messages.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/pci/pci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aa86e904f93c..73a0a4993f6a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5273,11 +5273,11 @@ void pcie_print_link_status(struct pci_dev *dev)
bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed, &width);
if (bw_avail >= bw_cap)
- pci_info(dev, "%u.%03u Gb/s available bandwidth (%s x%d link)\n",
+ pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n",
bw_cap / 1000, bw_cap % 1000,
PCIE_SPEED2STR(speed_cap), width_cap);
else
- pci_info(dev, "%u.%03u Gb/s available bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",
+ pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",
bw_avail / 1000, bw_avail % 1000,
PCIE_SPEED2STR(speed), width,
limiting_dev ? pci_name(limiting_dev) : "<unknown>",
--
2.16.2
^ permalink raw reply related
* [PATCH v2 net 1/3] sfc: insert ARFS filters with replace_equal=true
From: Edward Cree @ 2018-04-13 18:17 UTC (permalink / raw)
To: linux-net-drivers, David Miller; +Cc: netdev
In-Reply-To: <878265b2-a42a-d49e-0e68-0bbcabbabeaa@solarflare.com>
Necessary to allow redirecting a flow when the application moves.
Fixes: 3af0f34290f6 ("sfc: replace asynchronous filter operations")
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
drivers/net/ethernet/sfc/rx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 95682831484e..13b0eb71dbf3 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -851,7 +851,7 @@ static void efx_filter_rfs_work(struct work_struct *data)
struct efx_channel *channel = efx_get_channel(efx, req->rxq_index);
int rc;
- rc = efx->type->filter_insert(efx, &req->spec, false);
+ rc = efx->type->filter_insert(efx, &req->spec, true);
if (rc >= 0) {
/* Remember this so we can check whether to expire the filter
* later.
^ permalink raw reply related
* [PATCH v2 net 2/3] sfc: pass the correctly bogus filter_id to rps_may_expire_flow()
From: Edward Cree @ 2018-04-13 18:17 UTC (permalink / raw)
To: linux-net-drivers, David Miller; +Cc: netdev
In-Reply-To: <878265b2-a42a-d49e-0e68-0bbcabbabeaa@solarflare.com>
When we inserted an ARFS filter for ndo_rx_flow_steer(), we didn't know
what the filter ID would be, so we just returned 0. Thus, we must also
pass 0 as the filter ID when calling rps_may_expire_flow() for it, and
rely on the flow_id to identify what we're talking about.
Fixes: 3af0f34290f6 ("sfc: replace asynchronous filter operations")
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
drivers/net/ethernet/sfc/ef10.c | 3 +--
drivers/net/ethernet/sfc/farch.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 50daad0a1482..36f24c7e553a 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -4776,8 +4776,7 @@ static bool efx_ef10_filter_rfs_expire_one(struct efx_nic *efx, u32 flow_id,
goto out_unlock;
}
- if (!rps_may_expire_flow(efx->net_dev, spec->dmaq_id,
- flow_id, filter_idx)) {
+ if (!rps_may_expire_flow(efx->net_dev, spec->dmaq_id, flow_id, 0)) {
ret = false;
goto out_unlock;
}
diff --git a/drivers/net/ethernet/sfc/farch.c b/drivers/net/ethernet/sfc/farch.c
index 4a19c7efdf8d..7174ef5e5c5e 100644
--- a/drivers/net/ethernet/sfc/farch.c
+++ b/drivers/net/ethernet/sfc/farch.c
@@ -2912,7 +2912,7 @@ bool efx_farch_filter_rfs_expire_one(struct efx_nic *efx, u32 flow_id,
if (test_bit(index, table->used_bitmap) &&
table->spec[index].priority == EFX_FILTER_PRI_HINT &&
rps_may_expire_flow(efx->net_dev, table->spec[index].dmaq_id,
- flow_id, index)) {
+ flow_id, 0)) {
efx_farch_filter_table_clear_entry(efx, table, index);
ret = true;
}
^ permalink raw reply related
* [PATCH v2 net 3/3] sfc: limit ARFS workitems in flight per channel
From: Edward Cree @ 2018-04-13 18:18 UTC (permalink / raw)
To: linux-net-drivers, David Miller; +Cc: netdev
In-Reply-To: <878265b2-a42a-d49e-0e68-0bbcabbabeaa@solarflare.com>
A misconfigured system (e.g. with all interrupts affinitised to all CPUs)
may produce a storm of ARFS steering events. With the existing sfc ARFS
implementation, that could create a backlog of workitems that grinds the
system to a halt. To prevent this, limit the number of workitems that
may be in flight for a given SFC device to 8 (EFX_RPS_MAX_IN_FLIGHT), and
return EBUSY from our ndo_rx_flow_steer method if the limit is reached.
Given this limit, also store the workitems in an array of slots within the
struct efx_nic, rather than dynamically allocating for each request.
The limit should not negatively impact performance, because it is only
likely to be hit in cases where ARFS will be ineffective anyway.
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
drivers/net/ethernet/sfc/net_driver.h | 25 +++++++++++++++
drivers/net/ethernet/sfc/rx.c | 58 ++++++++++++++++++-----------------
2 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 5e379a83c729..eea3808b3f25 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -733,6 +733,27 @@ struct efx_rss_context {
u32 rx_indir_table[128];
};
+#ifdef CONFIG_RFS_ACCEL
+/**
+ * struct efx_async_filter_insertion - Request to asynchronously insert a filter
+ * @net_dev: Reference to the netdevice
+ * @spec: The filter to insert
+ * @work: Workitem for this request
+ * @rxq_index: Identifies the channel for which this request was made
+ * @flow_id: Identifies the kernel-side flow for which this request was made
+ */
+struct efx_async_filter_insertion {
+ struct net_device *net_dev;
+ struct efx_filter_spec spec;
+ struct work_struct work;
+ u16 rxq_index;
+ u32 flow_id;
+};
+
+/* Maximum number of ARFS workitems that may be in flight on an efx_nic */
+#define EFX_RPS_MAX_IN_FLIGHT 8
+#endif /* CONFIG_RFS_ACCEL */
+
/**
* struct efx_nic - an Efx NIC
* @name: Device name (net device name or bus id before net device registered)
@@ -850,6 +871,8 @@ struct efx_rss_context {
* @rps_expire_channel: Next channel to check for expiry
* @rps_expire_index: Next index to check for expiry in
* @rps_expire_channel's @rps_flow_id
+ * @rps_slot_map: bitmap of in-flight entries in @rps_slot
+ * @rps_slot: array of ARFS insertion requests for efx_filter_rfs_work()
* @active_queues: Count of RX and TX queues that haven't been flushed and drained.
* @rxq_flush_pending: Count of number of receive queues that need to be flushed.
* Decremented when the efx_flush_rx_queue() is called.
@@ -1004,6 +1027,8 @@ struct efx_nic {
struct mutex rps_mutex;
unsigned int rps_expire_channel;
unsigned int rps_expire_index;
+ unsigned long rps_slot_map;
+ struct efx_async_filter_insertion rps_slot[EFX_RPS_MAX_IN_FLIGHT];
#endif
atomic_t active_queues;
diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 13b0eb71dbf3..9c593c661cbf 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -827,28 +827,13 @@ MODULE_PARM_DESC(rx_refill_threshold,
#ifdef CONFIG_RFS_ACCEL
-/**
- * struct efx_async_filter_insertion - Request to asynchronously insert a filter
- * @net_dev: Reference to the netdevice
- * @spec: The filter to insert
- * @work: Workitem for this request
- * @rxq_index: Identifies the channel for which this request was made
- * @flow_id: Identifies the kernel-side flow for which this request was made
- */
-struct efx_async_filter_insertion {
- struct net_device *net_dev;
- struct efx_filter_spec spec;
- struct work_struct work;
- u16 rxq_index;
- u32 flow_id;
-};
-
static void efx_filter_rfs_work(struct work_struct *data)
{
struct efx_async_filter_insertion *req = container_of(data, struct efx_async_filter_insertion,
work);
struct efx_nic *efx = netdev_priv(req->net_dev);
struct efx_channel *channel = efx_get_channel(efx, req->rxq_index);
+ int slot_idx = req - efx->rps_slot;
int rc;
rc = efx->type->filter_insert(efx, &req->spec, true);
@@ -878,8 +863,8 @@ static void efx_filter_rfs_work(struct work_struct *data)
}
/* Release references */
+ clear_bit(slot_idx, &efx->rps_slot_map);
dev_put(req->net_dev);
- kfree(req);
}
int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
@@ -888,22 +873,36 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
struct efx_nic *efx = netdev_priv(net_dev);
struct efx_async_filter_insertion *req;
struct flow_keys fk;
+ int slot_idx;
+ int rc;
- if (flow_id == RPS_FLOW_ID_INVALID)
- return -EINVAL;
+ /* find a free slot */
+ for (slot_idx = 0; slot_idx < EFX_RPS_MAX_IN_FLIGHT; slot_idx++)
+ if (!test_and_set_bit(slot_idx, &efx->rps_slot_map))
+ break;
+ if (slot_idx >= EFX_RPS_MAX_IN_FLIGHT)
+ return -EBUSY;
- if (!skb_flow_dissect_flow_keys(skb, &fk, 0))
- return -EPROTONOSUPPORT;
+ if (flow_id == RPS_FLOW_ID_INVALID) {
+ rc = -EINVAL;
+ goto out_clear;
+ }
- if (fk.basic.n_proto != htons(ETH_P_IP) && fk.basic.n_proto != htons(ETH_P_IPV6))
- return -EPROTONOSUPPORT;
- if (fk.control.flags & FLOW_DIS_IS_FRAGMENT)
- return -EPROTONOSUPPORT;
+ if (!skb_flow_dissect_flow_keys(skb, &fk, 0)) {
+ rc = -EPROTONOSUPPORT;
+ goto out_clear;
+ }
- req = kmalloc(sizeof(*req), GFP_ATOMIC);
- if (!req)
- return -ENOMEM;
+ if (fk.basic.n_proto != htons(ETH_P_IP) && fk.basic.n_proto != htons(ETH_P_IPV6)) {
+ rc = -EPROTONOSUPPORT;
+ goto out_clear;
+ }
+ if (fk.control.flags & FLOW_DIS_IS_FRAGMENT) {
+ rc = -EPROTONOSUPPORT;
+ goto out_clear;
+ }
+ req = efx->rps_slot + slot_idx;
efx_filter_init_rx(&req->spec, EFX_FILTER_PRI_HINT,
efx->rx_scatter ? EFX_FILTER_FLAG_RX_SCATTER : 0,
rxq_index);
@@ -933,6 +932,9 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
req->flow_id = flow_id;
schedule_work(&req->work);
return 0;
+out_clear:
+ clear_bit(slot_idx, &efx->rps_slot_map);
+ return rc;
}
bool __efx_filter_rfs_expire(struct efx_nic *efx, unsigned int quota)
^ permalink raw reply related
* Re: [RFC bpf-next v2 4/8] bpf: add documentation for eBPF helpers (23-32)
From: Quentin Monnet @ 2018-04-13 18:18 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: daniel, ast, netdev, oss-drivers, linux-doc, linux-man
In-Reply-To: <20180413002838.atu7shp5cuubx32p@ast-mbp.dhcp.thefacebook.com>
2018-04-12 17:28 UTC-0700 ~ Alexei Starovoitov
<alexei.starovoitov@gmail.com>
> On Tue, Apr 10, 2018 at 03:41:53PM +0100, Quentin Monnet wrote:
>> Add documentation for eBPF helper functions to bpf.h user header file.
>> This documentation can be parsed with the Python script provided in
>> another commit of the patch series, in order to provide a RST document
>> that can later be converted into a man page.
>>
>> The objective is to make the documentation easily understandable and
>> accessible to all eBPF developers, including beginners.
>>
>> This patch contains descriptions for the following helper functions, all
>> written by Daniel:
>>
>> - bpf_get_prandom_u32()
>> - bpf_get_smp_processor_id()
>> - bpf_get_cgroup_classid()
>> - bpf_get_route_realm()
>> - bpf_skb_load_bytes()
>> - bpf_csum_diff()
>> - bpf_skb_get_tunnel_opt()
>> - bpf_skb_set_tunnel_opt()
>> - bpf_skb_change_proto()
>> - bpf_skb_change_type()
>>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>> ---
>> include/uapi/linux/bpf.h | 125 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 125 insertions(+)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index f3ea8824efbc..d147d9dd6a83 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
[...]
>> @@ -604,6 +612,13 @@ union bpf_attr {
>> * Return
>> * 0 on success, or a negative error in case of failure.
>> *
>> + * u32 bpf_get_cgroup_classid(struct sk_buff *skb)
>> + * Description
>> + * Retrieve the classid for the current task, i.e. for the
>> + * net_cls (network classifier) cgroup to which *skb* belongs.
>
> please add that kernel should be configured with CONFIG_NET_CLS_CGROUP=y|m
> and mention Documentation/cgroup-v1/net_cls.txt
Ok.
> Otherwise 'network classifier' is way too generic.
I am not so familiar with cgroups. What would you suggest instead?
> I'd also mention that placing a task into net_cls controller
> disables all of cgroup-bpf.
Could you please explain a bit more? Placing a task into the controller
is using:
echo <task_pid> > /sys/fs/cgroup/<my_cgroup_name>/tasks
correct? Then if I do this, it disables all of cgroup-bpf. Does this
mean that I loose the possibility to use or add BPF programs to all
cgroup-related attach points for this cgroup? I think I missed something
here.
>> + * Return
>> + * The classid, or 0 for the default unconfigured classid.
>> + *
>> * int bpf_skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>> * Description
>> * Push a *vlan_tci* (VLAN tag control information) of protocol
I have no particular comments on the other items you reported on this
patch, I will fix them. Thanks!
Quentin
^ permalink raw reply
* [next-queue PATCH 1/1] ixgbe: cleanup sparse warnings
From: cathy.zhou @ 2018-04-13 18:28 UTC (permalink / raw)
To: jeffrey.t.kirsher, intel-wired-lan; +Cc: netdev, shannon.nelson
From: Cathy Zhou <cathy.zhou@oracle.COM>
Sparse complains valid conversions between restricted types, force
attribute is used to avoid those warnings.
Signed-off-by: Cathy Zhou <cathy.zhou@oracle.COM>
Reviewed-by: Shannon Nelson <shannon.nelson@oracle.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c | 13 ++++++-----
drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 25 +++++++++++++--------
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 29 +++++++++++++++----------
drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 16 +++++++-------
drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 9 ++++----
7 files changed, 55 insertions(+), 41 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
index 66a74f4651e8..898b47b1a854 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
@@ -1462,7 +1462,8 @@ void ixgbe_atr_compute_perfect_hash_82599(union ixgbe_atr_input *input,
{
u32 hi_hash_dword, lo_hash_dword, flow_vm_vlan;
- u32 bucket_hash = 0, hi_dword = 0;
+ u32 bucket_hash = 0;
+ __be32 hi_dword = 0;
int i;
/* Apply masks to input data */
@@ -1501,7 +1502,7 @@ void ixgbe_atr_compute_perfect_hash_82599(union ixgbe_atr_input *input,
* Limit hash to 13 bits since max bucket count is 8K.
* Store result at the end of the input stream.
*/
- input->formatted.bkt_hash = bucket_hash & 0x1FFF;
+ input->formatted.bkt_hash = (__force __be16)(bucket_hash & 0x1FFF);
}
/**
@@ -1610,7 +1611,7 @@ s32 ixgbe_fdir_set_input_mask_82599(struct ixgbe_hw *hw,
return IXGBE_ERR_CONFIG;
}
- switch (input_mask->formatted.flex_bytes & 0xFFFF) {
+ switch ((__force u16)input_mask->formatted.flex_bytes & 0xFFFF) {
case 0x0000:
/* Mask Flex Bytes */
fdirm |= IXGBE_FDIRM_FLEX;
@@ -1680,13 +1681,13 @@ s32 ixgbe_fdir_write_perfect_filter_82599(struct ixgbe_hw *hw,
IXGBE_WRITE_REG(hw, IXGBE_FDIRPORT, fdirport);
/* record vlan (little-endian) and flex_bytes(big-endian) */
- fdirvlan = IXGBE_STORE_AS_BE16(input->formatted.flex_bytes);
+ fdirvlan = IXGBE_STORE_AS_BE16((__force u16)input->formatted.flex_bytes);
fdirvlan <<= IXGBE_FDIRVLAN_FLEX_SHIFT;
fdirvlan |= ntohs(input->formatted.vlan_id);
IXGBE_WRITE_REG(hw, IXGBE_FDIRVLAN, fdirvlan);
/* configure FDIRHASH register */
- fdirhash = input->formatted.bkt_hash;
+ fdirhash = (__force u32)input->formatted.bkt_hash;
fdirhash |= soft_id << IXGBE_FDIRHASH_SIG_SW_INDEX_SHIFT;
IXGBE_WRITE_REG(hw, IXGBE_FDIRHASH, fdirhash);
@@ -1724,7 +1725,7 @@ s32 ixgbe_fdir_erase_perfect_filter_82599(struct ixgbe_hw *hw,
s32 err;
/* configure FDIRHASH register */
- fdirhash = input->formatted.bkt_hash;
+ fdirhash = (__force u32)input->formatted.bkt_hash;
fdirhash |= soft_id << IXGBE_FDIRHASH_SIG_SW_INDEX_SHIFT;
IXGBE_WRITE_REG(hw, IXGBE_FDIRHASH, fdirhash);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
index 633be93f3dbb..7db2722366c2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
@@ -3652,7 +3652,7 @@ s32 ixgbe_hic_unlocked(struct ixgbe_hw *hw, u32 *buffer, u32 length,
*/
for (i = 0; i < dword_len; i++)
IXGBE_WRITE_REG_ARRAY(hw, IXGBE_FLEX_MNG,
- i, cpu_to_le32(buffer[i]));
+ i, (__force u32)cpu_to_le32(buffer[i]));
/* Setting this bit tells the ARC that a new command is pending. */
IXGBE_WRITE_REG(hw, IXGBE_HICR, hicr | IXGBE_HICR_C);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
index 7a09a40e4472..4e4c5eeda50d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
@@ -465,7 +465,7 @@ int ixgbe_fcoe_ddp(struct ixgbe_adapter *adapter,
case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):
dma_unmap_sg(&adapter->pdev->dev, ddp->sgl,
ddp->sgc, DMA_FROM_DEVICE);
- ddp->err = ddp_err;
+ ddp->err = (__force u32)ddp_err;
ddp->sgl = NULL;
ddp->sgc = 0;
/* fall through */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 68af127987bc..33e8c588ff51 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -43,8 +43,9 @@ static void ixgbe_ipsec_set_tx_sa(struct ixgbe_hw *hw, u16 idx,
int i;
for (i = 0; i < 4; i++)
- IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(i), cpu_to_be32(key[3 - i]));
- IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, cpu_to_be32(salt));
+ IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(i),
+ (__force u32)cpu_to_be32(key[3 - i]));
+ IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, (__force u32)cpu_to_be32(salt));
IXGBE_WRITE_FLUSH(hw);
reg = IXGBE_READ_REG(hw, IXGBE_IPSTXIDX);
@@ -93,7 +94,8 @@ static void ixgbe_ipsec_set_rx_sa(struct ixgbe_hw *hw, u16 idx, __be32 spi,
int i;
/* store the SPI (in bigendian) and IPidx */
- IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, cpu_to_le32(spi));
+ IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI,
+ (__force u32)cpu_to_le32((__force u32)spi));
IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, ip_idx);
IXGBE_WRITE_FLUSH(hw);
@@ -101,8 +103,9 @@ static void ixgbe_ipsec_set_rx_sa(struct ixgbe_hw *hw, u16 idx, __be32 spi,
/* store the key, salt, and mode */
for (i = 0; i < 4; i++)
- IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(i), cpu_to_be32(key[3 - i]));
- IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, cpu_to_be32(salt));
+ IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(i),
+ (__force u32)cpu_to_be32(key[3 - i]));
+ IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, (__force u32)cpu_to_be32(salt));
IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, mode);
IXGBE_WRITE_FLUSH(hw);
@@ -121,7 +124,8 @@ static void ixgbe_ipsec_set_rx_ip(struct ixgbe_hw *hw, u16 idx, __be32 addr[])
/* store the ip address */
for (i = 0; i < 4; i++)
- IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(i), cpu_to_le32(addr[i]));
+ IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(i),
+ (__force u32)cpu_to_le32((__force u32)addr[i]));
IXGBE_WRITE_FLUSH(hw);
ixgbe_ipsec_set_rx_item(hw, idx, ips_rx_ip_tbl);
@@ -391,7 +395,8 @@ static struct xfrm_state *ixgbe_ipsec_find_rx_state(struct ixgbe_ipsec *ipsec,
struct xfrm_state *ret = NULL;
rcu_read_lock();
- hash_for_each_possible_rcu(ipsec->rx_sa_list, rsa, hlist, spi)
+ hash_for_each_possible_rcu(ipsec->rx_sa_list, rsa, hlist,
+ (__force u32)spi) {
if (spi == rsa->xs->id.spi &&
((ip4 && *daddr == rsa->xs->id.daddr.a4) ||
(!ip4 && !memcmp(daddr, &rsa->xs->id.daddr.a6,
@@ -401,6 +406,7 @@ static struct xfrm_state *ixgbe_ipsec_find_rx_state(struct ixgbe_ipsec *ipsec,
xfrm_state_hold(ret);
break;
}
+ }
rcu_read_unlock();
return ret;
}
@@ -593,7 +599,7 @@ static int ixgbe_ipsec_add_sa(struct xfrm_state *xs)
/* hash the new entry for faster search in Rx path */
hash_add_rcu(ipsec->rx_sa_list, &ipsec->rx_tbl[sa_idx].hlist,
- rsa.xs->id.spi);
+ (__force u64)rsa.xs->id.spi);
} else {
struct tx_sa tsa;
@@ -677,7 +683,8 @@ static void ixgbe_ipsec_del_sa(struct xfrm_state *xs)
if (!ipsec->ip_tbl[ipi].ref_cnt) {
memset(&ipsec->ip_tbl[ipi], 0,
sizeof(struct rx_ip_sa));
- ixgbe_ipsec_set_rx_ip(hw, ipi, zerobuf);
+ ixgbe_ipsec_set_rx_ip(hw, ipi,
+ (__force __be32 *)zerobuf);
}
}
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index afadba99f7b8..ef40b226edfc 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -752,8 +752,8 @@ static void ixgbe_dump(struct ixgbe_adapter *adapter)
ring_desc = "";
pr_info("T [0x%03X] %016llX %016llX %016llX %08X %p %016llX %p%s",
i,
- le64_to_cpu(u0->a),
- le64_to_cpu(u0->b),
+ le64_to_cpu((__force __le64)u0->a),
+ le64_to_cpu((__force __le64)u0->b),
(u64)dma_unmap_addr(tx_buffer, dma),
dma_unmap_len(tx_buffer, len),
tx_buffer->next_to_watch,
@@ -864,15 +864,15 @@ static void ixgbe_dump(struct ixgbe_adapter *adapter)
/* Descriptor Done */
pr_info("RWB[0x%03X] %016llX %016llX ---------------- %p%s\n",
i,
- le64_to_cpu(u0->a),
- le64_to_cpu(u0->b),
+ le64_to_cpu((__force __le64)u0->a),
+ le64_to_cpu((__force __le64)u0->b),
rx_buffer_info->skb,
ring_desc);
} else {
pr_info("R [0x%03X] %016llX %016llX %016llX %p%s\n",
i,
- le64_to_cpu(u0->a),
- le64_to_cpu(u0->b),
+ le64_to_cpu((__force __le64)u0->a),
+ le64_to_cpu((__force __le64)u0->b),
(u64)rx_buffer_info->dma,
rx_buffer_info->skb,
ring_desc);
@@ -7801,7 +7801,7 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
/* remove payload length from inner checksum */
paylen = skb->len - l4_offset;
- csum_replace_by_diff(&l4.tcp->check, htonl(paylen));
+ csum_replace_by_diff(&l4.tcp->check, (__force __wsum)htonl(paylen));
/* update gso size and bytecount with header size */
first->gso_segs = skb_shinfo(skb)->gso_segs;
@@ -9109,7 +9109,8 @@ static int ixgbe_clsu32_build_input(struct ixgbe_fdir_filter *input,
for (j = 0; field_ptr[j].val; j++) {
if (field_ptr[j].off == off) {
- field_ptr[j].val(input, mask, val, m);
+ field_ptr[j].val(input, mask, (__force u32)val,
+ (__force u32)m);
input->filter.formatted.flow_type |=
field_ptr[j].type;
found_entry = true;
@@ -9118,8 +9119,10 @@ static int ixgbe_clsu32_build_input(struct ixgbe_fdir_filter *input,
}
if (nexthdr) {
if (nexthdr->off == cls->knode.sel->keys[i].off &&
- nexthdr->val == cls->knode.sel->keys[i].val &&
- nexthdr->mask == cls->knode.sel->keys[i].mask)
+ nexthdr->val ==
+ (__force u32)cls->knode.sel->keys[i].val &&
+ nexthdr->mask ==
+ (__force u32)cls->knode.sel->keys[i].mask)
found_jump_field = true;
else
continue;
@@ -9223,7 +9226,8 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
for (i = 0; nexthdr[i].jump; i++) {
if (nexthdr[i].o != cls->knode.sel->offoff ||
nexthdr[i].s != cls->knode.sel->offshift ||
- nexthdr[i].m != cls->knode.sel->offmask)
+ nexthdr[i].m !=
+ (__force u32)cls->knode.sel->offmask)
return err;
jump = kzalloc(sizeof(*jump), GFP_KERNEL);
@@ -9970,7 +9974,8 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
}
} else {
for (i = 0; i < adapter->num_rx_queues; i++)
- xchg(&adapter->rx_ring[i]->xdp_prog, adapter->xdp_prog);
+ (void)xchg(&adapter->rx_ring[i]->xdp_prog,
+ adapter->xdp_prog);
}
if (old_prog)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
index 72446644f9fa..57de21a299ea 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
@@ -53,8 +53,8 @@ static inline int ixgbe_mat_prgm_sip(struct ixgbe_fdir_filter *input,
union ixgbe_atr_input *mask,
u32 val, u32 m)
{
- input->filter.formatted.src_ip[0] = val;
- mask->formatted.src_ip[0] = m;
+ input->filter.formatted.src_ip[0] = (__force __be32)val;
+ mask->formatted.src_ip[0] = (__force __be32)m;
return 0;
}
@@ -62,8 +62,8 @@ static inline int ixgbe_mat_prgm_dip(struct ixgbe_fdir_filter *input,
union ixgbe_atr_input *mask,
u32 val, u32 m)
{
- input->filter.formatted.dst_ip[0] = val;
- mask->formatted.dst_ip[0] = m;
+ input->filter.formatted.dst_ip[0] = (__force __be32)val;
+ mask->formatted.dst_ip[0] = (__force __be32)m;
return 0;
}
@@ -79,10 +79,10 @@ static inline int ixgbe_mat_prgm_ports(struct ixgbe_fdir_filter *input,
union ixgbe_atr_input *mask,
u32 val, u32 m)
{
- input->filter.formatted.src_port = val & 0xffff;
- mask->formatted.src_port = m & 0xffff;
- input->filter.formatted.dst_port = val >> 16;
- mask->formatted.dst_port = m >> 16;
+ input->filter.formatted.src_port = (__force __be16)(val & 0xffff);
+ mask->formatted.src_port = (__force __be16)(m & 0xffff);
+ input->filter.formatted.dst_port = (__force __be16)(val >> 16);
+ mask->formatted.dst_port = (__force __be16)(m >> 16);
return 0;
};
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
index 3123267dfba9..336f3218177a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
@@ -898,8 +898,9 @@ static s32 ixgbe_read_ee_hostif_buffer_X550(struct ixgbe_hw *hw,
buffer.hdr.req.checksum = FW_DEFAULT_CHECKSUM;
/* convert offset from words to bytes */
- buffer.address = cpu_to_be32((offset + current_word) * 2);
- buffer.length = cpu_to_be16(words_to_read * 2);
+ buffer.address = (__force u32)cpu_to_be32((offset +
+ current_word) * 2);
+ buffer.length = (__force u16)cpu_to_be16(words_to_read * 2);
buffer.pad2 = 0;
buffer.pad3 = 0;
@@ -1109,9 +1110,9 @@ static s32 ixgbe_read_ee_hostif_X550(struct ixgbe_hw *hw, u16 offset, u16 *data)
buffer.hdr.req.checksum = FW_DEFAULT_CHECKSUM;
/* convert offset from words to bytes */
- buffer.address = cpu_to_be32(offset * 2);
+ buffer.address = (__force u32)cpu_to_be32(offset * 2);
/* one word */
- buffer.length = cpu_to_be16(sizeof(u16));
+ buffer.length = (__force u16)cpu_to_be16(sizeof(u16));
status = hw->mac.ops.acquire_swfw_sync(hw, mask);
if (status)
--
2.11.1
^ permalink raw reply related
* Re: SRIOV switchdev mode BoF minutes
From: Or Gerlitz @ 2018-04-13 20:16 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: David Miller, Anjali Singhai Jain, Andy Gospodarek, Michael Chan,
Simon Horman, Jakub Kicinski, John Fastabend, Saeed Mahameed,
Jiri Pirko, Rony Efraim, Linux Netdev List
In-Reply-To: <bec866ff-2898-72b3-247d-f64189ef6d0f@intel.com>
On Fri, Apr 13, 2018 at 7:49 PM, Samudrala, Sridhar
<sridhar.samudrala@intel.com> wrote:
> On 4/13/2018 1:57 AM, Or Gerlitz wrote:
>>> in overlay networks scheme, the uplink rep has the VTEP ip and is not connected
>>> to the bridge, e.g you use ovs you have vf reps and vxlan ports connected
>>> to ovs and the ip stack routes through the uplink rep
> This changes the legacy mode behavior of configuring vtep ip on the pf
> netdev. How does host to host traffic expected to work when vtep ip is moved to uplink rep?
What do you mean host to host traffic, is that two VFs on the same host?
control plane SWs (such as OVS) don't apply encapsulation within the same host
>>>> What about pf-rep?
> Are you planning to create a pf-rep too? Is pf also treated similar to vf in
> switchdev mode?
> All pf traffic goes to pf-rep and pf-rep traffic goes to pf by default
> without any rules programmed?
@ the sriov switchdev ARCH level, pf/pf-rep would work indeed as you described.
We will have pf rep for smartnic schemes where the the pf on the host
is not the manager of the eswitch but rather the smartnic driver instance.
on non smart env, there are some challenges to address for the pf
nic to be fully functional for the slow path (what you described), we
will get there down the road if there is a real need.
^ permalink raw reply
* Re: v6/sit tunnels and VRFs
From: Jeff Barnhill @ 2018-04-13 20:23 UTC (permalink / raw)
To: David Ahern; +Cc: netdev
In-Reply-To: <66ebbad0-68e4-ba2d-ffb6-2a8057e72b04@gmail.com>
Thanks for the response, David. I'm not questioning the need to stop
the fib lookup once the end of the VRF table is reached - I agree that
is needed. I'm concerned with the difference in the response/error
returned from the failed lookup.
For instance, with vrf "unreachable default" route, I get this:
# ip route get 1.1.1.1 vrf vrf_258
RTNETLINK answers: No route to host
Without it (and assuming no match for 1.1.1.1 in local/main/default
tables), I get this:
# ip route get 1.1.1.1 vrf vrf_258
RTNETLINK answers: Network is unreachable
Which is also what happens when not using VRFs at all.
It seems that the ENETUNREACH response is still desirable in the VRF
case since the only difference (when using VRF vs. not) is that the
lookup should be restrained to a specific VRF.
Jeff
On Thu, Apr 12, 2018 at 10:25 PM, David Ahern <dsahern@gmail.com> wrote:
> On 4/12/18 10:54 AM, Jeff Barnhill wrote:
>> Hi David,
>>
>> In the slides referenced, you recommend adding an "unreachable
>> default" route to the end of each VRF route table. In my testing (for
>> v4) this results in a change to fib lookup failures such that instead
>> of ENETUNREACH being returned, EHOSTUNREACH is returned since the fib
>> finds the unreachable route, versus failing to find a route
>> altogether.
>>
>> Have the implications of this been considered? I don't see a
>> clean/easy way to achieve the old behavior without affecting non-VRF
>> routing (eg. remove the unreachable route and delete the non-VRF
>> rules). I'm guessing that programmatically, it may not make much
>> difference, ie. lookup fails, but for debugging or to a user looking
>> at it, the difference matters. Do you (or anyone else) have any
>> thoughts on this?
>
> We have recommended moving the local table down in the FIB rules:
>
> # ip ru ls
> 1000: from all lookup [l3mdev-table]
> 32765: from all lookup local
> 32766: from all lookup main
> 32767: from all lookup default
>
> and adding a default route to VRF tables:
>
> # ip ro ls vrf red
> unreachable default metric 4278198272
> 172.16.2.0/24 proto bgp metric 20
> nexthop via 169.254.0.1 dev swp3 weight 1 onlink
> nexthop via 169.254.0.1 dev swp4 weight 1 onlink
>
> # ip -6 ro ls vrf red
> 2001:db8:2::/64 proto bgp metric 20
> nexthop via fe80::202:ff:fe00:e dev swp3 weight 1
> nexthop via fe80::202:ff:fe00:f dev swp4 weight 1
> anycast fe80:: dev lo proto kernel metric 0 pref medium
> anycast fe80:: dev lo proto kernel metric 0 pref medium
> fe80::/64 dev swp3 proto kernel metric 256 pref medium
> fe80::/64 dev swp4 proto kernel metric 256 pref medium
> ff00::/8 dev swp3 metric 256 pref medium
> ff00::/8 dev swp4 metric 256 pref medium
> unreachable default dev lo metric 4278198272 error -101 pref medium
>
> Over the last 2 years we have not seen any negative side effects to this
> and is what you want to have proper VRF separation.
>
> Without a default route lookups will proceed to the next fib rule which
> means a lookup in the next table and barring other PBR rules will be the
> main table. It will lead to wrong lookups.
>
> Here is an example:
> ip netns add foo
> ip netns exec foo bash
> ip li set lo up
> ip li add red type vrf table 123
> ip li set red up
> ip li add dummy1 type dummy
> ip addr add 10.100.1.1/24 dev dummy1
> ip li set dummy1 master red
> ip li set dummy1 up
> ip li add dummy2 type dummy
> ip addr add 10.100.1.1/24 dev dummy2
> ip li set dummy2 up
> ip ro get 10.100.2.2
> ip ro get 10.100.2.2 vrf red
>
> # ip ru ls
> 0: from all lookup local
> 1000: from all lookup [l3mdev-table]
> 32766: from all lookup main
> 32767: from all lookup default
>
> # ip ro ls
> 10.100.1.0/24 dev dummy2 proto kernel scope link src 10.100.1.1
> 10.100.2.0/24 via 10.100.1.2 dev dummy2
>
> # ip ro ls vrf red
> 10.100.1.0/24 dev dummy1 proto kernel scope link src 10.100.1.1
>
> That's the setup. What happens on route lookups?
> # ip ro get vrf red 10.100.2.1
> 10.100.2.1 via 10.100.1.2 dev dummy2 src 10.100.1.1 uid 0
> cache
>
> which is clearly wrong. Let's look at the lookup sequence
>
> # perf record -e fib:* ip ro get vrf red 10.100.2.1
> 10.100.2.1 via 10.100.1.2 dev dummy2 src 10.100.1.1 uid 0
> cache
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.003 MB perf.data (4 samples) ]
>
> # perf script --fields trace:trace
> table 255 oif 2 iif 1 src 0.0.0.0 dst 10.100.2.1 tos 0 scope 0 flags 4
> table 123 oif 2 iif 1 src 0.0.0.0 dst 10.100.2.1 tos 0 scope 0 flags 4
> table 254 oif 2 iif 1 src 0.0.0.0 dst 10.100.2.1 tos 0 scope 0 flags 4
> nexthop dev dummy2 oif 4 src 10.100.1.1
>
> The first one is because I did not move the local table down.
> The second one is the correct vrf lookup
> The third one is the continuation to the next table - the main table.
>
> Adding a default route:
> # ip ro add vrf red unreachable default
>
> And the lookup is proper:
> # ip ro get vrf red 10.100.2.1
> RTNETLINK answers: No route to host
^ permalink raw reply
* Re: v6/sit tunnels and VRFs
From: David Ahern @ 2018-04-13 20:31 UTC (permalink / raw)
To: Jeff Barnhill; +Cc: netdev
In-Reply-To: <CAL6e_pdihSXvYFQQ0MAXFtw=eUjJPfR9kS9j5QQNSqCNzdQteg@mail.gmail.com>
On 4/13/18 2:23 PM, Jeff Barnhill wrote:
> It seems that the ENETUNREACH response is still desirable in the VRF
> case since the only difference (when using VRF vs. not) is that the
> lookup should be restrained to a specific VRF.
VRF is just policy routing to a table. If the table wants the lookup to
stop, then it needs a default route. What you are referring to is the
lookup goes through all tables and does not find an answer so it fails
with -ENETUNREACH. I do not know of any way to make that happen with the
existing default route options and in the past 2+ years we have not hit
any s/w that discriminates -ENETUNREACH from -EHOSTUNREACH.
I take it this is code from your internal code base. Why does it care
between those two failures?
^ permalink raw reply
* Re: [PATCH] x86/cpufeature: guard asm_volatile_goto usage with CC_HAVE_ASM_GOTO
From: Alexei Starovoitov @ 2018-04-13 20:42 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Yonghong Song, mingo, daniel, linux-kernel, x86, kernel-team,
Thomas Gleixner, netdev, Jesper Dangaard Brouer
In-Reply-To: <20180413181924.GT4064@hirez.programming.kicks-ass.net>
On 4/13/18 11:19 AM, Peter Zijlstra wrote:
> On Tue, Apr 10, 2018 at 02:28:04PM -0700, Alexei Starovoitov wrote:
>> Instead of
>> #ifdef CC_HAVE_ASM_GOTO
>> we can replace it with
>> #ifndef __BPF__
>> or some other name,
>
> I would prefer the BPF specific hack; otherwise we might be encouraging
> people to build the kernel proper without asm-goto.
>
I don't understand this concern.
1.
arch/x86/Makefile does
ifndef CC_HAVE_ASM_GOTO
$(error Compiler lacks asm-goto support.)
endif
which is pretty strong statement of the kernel direction.
2.
Even with this patch that adds #ifdef CC_HAVE_ASM_GOTO back
the x86 arch still needs asm-goto in the compiler to be built.
As far as I can see there are other places where asm-goto
is open coded.
So there is no 'encouraging'.
Whereas if we do bpf specific marco we'd need to explain that
to all bpf users and they would need to fix their user space scripts.
Amount of user space breakage is unknown at this point.
^ permalink raw reply
* Panic since 4.15.15 in tcp_retransmit_timer when doing ss -K
From: Sami Farin @ 2018-04-13 20:46 UTC (permalink / raw)
To: Linux Networking Mailing List
I started getting this since 4.15.15. It's easy to trigger,
for example I get new IP address via dhcp (NetworkManager),
then ss -K the_old_ip_address .
Happens on Ryzen and SandyBridge systems.
My guess of the cause: commit 960058fe196397aecb16bb14e64980e265d2bc5e
(didn't try reverting)
BUG: unable to handle kernel NULL pointer dereference at 000030
IP: tcp_retransmit_skb+0x57/0xc0
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP NOPTI
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 4.15.17+ #42
Hardware name: To Be Filled By O.E.M./To Be Filled By O.E.M./X370 Taichi, BIOS P4.60 03/02/2018
RIP: 0010:tcp_retransmit_skb+0x57/0xc0
RSP: 0018:ffff95b1dea03e00 EFLAGS: 00010206
RAX: 00000000fffffff5 RBX: ffff95b15876d000 RCX: 5
RDX: 5 RSI: 0 RDI: ffff95b115876d000
...
Call Trace:
<IRQ>
tcp_retransmit_timer
tcp_write_timer_handler
tcp_write_timer
? tcp_write_timer_handler
expire_timers
run_timer_softirq
sched_clock
sched_clock
sched_clock_cpu
irqtime_account_irq
__do_softirq
sched_clock
irq_exit
smp_apic_timer_interrupt
apic_timer_interrupt
</IRQ>
--
Do what you love because life is too short for anything else.
^ permalink raw reply
* [PATCH iproute2-next 1/1] tc: jsonify ife action
From: Roman Mashak @ 2018-04-13 21:40 UTC (permalink / raw)
To: dsahern; +Cc: stephen, netdev, kernel, jhs, xiyou.wangcong, jiri, Roman Mashak
Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
tc/m_ife.c | 54 ++++++++++++++++++++++++++++++++----------------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/tc/m_ife.c b/tc/m_ife.c
index d7e61703f666..15d09a167450 100644
--- a/tc/m_ife.c
+++ b/tc/m_ife.c
@@ -240,22 +240,24 @@ static int print_ife(struct action_util *au, FILE *f, struct rtattr *arg)
parse_rtattr_nested(tb, TCA_IFE_MAX, arg);
if (tb[TCA_IFE_PARMS] == NULL) {
- fprintf(f, "[NULL ife parameters]");
+ print_string(PRINT_FP, NULL, "%s", "[NULL ife parameters]");
return -1;
}
p = RTA_DATA(tb[TCA_IFE_PARMS]);
- fprintf(f, "ife %s ", p->flags & IFE_ENCODE ? "encode" : "decode");
+ print_string(PRINT_ANY, "kind", "%s ", "ife");
+ print_string(PRINT_ANY, "mode", "%s",
+ p->flags & IFE_ENCODE ? "encode" : "decode");
print_action_control(f, "action ", p->action, " ");
if (tb[TCA_IFE_TYPE]) {
ife_type = rta_getattr_u16(tb[TCA_IFE_TYPE]);
has_optional = 1;
- fprintf(f, "type 0x%X ", ife_type);
+ print_0xhex(PRINT_ANY, "type", "type 0x%X ", ife_type);
}
if (has_optional)
- fprintf(f, "\n\t ");
+ print_string(PRINT_FP, NULL, "%s\t", _SL_);
if (tb[TCA_IFE_METALST]) {
struct rtattr *metalist[IFE_META_MAX + 1];
@@ -268,9 +270,11 @@ static int print_ife(struct action_util *au, FILE *f, struct rtattr *arg)
len = RTA_PAYLOAD(metalist[IFE_META_SKBMARK]);
if (len) {
mmark = rta_getattr_u32(metalist[IFE_META_SKBMARK]);
- fprintf(f, "use mark %u ", mmark);
+ print_uint(PRINT_ANY, "mark", "use mark %u ",
+ mmark);
} else
- fprintf(f, "allow mark ");
+ print_string(PRINT_ANY, "mark", "%s mark ",
+ "allow");
}
if (metalist[IFE_META_TCINDEX]) {
@@ -278,41 +282,47 @@ static int print_ife(struct action_util *au, FILE *f, struct rtattr *arg)
if (len) {
mtcindex =
rta_getattr_u16(metalist[IFE_META_TCINDEX]);
- fprintf(f, "use tcindex %d ", mtcindex);
+ print_uint(PRINT_ANY, "tcindex",
+ "use tcindex %u ", mtcindex);
} else
- fprintf(f, "allow tcindex ");
+ print_string(PRINT_ANY, "tcindex",
+ "%s tcindex ", "allow");
}
if (metalist[IFE_META_PRIO]) {
len = RTA_PAYLOAD(metalist[IFE_META_PRIO]);
if (len) {
mprio = rta_getattr_u32(metalist[IFE_META_PRIO]);
- fprintf(f, "use prio %u ", mprio);
+ print_uint(PRINT_ANY, "prio", "use prio %u ",
+ mprio);
} else
- fprintf(f, "allow prio ");
+ print_string(PRINT_ANY, "prio", "%s prio ",
+ "allow");
}
}
if (tb[TCA_IFE_DMAC]) {
has_optional = 1;
- fprintf(f, "dst %s ",
- ll_addr_n2a(RTA_DATA(tb[TCA_IFE_DMAC]),
- RTA_PAYLOAD(tb[TCA_IFE_DMAC]), 0, b2,
- sizeof(b2)));
-
+ print_string(PRINT_ANY, "dst", "dst %s ",
+ ll_addr_n2a(RTA_DATA(tb[TCA_IFE_DMAC]),
+ RTA_PAYLOAD(tb[TCA_IFE_DMAC]), 0, b2,
+ sizeof(b2)));
}
if (tb[TCA_IFE_SMAC]) {
has_optional = 1;
- fprintf(f, "src %s ",
- ll_addr_n2a(RTA_DATA(tb[TCA_IFE_SMAC]),
- RTA_PAYLOAD(tb[TCA_IFE_SMAC]), 0, b2,
- sizeof(b2)));
+ print_string(PRINT_ANY, "src", "src %s ",
+ ll_addr_n2a(RTA_DATA(tb[TCA_IFE_SMAC]),
+ RTA_PAYLOAD(tb[TCA_IFE_SMAC]), 0, b2,
+ sizeof(b2)));
}
- fprintf(f, "\n\t index %u ref %d bind %d", p->index, p->refcnt,
- p->bindcnt);
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+ print_uint(PRINT_ANY, "index", "\t index %u", p->index);
+ print_int(PRINT_ANY, "ref", " ref %d", p->refcnt);
+ print_int(PRINT_ANY, "bind", " bind %d", p->bindcnt);
+
if (show_stats) {
if (tb[TCA_IFE_TM]) {
struct tcf_t *tm = RTA_DATA(tb[TCA_IFE_TM]);
@@ -321,7 +331,7 @@ static int print_ife(struct action_util *au, FILE *f, struct rtattr *arg)
}
}
- fprintf(f, "\n");
+ print_string(PRINT_FP, NULL, "%s", _SL_);
return 0;
}
--
2.7.4
^ permalink raw reply related
* Cavium Octeon III network driver.
From: Steven J. Hill @ 2018-04-13 22:43 UTC (permalink / raw)
To: netdev
Patches for Cavium's Octeon III network driver were submitted by
David Daney back on 20180222. David has since left the company and
I am now responsible for the upstreaming effort. When looking at
<pachwork.ozlabs.org> they are marked as "Not Applicable". What
steps do I take next? Thanks.
Steve
^ permalink raw reply
* Re: [PATCH iproute2-next 3/3] treewide: Use addattr_nest()/addattr_nest_end() to handle nested attributes
From: Vinicius Costa Gomes @ 2018-04-13 22:57 UTC (permalink / raw)
To: Serhey Popovych, netdev
In-Reply-To: <1517386508-10915-4-git-send-email-serhe.popovych@gmail.com>
Hi,
Serhey Popovych <serhe.popovych@gmail.com> writes:
[...]
> diff --git a/tc/q_mqprio.c b/tc/q_mqprio.c
> index 89b4600..207d644 100644
> --- a/tc/q_mqprio.c
> +++ b/tc/q_mqprio.c
> @@ -173,8 +173,7 @@ static int mqprio_parse_opt(struct qdisc_util *qu, int argc,
> argc--; argv++;
> }
>
> - tail = NLMSG_TAIL(n);
> - addattr_l(n, 1024, TCA_OPTIONS, &opt, sizeof(opt));
> + tail = addattr_nest_compat(n, 1024, TCA_OPTIONS, &opt, sizeof(opt));
>
> if (flags & TC_MQPRIO_F_MODE)
> addattr_l(n, 1024, TCA_MQPRIO_MODE,
> @@ -209,7 +208,7 @@ static int mqprio_parse_opt(struct qdisc_util *qu, int argc,
> addattr_nest_end(n, start);
> }
>
> - tail->rta_len = (void *)NLMSG_TAIL(n) - (void *)tail;
> + addattr_nest_compat_end(n, tail);
>
> return 0;
> }
Sorry if I am too late, but this breaks mqprio, i.e. something like
this:
$ tc qdisc replace dev enp2s0 handle 100: parent root mqprio \
num_tc 3 map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \
queues 1@0 1@1 2@2 hw 0
that used to work, now doesn't.
This patch looks right, so I thought that it could be possible that mqprio
(in the kernel side) was making some wrong assumptions about the format
of the messages.
And after some investigation, what seems to be happening is something
like this (not too familiar with netlink protocol internals, I may be
missing something).
In the "wire", after this patch, the mqprio part of message may be
represented as:
/* The message format is [ len | type | payload ] */
[ S | 2 | <S bytes> ]
[ 0 | 2 | ]
Some notes:
- S is the aligned value of sizeof(opt);
- The value of TCA_OPTIONS is 2;
Before this patch, I think it was something like:
[ S | 2 | <S bytes> ]
The problem is that mqprio defines an internal type with the same value
as TCA_OPTIONS (2), and that finalizing (empty) is interpreted as the
"internal" field instead of indicating the end of TCA_OPTIONS, which
causes a size mismatch with 'mqprio_policy', causing the command to
create a mqprio qdisc to fail.
In short, I think that replacing the "open coded" version with
addattr_nest_compat() is not a functionally equivalent change.
Cheers,
^ permalink raw reply
* Re: SRIOV switchdev mode BoF minutes
From: Samudrala, Sridhar @ 2018-04-13 23:03 UTC (permalink / raw)
To: Or Gerlitz
Cc: David Miller, Anjali Singhai Jain, Andy Gospodarek, Michael Chan,
Simon Horman, Jakub Kicinski, John Fastabend, Saeed Mahameed,
Jiri Pirko, Rony Efraim, Linux Netdev List
In-Reply-To: <CAJ3xEMiDundB9p0CvnQ33mLm=7OvG4Yvd5ASH9uR+TkCd_uJhg@mail.gmail.com>
On 4/13/2018 1:16 PM, Or Gerlitz wrote:
> On Fri, Apr 13, 2018 at 7:49 PM, Samudrala, Sridhar
> <sridhar.samudrala@intel.com> wrote:
>> On 4/13/2018 1:57 AM, Or Gerlitz wrote:
>
>>>> in overlay networks scheme, the uplink rep has the VTEP ip and is not connected
>>>> to the bridge, e.g you use ovs you have vf reps and vxlan ports connected
>>>> to ovs and the ip stack routes through the uplink rep
>> This changes the legacy mode behavior of configuring vtep ip on the pf
>> netdev. How does host to host traffic expected to work when vtep ip is moved to uplink rep?
> What do you mean host to host traffic, is that two VFs on the same host?
> control plane SWs (such as OVS) don't apply encapsulation within the same host
I meant between PFs on 2 compute nodes.
>
>>>>> What about pf-rep?
>> Are you planning to create a pf-rep too? Is pf also treated similar to vf in
>> switchdev mode?
>> All pf traffic goes to pf-rep and pf-rep traffic goes to pf by default
>> without any rules programmed?
> @ the sriov switchdev ARCH level, pf/pf-rep would work indeed as you described.
>
> We will have pf rep for smartnic schemes where the the pf on the host
> is not the manager of the eswitch but rather the smartnic driver instance.
>
> on non smart env, there are some challenges to address for the pf
> nic to be fully functional for the slow path (what you described), we
> will get there down the road if there is a real need.
So on non-smart env, are you planning to only expose uplink rep and vf reps as netdevs.
By smartnic env, i guess you are referring to OVS control plane also running on the NIC.
I will look forward to your patches.
Thanks
Sridhar
^ permalink raw reply
* RE: [Resend Patch 3/3] Storvsc: Select channel based on available percentage of ring buffer to write
From: Michael Kelley (EOSG) @ 2018-04-13 23:28 UTC (permalink / raw)
To: Long Li, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
James E . J . Bottomley, Martin K . Petersen,
devel@linuxdriverproject.org, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <20180328004840.22787-3-longli@linuxonhyperv.com>
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf
> Of Long Li
> Sent: Tuesday, March 27, 2018 5:49 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; James E . J . Bottomley <JBottomley@odin.com>;
> Martin K . Petersen <martin.petersen@oracle.com>; devel@linuxdriverproject.org; linux-
> scsi@vger.kernel.org; linux-kernel@vger.kernel.org; netdev@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>
> Subject: [Resend Patch 3/3] Storvsc: Select channel based on available percentage of ring
> buffer to write
>
> From: Long Li <longli@microsoft.com>
>
> This is a best effort for estimating on how busy the ring buffer is for
> that channel, based on available buffer to write in percentage. It is still
> possible that at the time of actual ring buffer write, the space may not be
> available due to other processes may be writing at the time.
>
> Selecting a channel based on how full it is can reduce the possibility that
> a ring buffer write will fail, and avoid the situation a channel is over
> busy.
>
> Now it's possible that storvsc can use a smaller ring buffer size
> (e.g. 40k bytes) to take advantage of cache locality.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
> drivers/scsi/storvsc_drv.c | 62 +++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 50 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index a2ec0bc9e9fa..b1a87072b3ab 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -395,6 +395,12 @@ MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer size
> (bytes)");
>
> module_param(storvsc_vcpus_per_sub_channel, int, S_IRUGO);
> MODULE_PARM_DESC(storvsc_vcpus_per_sub_channel, "Ratio of VCPUs to subchannels");
> +
> +static int ring_avail_percent_lowater = 10;
Reserving 10% of each ring buffer by default seems like more than is needed
in the storvsc driver. That would be about 4Kbytes for the 40K ring buffer
you suggest, and even more for a ring buffer of 128K. Each outgoing record
is only about 344 bytes (I'd have to check exactly). With the new channel
selection algorithm below, the only time we use a channel that is already
below the low water mark is when no channel could be found that is above
the low water mark. There could be a case of two or more threads deciding
that a channel is above the low water mark at the same time and both
choosing it, but that's likely to be rare. So it seems like we could set the
default low water mark to 5 percent or even 3 percent, which will let more
of the ring buffer be used, and let a channel be assigned according to the
algorithm, rather than falling through to the default because all channels
appear to be "full".
> +module_param(ring_avail_percent_lowater, int, S_IRUGO);
> +MODULE_PARM_DESC(ring_avail_percent_lowater,
> + "Select a channel if available ring size > this in percent");
> +
> /*
> * Timeout in seconds for all devices managed by this driver.
> */
> @@ -1285,9 +1291,9 @@ static int storvsc_do_io(struct hv_device *device,
> {
> struct storvsc_device *stor_device;
> struct vstor_packet *vstor_packet;
> - struct vmbus_channel *outgoing_channel;
> + struct vmbus_channel *outgoing_channel, *channel;
> int ret = 0;
> - struct cpumask alloced_mask;
> + struct cpumask alloced_mask, other_numa_mask;
> int tgt_cpu;
>
> vstor_packet = &request->vstor_packet;
> @@ -1301,22 +1307,53 @@ static int storvsc_do_io(struct hv_device *device,
> /*
> * Select an an appropriate channel to send the request out.
> */
> -
> if (stor_device->stor_chns[q_num] != NULL) {
> outgoing_channel = stor_device->stor_chns[q_num];
> - if (outgoing_channel->target_cpu == smp_processor_id()) {
> + if (outgoing_channel->target_cpu == q_num) {
> /*
> * Ideally, we want to pick a different channel if
> * available on the same NUMA node.
> */
> cpumask_and(&alloced_mask, &stor_device->alloced_cpus,
> cpumask_of_node(cpu_to_node(q_num)));
> - for_each_cpu_wrap(tgt_cpu, &alloced_mask,
> - outgoing_channel->target_cpu + 1) {
> - if (tgt_cpu != outgoing_channel->target_cpu) {
> - outgoing_channel =
> - stor_device->stor_chns[tgt_cpu];
> - break;
> +
> + for_each_cpu_wrap(tgt_cpu, &alloced_mask, q_num + 1) {
> + if (tgt_cpu == q_num)
> + continue;
> + channel = stor_device->stor_chns[tgt_cpu];
> + if (hv_get_avail_to_write_percent(
> + &channel->outbound)
> + > ring_avail_percent_lowater) {
> + outgoing_channel = channel;
> + goto found_channel;
> + }
> + }
> +
> + /*
> + * All the other channels on the same NUMA node are
> + * busy. Try to use the channel on the current CPU
> + */
> + if (hv_get_avail_to_write_percent(
> + &outgoing_channel->outbound)
> + > ring_avail_percent_lowater)
> + goto found_channel;
> +
> + /*
> + * If we reach here, all the channels on the current
> + * NUMA node are busy. Try to find a channel in
> + * other NUMA nodes
> + */
> + cpumask_andnot(&other_numa_mask,
> + &stor_device->alloced_cpus,
> + cpumask_of_node(cpu_to_node(q_num)));
> +
> + for_each_cpu(tgt_cpu, &other_numa_mask) {
> + channel = stor_device->stor_chns[tgt_cpu];
> + if (hv_get_avail_to_write_percent(
> + &channel->outbound)
> + > ring_avail_percent_lowater) {
> + outgoing_channel = channel;
> + goto found_channel;
> }
> }
> }
> @@ -1324,7 +1361,7 @@ static int storvsc_do_io(struct hv_device *device,
> outgoing_channel = get_og_chn(stor_device, q_num);
> }
>
> -
> +found_channel:
> vstor_packet->flags |= REQUEST_COMPLETION_FLAG;
>
> vstor_packet->vm_srb.length = (sizeof(struct vmscsi_request) -
> @@ -1733,7 +1770,8 @@ static int storvsc_probe(struct hv_device *device,
> }
>
> scsi_driver.can_queue = (max_outstanding_req_per_channel *
> - (max_sub_channels + 1));
> + (max_sub_channels + 1)) *
> + (100 - ring_avail_percent_lowater) / 100;
A minor nit, but the use of parentheses here is inconsistent. There's a
set of parens around the first two expressions to explicitly code the
associativity, but not a set to encompass the third term, which must
be processed before the fourth one is. C does multiplication and
division with left to right associativity, so the result is as intended.
But if we're depending on C's default associativity, then that set of
parens around the first two expression really isn't needed, and one
wonders why they are there.
Michael
>
> host = scsi_host_alloc(&scsi_driver,
> sizeof(struct hv_host_device));
> --
> 2.14.1
^ permalink raw reply
* RE: [Resend Patch 2/3] Netvsc: Use the vmbus functiton to calculate ring buffer percentage
From: Michael Kelley (EOSG) @ 2018-04-13 23:39 UTC (permalink / raw)
To: Long Li, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
James E . J . Bottomley, Martin K . Petersen,
devel@linuxdriverproject.org, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <20180328004840.22787-2-longli@linuxonhyperv.com>
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf
> Of Long Li
> Sent: Tuesday, March 27, 2018 5:49 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; James E . J . Bottomley <JBottomley@odin.com>;
> Martin K . Petersen <martin.petersen@oracle.com>; devel@linuxdriverproject.org; linux-
> scsi@vger.kernel.org; linux-kernel@vger.kernel.org; netdev@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>
> Subject: [Resend Patch 2/3] Netvsc: Use the vmbus functiton to calculate ring buffer
> percentage
>
> From: Long Li <longli@microsoft.com>
>
> In Vmbus, we have defined a function to calculate available ring buffer
> percentage to write.
>
> Use that function and remove netvsc's private version.
>
> Signed-off-by: Long Li <longli@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> ---
> drivers/net/hyperv/hyperv_net.h | 1 -
> drivers/net/hyperv/netvsc.c | 17 +++--------------
> drivers/net/hyperv/netvsc_drv.c | 3 ---
> 3 files changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index cd538d5a7986..a0199ab13d67 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -189,7 +189,6 @@ struct netvsc_device;
> struct net_device_context;
>
> extern u32 netvsc_ring_bytes;
> -extern struct reciprocal_value netvsc_ring_reciprocal;
>
> struct netvsc_device *netvsc_device_add(struct hv_device *device,
> const struct netvsc_device_info *info);
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 0265d703eb03..8af0069e4d8c 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -31,7 +31,6 @@
> #include <linux/vmalloc.h>
> #include <linux/rtnetlink.h>
> #include <linux/prefetch.h>
> -#include <linux/reciprocal_div.h>
>
> #include <asm/sync_bitops.h>
>
> @@ -590,17 +589,6 @@ void netvsc_device_remove(struct hv_device *device)
> #define RING_AVAIL_PERCENT_HIWATER 20
> #define RING_AVAIL_PERCENT_LOWATER 10
>
> -/*
> - * Get the percentage of available bytes to write in the ring.
> - * The return value is in range from 0 to 100.
> - */
> -static u32 hv_ringbuf_avail_percent(const struct hv_ring_buffer_info *ring_info)
> -{
> - u32 avail_write = hv_get_bytes_to_write(ring_info);
> -
> - return reciprocal_divide(avail_write * 100, netvsc_ring_reciprocal);
> -}
> -
> static inline void netvsc_free_send_slot(struct netvsc_device *net_device,
> u32 index)
> {
> @@ -649,7 +637,8 @@ static void netvsc_send_tx_complete(struct netvsc_device
> *net_device,
> wake_up(&net_device->wait_drain);
>
> if (netif_tx_queue_stopped(netdev_get_tx_queue(ndev, q_idx)) &&
> - (hv_ringbuf_avail_percent(&channel->outbound) >
> RING_AVAIL_PERCENT_HIWATER ||
> + (hv_get_avail_to_write_percent(&channel->outbound) >
> + RING_AVAIL_PERCENT_HIWATER ||
> queue_sends < 1)) {
> netif_tx_wake_queue(netdev_get_tx_queue(ndev, q_idx));
> ndev_ctx->eth_stats.wake_queue++;
> @@ -757,7 +746,7 @@ static inline int netvsc_send_pkt(
> struct netdev_queue *txq = netdev_get_tx_queue(ndev, packet->q_idx);
> u64 req_id;
> int ret;
> - u32 ring_avail = hv_ringbuf_avail_percent(&out_channel->outbound);
> + u32 ring_avail = hv_get_avail_to_write_percent(&out_channel->outbound);
>
> nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT;
> if (skb)
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index faea0be18924..b0b1c2fd2b7b 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -35,7 +35,6 @@
> #include <linux/slab.h>
> #include <linux/rtnetlink.h>
> #include <linux/netpoll.h>
> -#include <linux/reciprocal_div.h>
>
> #include <net/arp.h>
> #include <net/route.h>
> @@ -55,7 +54,6 @@ static unsigned int ring_size __ro_after_init = 128;
> module_param(ring_size, uint, S_IRUGO);
> MODULE_PARM_DESC(ring_size, "Ring buffer size (# of pages)");
> unsigned int netvsc_ring_bytes __ro_after_init;
> -struct reciprocal_value netvsc_ring_reciprocal __ro_after_init;
>
> static const u32 default_msg = NETIF_MSG_DRV | NETIF_MSG_PROBE |
> NETIF_MSG_LINK | NETIF_MSG_IFUP |
> @@ -2186,7 +2184,6 @@ static int __init netvsc_drv_init(void)
> ring_size);
> }
> netvsc_ring_bytes = ring_size * PAGE_SIZE;
> - netvsc_ring_reciprocal = reciprocal_value(netvsc_ring_bytes);
>
> ret = vmbus_driver_register(&netvsc_drv);
> if (ret)
> --
> 2.14.1
^ 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