* Re: [PATCH bpf-next 0/8] bpf: TCP RTT sock_ops bpf callback
From: Soheil Hassas Yeganeh @ 2019-07-01 21:03 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, bpf, David Miller, Alexei Starovoitov, Daniel Borkmann,
Eric Dumazet, Priyaranjan Jha, Yuchung Cheng
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
On Mon, Jul 1, 2019 at 4:48 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Congestion control team would like to have a periodic callback to
> track some TCP statistics. Let's add a sock_ops callback that can be
> selectively enabled on a socket by socket basis and is executed for
> every RTT. BPF program frequency can be further controlled by calling
> bpf_ktime_get_ns and bailing out early.
>
> I run neper tcp_stream and tcp_rr tests with the sample program
> from the last patch and didn't observe any noticeable performance
> difference.
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Priyaranjan Jha <priyarjha@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Thank you for the nice patch series!
> Stanislav Fomichev (8):
> bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
> bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
> bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
> bpf: add icsk_retransmits to bpf_tcp_sock
> bpf/tools: sync bpf.h
> selftests/bpf: test BPF_SOCK_OPS_RTT_CB
> samples/bpf: add sample program that periodically dumps TCP stats
> samples/bpf: fix tcp_bpf.readme detach command
>
> include/net/tcp.h | 8 +
> include/uapi/linux/bpf.h | 12 +-
> net/core/filter.c | 207 +++++++++++-----
> net/ipv4/tcp_input.c | 4 +
> samples/bpf/Makefile | 1 +
> samples/bpf/tcp_bpf.readme | 2 +-
> samples/bpf/tcp_dumpstats_kern.c | 65 +++++
> tools/include/uapi/linux/bpf.h | 12 +-
> tools/testing/selftests/bpf/Makefile | 3 +-
> tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
> tools/testing/selftests/bpf/test_tcp_rtt.c | 253 ++++++++++++++++++++
> 11 files changed, 570 insertions(+), 58 deletions(-)
> create mode 100644 samples/bpf/tcp_dumpstats_kern.c
> create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
> create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: santosh.shilimkar @ 2019-07-01 21:00 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <01c251f4-c8f8-fcb8-bccc-341d4a3db90a@oracle.com>
On 7/1/19 1:55 PM, Gerd Rausch wrote:
> Hi Santosh,
>
> On 01/07/2019 13.41, santosh.shilimkar@oracle.com wrote:
>>> @@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>>> if (printk_ratelimit())
>>> pr_warn("RDS/IB: %s returned error(%d)\n",
>>> __func__, ret);
>>> + goto out;
>>> + }
>>> +
>>> + if (!frmr->fr_reg)
>>> + goto out;
>>> +
>>> + /* Wait for the registration to complete in order to prevent an invalid
>>> + * access error resulting from a race between the memory region already
>>> + * being accessed while registration is still pending.
>>> + */
>>> + wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
>>> + msecs_to_jiffies(100));
>>> +
>> This arbitrary timeout in this patch as well as pacth 1/7 which
>> Dave pointed out has any logic ?
>>
>
> It's empirical (see my response to David's question):
> Memory registrations took longer than invalidations, hence 100msec instead of 10msec.
>
>> MR registration command issued to hardware can at times take as
>> much as command timeout(e.g 60 seconds in CX3) and upto that its still
>> legitimate operation and not necessary failure. We shouldn't add
>> arbitrary time outs in ULPs.
>
> Where did you find the 60 seconds for CX3 you are referring to?
> Is there a "generic" upper-bound that is not tied to a specific vendor / HCA?
> Can you provide a pointer?
>
Look for command timeout in CX3 sources. 60 second is upper bound in
CX3. Its not standard in specs(at least not that I know) though
and may vary from vendor to vendor.
Regards,
Santosh
^ permalink raw reply
* Re: [PATCH net-next 1/7] net/rds: Give fr_state a chance to transition to FRMR_IS_FREE
From: Gerd Rausch @ 2019-07-01 20:58 UTC (permalink / raw)
To: santosh.shilimkar, David Miller; +Cc: netdev
In-Reply-To: <95d566af-30dc-fecc-9a1b-3c8c7d69b880@oracle.com>
Hi Santosh,
On 01/07/2019 13.53, santosh.shilimkar@oracle.com wrote:
> LOCAL_INV/REG etc are all end being HCA commands and the command timeouts are large. 60 seconds is what CX3 HCA has for example.
> Thats the worst case timeout from HCA before marking the command
> to be timeout and hence the operation to be failed.
>
It's a tradeoff between waiting for 60 seconds or just putting that memory-region on a drop_list.
IMO, penalizing an application with an up-to 60 seconds wait time is not necessarily the better of the 2 options.
Thanks,
Gerd
PS: If you've got a pointer to the 60 seconds CX3 HCA timeout, please share.
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: Gerd Rausch @ 2019-07-01 20:55 UTC (permalink / raw)
To: santosh.shilimkar, netdev; +Cc: David Miller
In-Reply-To: <c79821e0-307c-5736-6eb5-e20983097345@oracle.com>
Hi Santosh,
On 01/07/2019 13.41, santosh.shilimkar@oracle.com wrote:
>> @@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>> if (printk_ratelimit())
>> pr_warn("RDS/IB: %s returned error(%d)\n",
>> __func__, ret);
>> + goto out;
>> + }
>> +
>> + if (!frmr->fr_reg)
>> + goto out;
>> +
>> + /* Wait for the registration to complete in order to prevent an invalid
>> + * access error resulting from a race between the memory region already
>> + * being accessed while registration is still pending.
>> + */
>> + wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
>> + msecs_to_jiffies(100));
>> +
> This arbitrary timeout in this patch as well as pacth 1/7 which
> Dave pointed out has any logic ?
>
It's empirical (see my response to David's question):
Memory registrations took longer than invalidations, hence 100msec instead of 10msec.
> MR registration command issued to hardware can at times take as
> much as command timeout(e.g 60 seconds in CX3) and upto that its still
> legitimate operation and not necessary failure. We shouldn't add
> arbitrary time outs in ULPs.
Where did you find the 60 seconds for CX3 you are referring to?
Is there a "generic" upper-bound that is not tied to a specific vendor / HCA?
Can you provide a pointer?
Thanks,
Gerd
^ permalink raw reply
* Re: [PATCH net-next 1/7] net/rds: Give fr_state a chance to transition to FRMR_IS_FREE
From: santosh.shilimkar @ 2019-07-01 20:53 UTC (permalink / raw)
To: Gerd Rausch, David Miller; +Cc: netdev
In-Reply-To: <a4834749-4aa2-7e79-dbf8-004580364a39@oracle.com>
On 7/1/19 1:50 PM, Gerd Rausch wrote:
> Hi David,
>
> On 01/07/2019 11.27, David Miller wrote:
>> From: Gerd Rausch <gerd.rausch@oracle.com>
>> Date: Mon, 1 Jul 2019 09:39:44 -0700
>>
>>> + /* Memory regions make it onto the "clean_list" via
>>> + * "rds_ib_flush_mr_pool", after the memory region has
>>> + * been posted for invalidation via "rds_ib_post_inv".
>>> + *
>>> + * At that point in time, "fr_state" may still be
>>> + * in state "FRMR_IS_INUSE", since the only place where
>>> + * "fr_state" transitions to "FRMR_IS_FREE" is in
>>> + * is in "rds_ib_mr_cqe_handler", which is
>>> + * triggered by a tasklet.
>>> + *
>>> + * So in case we notice that
>>> + * "fr_state != FRMR_IS_FREE" (see below), * we wait for
>>> + * "fr_inv_done" to trigger with a maximum of 10msec.
>>> + * Then we check again, and only put the memory region
>>> + * onto the drop_list (via "rds_ib_free_frmr")
>>> + * in case the situation remains unchanged.
>>> + *
>>> + * This avoids the problem of memory-regions bouncing
>>> + * between "clean_list" and "drop_list" before they
>>> + * even have a chance to be properly invalidated.
>>> + */
>>> + frmr = &ibmr->u.frmr;
>>> + wait_event_timeout(frmr->fr_inv_done,
>>> + frmr->fr_state == FRMR_IS_FREE,
>>> + msecs_to_jiffies(10));
>>> + if (frmr->fr_state == FRMR_IS_FREE)
>>> + break;
>>
>> If we see FRMR_IS_FREE after the timeout, what cleans this up?
>>
>
> In that case, the memory-region is subjected to the
> "rds_ib_free_frmr(ibmr, true)" call that follows:
> In essence making it onto the "drop_list".
>
> It's the same as if it wouldn't transition to FRMR_IS_FREE at all.
> In both cases, the memory region should get dropped, and the application
> would have been penalized by an extra 10msec wait-time (for having tried to invalidate it).
>
>> Also, why 10msec?
>
> It's empirical.
> I had added some debugging code (not part of this submission) that traced
> the return value of "wait_event_timeout" in order to see the out-lier in terms
> of processing the "IB_WR_LOCAL_INV" request.
>
> On my test-systems the majority of requests were done in less than 1msec.
> I saw an outlier at almost 2msec once.
> So I gave it an extra order-of-magnitude multiplier for extra buffer / paranoia.
>
>> Why that specific value and not some other value?
>
> I looked around to find what Mellanox or any other reference material had
> so say about the expected turn--around time of an "IB_WR_LOCAL_INV" ought to be.
> I wasn't able to find any.
>
LOCAL_INV/REG etc are all end being HCA commands and the command
timeouts are large. 60 seconds is what CX3 HCA has for example.
Thats the worst case timeout from HCA before marking the command
to be timeout and hence the operation to be failed.
Regards,
Santosh
^ permalink raw reply
* Re: [PATCH net-next 1/7] net/rds: Give fr_state a chance to transition to FRMR_IS_FREE
From: Gerd Rausch @ 2019-07-01 20:50 UTC (permalink / raw)
To: David Miller; +Cc: santosh.shilimkar, netdev
In-Reply-To: <20190701.112751.509316780582361121.davem@davemloft.net>
Hi David,
On 01/07/2019 11.27, David Miller wrote:
> From: Gerd Rausch <gerd.rausch@oracle.com>
> Date: Mon, 1 Jul 2019 09:39:44 -0700
>
>> + /* Memory regions make it onto the "clean_list" via
>> + * "rds_ib_flush_mr_pool", after the memory region has
>> + * been posted for invalidation via "rds_ib_post_inv".
>> + *
>> + * At that point in time, "fr_state" may still be
>> + * in state "FRMR_IS_INUSE", since the only place where
>> + * "fr_state" transitions to "FRMR_IS_FREE" is in
>> + * is in "rds_ib_mr_cqe_handler", which is
>> + * triggered by a tasklet.
>> + *
>> + * So in case we notice that
>> + * "fr_state != FRMR_IS_FREE" (see below), * we wait for
>> + * "fr_inv_done" to trigger with a maximum of 10msec.
>> + * Then we check again, and only put the memory region
>> + * onto the drop_list (via "rds_ib_free_frmr")
>> + * in case the situation remains unchanged.
>> + *
>> + * This avoids the problem of memory-regions bouncing
>> + * between "clean_list" and "drop_list" before they
>> + * even have a chance to be properly invalidated.
>> + */
>> + frmr = &ibmr->u.frmr;
>> + wait_event_timeout(frmr->fr_inv_done,
>> + frmr->fr_state == FRMR_IS_FREE,
>> + msecs_to_jiffies(10));
>> + if (frmr->fr_state == FRMR_IS_FREE)
>> + break;
>
> If we see FRMR_IS_FREE after the timeout, what cleans this up?
>
In that case, the memory-region is subjected to the
"rds_ib_free_frmr(ibmr, true)" call that follows:
In essence making it onto the "drop_list".
It's the same as if it wouldn't transition to FRMR_IS_FREE at all.
In both cases, the memory region should get dropped, and the application
would have been penalized by an extra 10msec wait-time (for having tried to invalidate it).
> Also, why 10msec?
It's empirical.
I had added some debugging code (not part of this submission) that traced
the return value of "wait_event_timeout" in order to see the out-lier in terms
of processing the "IB_WR_LOCAL_INV" request.
On my test-systems the majority of requests were done in less than 1msec.
I saw an outlier at almost 2msec once.
So I gave it an extra order-of-magnitude multiplier for extra buffer / paranoia.
> Why that specific value and not some other value?
I looked around to find what Mellanox or any other reference material had
so say about the expected turn--around time of an "IB_WR_LOCAL_INV" ought to be.
I wasn't able to find any.
Please note that even if there was an upper-bound specified, such as minutes:
It wouldn't necessarily be a good idea to penalize an application by wait-times
up to one minute, if the alternative is to just put this memory region on a
drop-list and pick another one (which is suggested here).
> Why not wait for however long it takes for the tasklet to run and clean it up?
Two reasons I can think of:
1) The penalty of long wait-times would go to the application
2) If there were a firmware-bug, the "wait_event" would not terminate
Thanks,
Gerd
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Heiner Kallweit @ 2019-07-01 20:37 UTC (permalink / raw)
To: Andrew Lunn, Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
netdev, devicetree, linux-kernel, Douglas Anderson
In-Reply-To: <20190701200248.GJ30468@lunn.ch>
On 01.07.2019 22:02, Andrew Lunn wrote:
> On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
>> The RTL8211E has extension pages, which can be accessed after
>> selecting a page through a custom method. Add a function to
>> modify bits in a register of an extension page and a few
>> helpers for dealing with ext pages.
>>
>> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
>> inspired by their counterparts phy_modify_paged() and
>> phy_restore_page().
>
> Hi Matthias
>
> While an extended page is selected, what happens to the normal
> registers in the range 0-0x1c? Are they still accessible?
>
AFAIK: no
> Andrew
>
Heiner
^ permalink raw reply
* Re: [PATCH 3/3] net: phy: realtek: Support SSC for the RTL8211E
From: Heiner Kallweit @ 2019-07-01 20:49 UTC (permalink / raw)
To: Matthias Kaehlcke, David S . Miller, Rob Herring, Mark Rutland,
Andrew Lunn, Florian Fainelli
Cc: netdev, devicetree, linux-kernel, Douglas Anderson
In-Reply-To: <20190701195225.120808-3-mka@chromium.org>
On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> By default Spread-Spectrum Clocking (SSC) is disabled on the RTL8211E.
> Enable it if the device tree property 'realtek,enable-ssc' exists.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/net/phy/realtek.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index dfc2e20ef335..b617169ccc8c 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -9,8 +9,10 @@
> * Copyright (c) 2004 Freescale Semiconductor, Inc.
> */
> #include <linux/bitops.h>
> -#include <linux/phy.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> #include <linux/module.h>
> +#include <linux/phy.h>
>
> #define RTL821x_PHYSR 0x11
> #define RTL821x_PHYSR_DUPLEX BIT(13)
> @@ -28,6 +30,8 @@
>
> #define RTL8211E_EXT_PAGE 7
> #define RTL8211E_EPAGSR 0x1e
> +#define RTL8211E_SCR 0x1a
> +#define RTL8211E_SCR_DISABLE_RXC_SSC BIT(2)
>
> #define RTL8211F_INSR 0x1d
>
> @@ -87,8 +91,8 @@ static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
> return ret;
> }
>
> -static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> - int page, u32 regnum, u16 mask, u16 set)
> +static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
> + u32 regnum, u16 mask, u16 set)
> {
> int ret = 0;
> int oldpage;
> @@ -114,6 +118,22 @@ static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> return rtl821e_restore_page(phydev, oldpage, ret);
> }
>
> +static int rtl8211e_probe(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + int err;
> +
> + if (of_property_read_bool(dev->of_node, "realtek,enable-ssc")) {
> + err = rtl8211e_modify_ext_paged(phydev, 0xa0, RTL8211E_SCR,
> + RTL8211E_SCR_DISABLE_RXC_SSC,
> + 0);
> + if (err)
> + dev_err(dev, "failed to enable SSC on RXC: %d\n", err);
> + }
> +
> + return 0;
> +}
> +
> static int rtl8201_ack_interrupt(struct phy_device *phydev)
> {
> int err;
> @@ -372,6 +392,7 @@ static struct phy_driver realtek_drvs[] = {
> .config_init = &rtl8211e_config_init,
> .ack_interrupt = &rtl821x_ack_interrupt,
> .config_intr = &rtl8211e_config_intr,
> + .probe = rtl8211e_probe,
I'm not sure whether this setting survives soft reset and power-down.
Maybe it should be better applied in the config_init callback.
> .suspend = genphy_suspend,
> .resume = genphy_resume,
> .read_page = rtl821x_read_page,
>
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Heiner Kallweit @ 2019-07-01 20:43 UTC (permalink / raw)
To: Matthias Kaehlcke, David S . Miller, Rob Herring, Mark Rutland,
Andrew Lunn, Florian Fainelli
Cc: netdev, devicetree, linux-kernel, Douglas Anderson
In-Reply-To: <20190701195225.120808-2-mka@chromium.org>
On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> The RTL8211E has extension pages, which can be accessed after
> selecting a page through a custom method. Add a function to
> modify bits in a register of an extension page and a few
> helpers for dealing with ext pages.
>
> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> inspired by their counterparts phy_modify_paged() and
> phy_restore_page().
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> This code might be applicable to other Realtek PHYs, but I don't
> have access to the datasheets to confirm it, so for now it's just
> for the RTL8211E.
>
This extended page mechanism exists on a number of older Realtek
PHY's. For most extended pages however Realtek releases no public
documentation.
Considering that we use these helpers in one place only, I don't
really see a need for them.
> drivers/net/phy/realtek.c | 61 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index a669945eb829..dfc2e20ef335 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -26,6 +26,9 @@
> #define RTL821x_EXT_PAGE_SELECT 0x1e
> #define RTL821x_PAGE_SELECT 0x1f
>
> +#define RTL8211E_EXT_PAGE 7
> +#define RTL8211E_EPAGSR 0x1e
> +
> #define RTL8211F_INSR 0x1d
>
> #define RTL8211F_TX_DELAY BIT(8)
> @@ -53,6 +56,64 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
> return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
> }
>
> +static int rtl821e_select_ext_page(struct phy_device *phydev, int page)
> +{
> + int rc;
> +
> + rc = phy_write(phydev, RTL821x_PAGE_SELECT, RTL8211E_EXT_PAGE);
> + if (rc)
> + return rc;
> +
> + return phy_write(phydev, RTL8211E_EPAGSR, page);
> +}
> +
> +static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
> +{
> + int r;
> +
> + if (oldpage >= 0) {
> + r = phy_write(phydev, RTL821x_PAGE_SELECT, oldpage);
> +
> + /* Propagate the operation return code if the page write
> + * was successful.
> + */
> + if (ret >= 0 && r < 0)
> + ret = r;
> + } else {
> + /* Propagate the page selection error code */
> + ret = oldpage;
> + }
> +
> + return ret;
> +}
> +
> +static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> + int page, u32 regnum, u16 mask, u16 set)
> +{
> + int ret = 0;
> + int oldpage;
> + int new;
> +
> + oldpage = phy_read(phydev, RTL821x_PAGE_SELECT);
> + if (oldpage < 0)
> + goto out;
> +
> + ret = rtl821e_select_ext_page(phydev, page);
> + if (ret)
> + goto out;
> +
> + ret = phy_read(phydev, regnum);
> + if (ret < 0)
> + goto out;
> +
> + new = (ret & ~mask) | set;
> + if (new != ret)
> + ret = phy_write(phydev, regnum, new);
> +
> +out:
> + return rtl821e_restore_page(phydev, oldpage, ret);
> +}
> +
> static int rtl8201_ack_interrupt(struct phy_device *phydev)
> {
> int err;
>
^ permalink raw reply
* [PATCH bpf-next 8/8] samples/bpf: fix tcp_bpf.readme detach command
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Copy-paste, should be detach, not attach.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
samples/bpf/tcp_bpf.readme | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/samples/bpf/tcp_bpf.readme b/samples/bpf/tcp_bpf.readme
index fee746621aec..78e247f62108 100644
--- a/samples/bpf/tcp_bpf.readme
+++ b/samples/bpf/tcp_bpf.readme
@@ -25,4 +25,4 @@ attached to the cgroupv2).
To remove (unattach) a socket_ops BPF program from a cgroupv2:
- bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
+ bpftool cgroup detach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 7/8] samples/bpf: add sample program that periodically dumps TCP stats
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Uses new RTT callback to dump stats every second.
$ mkdir -p /tmp/cgroupv2
$ mount -t cgroup2 none /tmp/cgroupv2
$ mkdir -p /tmp/cgroupv2/foo
$ echo $$ >> /tmp/cgroupv2/foo/cgroup.procs
$ bpftool prog load ./tcp_dumpstats_kern.o /sys/fs/bpf/tcp_prog
$ bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
$ bpftool prog tracelog
$ # run neper/netperf/etc
Used neper to compare performance with and without this program attached
and didn't see any noticeable performance impact.
Sample output:
<idle>-0 [015] ..s. 2074.128800: 0: dsack_dups=0 delivered=242526
<idle>-0 [015] ..s. 2074.128808: 0: delivered_ce=0 icsk_retransmits=0
<idle>-0 [015] ..s. 2075.130133: 0: dsack_dups=0 delivered=323599
<idle>-0 [015] ..s. 2075.130138: 0: delivered_ce=0 icsk_retransmits=0
<idle>-0 [005] .Ns. 2076.131440: 0: dsack_dups=0 delivered=404648
<idle>-0 [005] .Ns. 2076.131447: 0: delivered_ce=0 icsk_retransmits=0
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
samples/bpf/Makefile | 1 +
samples/bpf/tcp_dumpstats_kern.c | 65 ++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
create mode 100644 samples/bpf/tcp_dumpstats_kern.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 0917f8cf4fab..eaebbeead42f 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -154,6 +154,7 @@ always += tcp_iw_kern.o
always += tcp_clamp_kern.o
always += tcp_basertt_kern.o
always += tcp_tos_reflect_kern.o
+always += tcp_dumpstats_kern.o
always += xdp_redirect_kern.o
always += xdp_redirect_map_kern.o
always += xdp_redirect_cpu_kern.o
diff --git a/samples/bpf/tcp_dumpstats_kern.c b/samples/bpf/tcp_dumpstats_kern.c
new file mode 100644
index 000000000000..5d22bf61db65
--- /dev/null
+++ b/samples/bpf/tcp_dumpstats_kern.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+#define INTERVAL 1000000000ULL
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
+
+struct {
+ __u32 type;
+ __u32 map_flags;
+ int *key;
+ __u64 *value;
+} bpf_next_dump SEC(".maps") = {
+ .type = BPF_MAP_TYPE_SK_STORAGE,
+ .map_flags = BPF_F_NO_PREALLOC,
+};
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+ struct bpf_tcp_sock *tcp_sk;
+ struct bpf_sock *sk;
+ __u64 *next_dump;
+ __u64 now;
+
+ switch (ctx->op) {
+ case BPF_SOCK_OPS_TCP_CONNECT_CB:
+ bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+ return 1;
+ case BPF_SOCK_OPS_RTT_CB:
+ break;
+ default:
+ return 1;
+ }
+
+ sk = ctx->sk;
+ if (!sk)
+ return 1;
+
+ next_dump = bpf_sk_storage_get(&bpf_next_dump, sk, 0,
+ BPF_SK_STORAGE_GET_F_CREATE);
+ if (!next_dump)
+ return 1;
+
+ now = bpf_ktime_get_ns();
+ if (now < *next_dump)
+ return 1;
+
+ tcp_sk = bpf_tcp_sock(sk);
+ if (!tcp_sk)
+ return 1;
+
+ *next_dump = now + INTERVAL;
+
+ bpf_printk("dsack_dups=%u delivered=%u\n",
+ tcp_sk->dsack_dups, tcp_sk->delivered);
+ bpf_printk("delivered_ce=%u icsk_retransmits=%u\n",
+ tcp_sk->delivered_ce, tcp_sk->icsk_retransmits);
+
+ return 1;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 6/8] selftests/bpf: test BPF_SOCK_OPS_RTT_CB
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Make sure the callback is invoked for syn-ack and data packet.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/Makefile | 3 +-
tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
tools/testing/selftests/bpf/test_tcp_rtt.c | 253 ++++++++++++++++++++
3 files changed, 316 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index de1754a8f5fe..2620406a53ec 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -27,7 +27,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
test_cgroup_storage test_select_reuseport test_section_names \
test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
test_btf_dump test_cgroup_attach xdping test_sockopt test_sockopt_sk \
- test_sockopt_multi
+ test_sockopt_multi test_tcp_rtt
BPF_OBJ_FILES = $(patsubst %.c,%.o, $(notdir $(wildcard progs/*.c)))
TEST_GEN_FILES = $(BPF_OBJ_FILES)
@@ -107,6 +107,7 @@ $(OUTPUT)/test_cgroup_attach: cgroup_helpers.c
$(OUTPUT)/test_sockopt: cgroup_helpers.c
$(OUTPUT)/test_sockopt_sk: cgroup_helpers.c
$(OUTPUT)/test_sockopt_multi: cgroup_helpers.c
+$(OUTPUT)/test_tcp_rtt: cgroup_helpers.c
.PHONY: force
diff --git a/tools/testing/selftests/bpf/progs/tcp_rtt.c b/tools/testing/selftests/bpf/progs/tcp_rtt.c
new file mode 100644
index 000000000000..233bdcb1659e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tcp_rtt.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+__u32 _version SEC("version") = 1;
+
+struct tcp_rtt_storage {
+ __u32 invoked;
+ __u32 dsack_dups;
+ __u32 delivered;
+ __u32 delivered_ce;
+ __u32 icsk_retransmits;
+};
+
+struct bpf_map_def SEC("maps") socket_storage_map = {
+ .type = BPF_MAP_TYPE_SK_STORAGE,
+ .key_size = sizeof(int),
+ .value_size = sizeof(struct tcp_rtt_storage),
+ .map_flags = BPF_F_NO_PREALLOC,
+};
+BPF_ANNOTATE_KV_PAIR(socket_storage_map, int, struct tcp_rtt_storage);
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+ struct tcp_rtt_storage *storage;
+ struct bpf_tcp_sock *tcp_sk;
+ int op = (int) ctx->op;
+ struct bpf_sock *sk;
+
+ sk = ctx->sk;
+ if (!sk)
+ return 1;
+
+ storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
+ BPF_SK_STORAGE_GET_F_CREATE);
+ if (!storage)
+ return 1;
+
+ if (op == BPF_SOCK_OPS_TCP_CONNECT_CB) {
+ bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+ return 1;
+ }
+
+ if (op != BPF_SOCK_OPS_RTT_CB)
+ return 1;
+
+ tcp_sk = bpf_tcp_sock(sk);
+ if (!tcp_sk)
+ return 1;
+
+ storage->invoked++;
+
+ storage->dsack_dups = tcp_sk->dsack_dups;
+ storage->delivered = tcp_sk->delivered;
+ storage->delivered_ce = tcp_sk->delivered_ce;
+ storage->icsk_retransmits = tcp_sk->icsk_retransmits;
+
+ return 1;
+}
diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
new file mode 100644
index 000000000000..413fd8514adc
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <error.h>
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <pthread.h>
+
+#include <linux/filter.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "bpf_util.h"
+#include "cgroup_helpers.h"
+
+#define CG_PATH "/tcp_rtt"
+
+struct tcp_rtt_storage {
+ __u32 invoked;
+ __u32 dsack_dups;
+ __u32 delivered;
+ __u32 delivered_ce;
+ __u32 icsk_retransmits;
+};
+
+static void send_byte(int fd)
+{
+ char b = 0x55;
+
+ if (write(fd, &b, sizeof(b)) != 1)
+ error(1, errno, "Failed to send single byte");
+}
+
+static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
+ __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
+ __u32 icsk_retransmits)
+{
+ int err = 0;
+ struct tcp_rtt_storage val;
+
+ if (bpf_map_lookup_elem(map_fd, &client_fd, &val) < 0)
+ error(1, errno, "Failed to read socket storage");
+
+ if (val.invoked != invoked) {
+ log_err("%s: unexpected bpf_tcp_sock.invoked %d != %d",
+ msg, val.invoked, invoked);
+ err++;
+ }
+
+ if (val.dsack_dups != dsack_dups) {
+ log_err("%s: unexpected bpf_tcp_sock.dsack_dups %d != %d",
+ msg, val.dsack_dups, dsack_dups);
+ err++;
+ }
+
+ if (val.delivered != delivered) {
+ log_err("%s: unexpected bpf_tcp_sock.delivered %d != %d",
+ msg, val.delivered, delivered);
+ err++;
+ }
+
+ if (val.delivered_ce != delivered_ce) {
+ log_err("%s: unexpected bpf_tcp_sock.delivered_ce %d != %d",
+ msg, val.delivered_ce, delivered_ce);
+ err++;
+ }
+
+ if (val.icsk_retransmits != icsk_retransmits) {
+ log_err("%s: unexpected bpf_tcp_sock.icsk_retransmits %d != %d",
+ msg, val.icsk_retransmits, icsk_retransmits);
+ err++;
+ }
+
+ return err;
+}
+
+static int connect_to_server(int server_fd)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ log_err("Failed to create client socket");
+ return -1;
+ }
+
+ if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+ log_err("Failed to get server addr");
+ goto out;
+ }
+
+ if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
+ log_err("Fail to connect to server");
+ goto out;
+ }
+
+ return fd;
+
+out:
+ close(fd);
+ return -1;
+}
+
+static int run_test(int cgroup_fd, int server_fd)
+{
+ struct bpf_prog_load_attr attr = {
+ .prog_type = BPF_PROG_TYPE_SOCK_OPS,
+ .file = "./tcp_rtt.o",
+ .expected_attach_type = BPF_CGROUP_SOCK_OPS,
+ };
+ struct bpf_program *prog;
+ struct bpf_object *obj;
+ struct bpf_map *map;
+ int client_fd;
+ int ignored;
+ int map_fd;
+ int err;
+
+ err = bpf_prog_load_xattr(&attr, &obj, &ignored);
+ if (err) {
+ log_err("Failed to load BPF object");
+ return -1;
+ }
+
+ map = bpf_map__next(NULL, obj);
+ map_fd = bpf_map__fd(map);
+
+ prog = bpf_program__next(NULL, obj);
+ err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
+ BPF_CGROUP_SOCK_OPS, 0);
+ if (err) {
+ log_err("Failed to attach BPF program");
+ goto close_bpf_object;
+ }
+
+ client_fd = connect_to_server(server_fd);
+ if (client_fd < 0) {
+ err = -1;
+ goto close_bpf_object;
+ }
+
+ err += verify_sk(map_fd, client_fd, "syn-ack",
+ /*invoked=*/1,
+ /*dsack_dups=*/0,
+ /*delivered=*/1,
+ /*delivered_ce=*/0,
+ /*icsk_retransmits=*/0);
+
+ send_byte(client_fd);
+
+ err += verify_sk(map_fd, client_fd, "first payload byte",
+ /*invoked=*/2,
+ /*dsack_dups=*/0,
+ /*delivered=*/2,
+ /*delivered_ce=*/0,
+ /*icsk_retransmits=*/0);
+
+ close(client_fd);
+
+close_bpf_object:
+ bpf_object__close(obj);
+ return err;
+}
+
+static int start_server(void)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
+ };
+ int fd;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ log_err("Failed to create server socket");
+ return -1;
+ }
+
+ if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
+ log_err("Failed to bind socket");
+ close(fd);
+ return -1;
+ }
+
+ return fd;
+}
+
+static void *server_thread(void *arg)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd = *(int *)arg;
+ int client_fd;
+
+ if (listen(fd, 1) < 0)
+ error(1, errno, "Failed to listed on socket");
+
+ client_fd = accept(fd, (struct sockaddr *)&addr, &len);
+ if (client_fd < 0)
+ error(1, errno, "Failed to accept client");
+
+ if (accept(fd, (struct sockaddr *)&addr, &len) >= 0)
+ error(1, errno, "Unexpected success in second accept");
+
+ close(client_fd);
+
+ return NULL;
+}
+
+int main(int args, char **argv)
+{
+ int server_fd, cgroup_fd;
+ int err = EXIT_SUCCESS;
+ pthread_t tid;
+
+ if (setup_cgroup_environment())
+ goto cleanup_obj;
+
+ cgroup_fd = create_and_get_cgroup(CG_PATH);
+ if (cgroup_fd < 0)
+ goto cleanup_cgroup_env;
+
+ if (join_cgroup(CG_PATH))
+ goto cleanup_cgroup;
+
+ server_fd = start_server();
+ if (server_fd < 0) {
+ err = EXIT_FAILURE;
+ goto cleanup_cgroup;
+ }
+
+ pthread_create(&tid, NULL, server_thread, (void *)&server_fd);
+
+ if (run_test(cgroup_fd, server_fd))
+ err = EXIT_FAILURE;
+
+ close(server_fd);
+
+ printf("test_sockopt_sk: %s\n",
+ err == EXIT_SUCCESS ? "PASSED" : "FAILED");
+
+cleanup_cgroup:
+ close(cgroup_fd);
+cleanup_cgroup_env:
+ cleanup_cgroup_environment();
+cleanup_obj:
+ return err;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 5/8] bpf/tools: sync bpf.h
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Sync new bpf_tcp_sock fields and new BPF_PROG_TYPE_SOCK_OPS RTT callback.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/include/uapi/linux/bpf.h | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a396b516a2b2..cecf42c871d4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1767,6 +1767,7 @@ union bpf_attr {
* * **BPF_SOCK_OPS_RTO_CB_FLAG** (retransmission time out)
* * **BPF_SOCK_OPS_RETRANS_CB_FLAG** (retransmission)
* * **BPF_SOCK_OPS_STATE_CB_FLAG** (TCP state change)
+ * * **BPF_SOCK_OPS_RTT_CB_FLAG** (every RTT)
*
* Therefore, this function can be used to clear a callback flag by
* setting the appropriate bit to zero. e.g. to disable the RTO
@@ -3069,6 +3070,12 @@ struct bpf_tcp_sock {
* sum(delta(snd_una)), or how many bytes
* were acked.
*/
+ __u32 dsack_dups; /* RFC4898 tcpEStatsStackDSACKDups
+ * total number of DSACK blocks received
+ */
+ __u32 delivered; /* Total data packets delivered incl. rexmits */
+ __u32 delivered_ce; /* Like the above but only ECE marked packets */
+ __u32 icsk_retransmits; /* Number of unrecovered [RTO] timeouts */
};
struct bpf_sock_tuple {
@@ -3311,7 +3318,8 @@ struct bpf_sock_ops {
#define BPF_SOCK_OPS_RTO_CB_FLAG (1<<0)
#define BPF_SOCK_OPS_RETRANS_CB_FLAG (1<<1)
#define BPF_SOCK_OPS_STATE_CB_FLAG (1<<2)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS 0x7 /* Mask of all currently
+#define BPF_SOCK_OPS_RTT_CB_FLAG (1<<3)
+#define BPF_SOCK_OPS_ALL_CB_FLAGS 0xF /* Mask of all currently
* supported cb flags
*/
@@ -3366,6 +3374,8 @@ enum {
BPF_SOCK_OPS_TCP_LISTEN_CB, /* Called on listen(2), right after
* socket transition to LISTEN state.
*/
+ BPF_SOCK_OPS_RTT_CB, /* Called on every RTT.
+ */
};
/* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 4/8] bpf: add icsk_retransmits to bpf_tcp_sock
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Add some inet_connection_sock fields to bpf_tcp_sock that might be useful
for debugging congestion control issues.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/uapi/linux/bpf.h | 1 +
net/core/filter.c | 20 +++++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bfb0b1a76684..ead27aebf491 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3078,6 +3078,7 @@ struct bpf_tcp_sock {
*/
__u32 delivered; /* Total data packets delivered incl. rexmits */
__u32 delivered_ce; /* Like the above but only ECE marked packets */
+ __u32 icsk_retransmits; /* Number of unrecovered [RTO] timeouts */
};
struct bpf_sock_tuple {
diff --git a/net/core/filter.c b/net/core/filter.c
index 3da4b6c38b46..089aaea0ccc6 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5544,7 +5544,8 @@ static const struct bpf_func_proto bpf_sock_addr_sk_lookup_udp_proto = {
bool bpf_tcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
struct bpf_insn_access_aux *info)
{
- if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, delivered_ce))
+ if (off < 0 || off >= offsetofend(struct bpf_tcp_sock,
+ icsk_retransmits))
return false;
if (off % size != 0)
@@ -5575,6 +5576,20 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, FIELD)); \
} while (0)
+#define BPF_INET_SOCK_GET_COMMON(FIELD) \
+ do { \
+ BUILD_BUG_ON(FIELD_SIZEOF(struct inet_connection_sock, \
+ FIELD) > \
+ FIELD_SIZEOF(struct bpf_tcp_sock, FIELD)); \
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \
+ struct inet_connection_sock, \
+ FIELD), \
+ si->dst_reg, si->src_reg, \
+ offsetof( \
+ struct inet_connection_sock, \
+ FIELD)); \
+ } while (0)
+
if (insn > insn_buf)
return insn - insn_buf;
@@ -5661,6 +5676,9 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct bpf_tcp_sock, delivered_ce):
BPF_TCP_SOCK_GET_COMMON(delivered_ce);
break;
+ case offsetof(struct bpf_tcp_sock, icsk_retransmits):
+ BPF_INET_SOCK_GET_COMMON(icsk_retransmits);
+ break;
}
return insn - insn_buf;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 3/8] bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Add more fields to bpf_tcp_sock that might be useful for debugging
congestion control issues.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/uapi/linux/bpf.h | 5 +++++
net/core/filter.c | 11 ++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9cdd0aaeba06..bfb0b1a76684 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3073,6 +3073,11 @@ struct bpf_tcp_sock {
* sum(delta(snd_una)), or how many bytes
* were acked.
*/
+ __u32 dsack_dups; /* RFC4898 tcpEStatsStackDSACKDups
+ * total number of DSACK blocks received
+ */
+ __u32 delivered; /* Total data packets delivered incl. rexmits */
+ __u32 delivered_ce; /* Like the above but only ECE marked packets */
};
struct bpf_sock_tuple {
diff --git a/net/core/filter.c b/net/core/filter.c
index ad908526545d..3da4b6c38b46 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5544,7 +5544,7 @@ static const struct bpf_func_proto bpf_sock_addr_sk_lookup_udp_proto = {
bool bpf_tcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
struct bpf_insn_access_aux *info)
{
- if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, bytes_acked))
+ if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, delivered_ce))
return false;
if (off % size != 0)
@@ -5652,6 +5652,15 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct bpf_tcp_sock, bytes_acked):
BPF_TCP_SOCK_GET_COMMON(bytes_acked);
break;
+ case offsetof(struct bpf_tcp_sock, dsack_dups):
+ BPF_TCP_SOCK_GET_COMMON(dsack_dups);
+ break;
+ case offsetof(struct bpf_tcp_sock, delivered):
+ BPF_TCP_SOCK_GET_COMMON(delivered);
+ break;
+ case offsetof(struct bpf_tcp_sock, delivered_ce):
+ BPF_TCP_SOCK_GET_COMMON(delivered_ce);
+ break;
}
return insn - insn_buf;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 2/8] bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
We've added bpf_tcp_sock member to bpf_sock_ops and don't expect
any new tcp_sock fields in bpf_sock_ops. Let's remove
CONVERT_COMMON_TCP_SOCK_FIELDS so bpf_tcp_sock can be independently
extended.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
net/core/filter.c | 180 ++++++++++++++++++++++++++++++++--------------
1 file changed, 126 insertions(+), 54 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 4836264f82ee..ad908526545d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5194,54 +5194,6 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
};
#endif /* CONFIG_IPV6_SEG6_BPF */
-#define CONVERT_COMMON_TCP_SOCK_FIELDS(md_type, CONVERT) \
-do { \
- switch (si->off) { \
- case offsetof(md_type, snd_cwnd): \
- CONVERT(snd_cwnd); break; \
- case offsetof(md_type, srtt_us): \
- CONVERT(srtt_us); break; \
- case offsetof(md_type, snd_ssthresh): \
- CONVERT(snd_ssthresh); break; \
- case offsetof(md_type, rcv_nxt): \
- CONVERT(rcv_nxt); break; \
- case offsetof(md_type, snd_nxt): \
- CONVERT(snd_nxt); break; \
- case offsetof(md_type, snd_una): \
- CONVERT(snd_una); break; \
- case offsetof(md_type, mss_cache): \
- CONVERT(mss_cache); break; \
- case offsetof(md_type, ecn_flags): \
- CONVERT(ecn_flags); break; \
- case offsetof(md_type, rate_delivered): \
- CONVERT(rate_delivered); break; \
- case offsetof(md_type, rate_interval_us): \
- CONVERT(rate_interval_us); break; \
- case offsetof(md_type, packets_out): \
- CONVERT(packets_out); break; \
- case offsetof(md_type, retrans_out): \
- CONVERT(retrans_out); break; \
- case offsetof(md_type, total_retrans): \
- CONVERT(total_retrans); break; \
- case offsetof(md_type, segs_in): \
- CONVERT(segs_in); break; \
- case offsetof(md_type, data_segs_in): \
- CONVERT(data_segs_in); break; \
- case offsetof(md_type, segs_out): \
- CONVERT(segs_out); break; \
- case offsetof(md_type, data_segs_out): \
- CONVERT(data_segs_out); break; \
- case offsetof(md_type, lost_out): \
- CONVERT(lost_out); break; \
- case offsetof(md_type, sacked_out): \
- CONVERT(sacked_out); break; \
- case offsetof(md_type, bytes_received): \
- CONVERT(bytes_received); break; \
- case offsetof(md_type, bytes_acked): \
- CONVERT(bytes_acked); break; \
- } \
-} while (0)
-
#ifdef CONFIG_INET
static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
int dif, int sdif, u8 family, u8 proto)
@@ -5623,9 +5575,6 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, FIELD)); \
} while (0)
- CONVERT_COMMON_TCP_SOCK_FIELDS(struct bpf_tcp_sock,
- BPF_TCP_SOCK_GET_COMMON);
-
if (insn > insn_buf)
return insn - insn_buf;
@@ -5640,6 +5589,69 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, rtt_min) +
offsetof(struct minmax_sample, v));
break;
+ case offsetof(struct bpf_tcp_sock, snd_cwnd):
+ BPF_TCP_SOCK_GET_COMMON(snd_cwnd);
+ break;
+ case offsetof(struct bpf_tcp_sock, srtt_us):
+ BPF_TCP_SOCK_GET_COMMON(srtt_us);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_ssthresh):
+ BPF_TCP_SOCK_GET_COMMON(snd_ssthresh);
+ break;
+ case offsetof(struct bpf_tcp_sock, rcv_nxt):
+ BPF_TCP_SOCK_GET_COMMON(rcv_nxt);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_nxt):
+ BPF_TCP_SOCK_GET_COMMON(snd_nxt);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_una):
+ BPF_TCP_SOCK_GET_COMMON(snd_una);
+ break;
+ case offsetof(struct bpf_tcp_sock, mss_cache):
+ BPF_TCP_SOCK_GET_COMMON(mss_cache);
+ break;
+ case offsetof(struct bpf_tcp_sock, ecn_flags):
+ BPF_TCP_SOCK_GET_COMMON(ecn_flags);
+ break;
+ case offsetof(struct bpf_tcp_sock, rate_delivered):
+ BPF_TCP_SOCK_GET_COMMON(rate_delivered);
+ break;
+ case offsetof(struct bpf_tcp_sock, rate_interval_us):
+ BPF_TCP_SOCK_GET_COMMON(rate_interval_us);
+ break;
+ case offsetof(struct bpf_tcp_sock, packets_out):
+ BPF_TCP_SOCK_GET_COMMON(packets_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, retrans_out):
+ BPF_TCP_SOCK_GET_COMMON(retrans_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, total_retrans):
+ BPF_TCP_SOCK_GET_COMMON(total_retrans);
+ break;
+ case offsetof(struct bpf_tcp_sock, segs_in):
+ BPF_TCP_SOCK_GET_COMMON(segs_in);
+ break;
+ case offsetof(struct bpf_tcp_sock, data_segs_in):
+ BPF_TCP_SOCK_GET_COMMON(data_segs_in);
+ break;
+ case offsetof(struct bpf_tcp_sock, segs_out):
+ BPF_TCP_SOCK_GET_COMMON(segs_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, data_segs_out):
+ BPF_TCP_SOCK_GET_COMMON(data_segs_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, lost_out):
+ BPF_TCP_SOCK_GET_COMMON(lost_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, sacked_out):
+ BPF_TCP_SOCK_GET_COMMON(sacked_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, bytes_received):
+ BPF_TCP_SOCK_GET_COMMON(bytes_received);
+ break;
+ case offsetof(struct bpf_tcp_sock, bytes_acked):
+ BPF_TCP_SOCK_GET_COMMON(bytes_acked);
+ break;
}
return insn - insn_buf;
@@ -7913,9 +7925,6 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
SOCK_OPS_GET_FIELD(BPF_FIELD, OBJ_FIELD, OBJ); \
} while (0)
- CONVERT_COMMON_TCP_SOCK_FIELDS(struct bpf_sock_ops,
- SOCK_OPS_GET_TCP_SOCK_FIELD);
-
if (insn > insn_buf)
return insn - insn_buf;
@@ -8085,6 +8094,69 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
SOCK_OPS_GET_OR_SET_FIELD(sk_txhash, sk_txhash,
struct sock, type);
break;
+ case offsetof(struct bpf_sock_ops, snd_cwnd):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_cwnd);
+ break;
+ case offsetof(struct bpf_sock_ops, srtt_us):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(srtt_us);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_ssthresh):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_ssthresh);
+ break;
+ case offsetof(struct bpf_sock_ops, rcv_nxt):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rcv_nxt);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_nxt):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_nxt);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_una):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_una);
+ break;
+ case offsetof(struct bpf_sock_ops, mss_cache):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(mss_cache);
+ break;
+ case offsetof(struct bpf_sock_ops, ecn_flags):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(ecn_flags);
+ break;
+ case offsetof(struct bpf_sock_ops, rate_delivered):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rate_delivered);
+ break;
+ case offsetof(struct bpf_sock_ops, rate_interval_us):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rate_interval_us);
+ break;
+ case offsetof(struct bpf_sock_ops, packets_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(packets_out);
+ break;
+ case offsetof(struct bpf_sock_ops, retrans_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(retrans_out);
+ break;
+ case offsetof(struct bpf_sock_ops, total_retrans):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(total_retrans);
+ break;
+ case offsetof(struct bpf_sock_ops, segs_in):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(segs_in);
+ break;
+ case offsetof(struct bpf_sock_ops, data_segs_in):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(data_segs_in);
+ break;
+ case offsetof(struct bpf_sock_ops, segs_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(segs_out);
+ break;
+ case offsetof(struct bpf_sock_ops, data_segs_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(data_segs_out);
+ break;
+ case offsetof(struct bpf_sock_ops, lost_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(lost_out);
+ break;
+ case offsetof(struct bpf_sock_ops, sacked_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(sacked_out);
+ break;
+ case offsetof(struct bpf_sock_ops, bytes_received):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(bytes_received);
+ break;
+ case offsetof(struct bpf_sock_ops, bytes_acked):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(bytes_acked);
+ break;
case offsetof(struct bpf_sock_ops, sk):
*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
struct bpf_sock_ops_kern,
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 1/8] bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Performance impact should be minimal because it's under a new
BPF_SOCK_OPS_RTT_CB_FLAG flag that has to be explicitly enabled.
Suggested-by: Eric Dumazet <edumazet@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/net/tcp.h | 8 ++++++++
include/uapi/linux/bpf.h | 6 +++++-
net/ipv4/tcp_input.c | 4 ++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 9d36cc88d043..e16d8a3fd3b4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2221,6 +2221,14 @@ static inline bool tcp_bpf_ca_needs_ecn(struct sock *sk)
return (tcp_call_bpf(sk, BPF_SOCK_OPS_NEEDS_ECN, 0, NULL) == 1);
}
+static inline void tcp_bpf_rtt(struct sock *sk)
+{
+ struct tcp_sock *tp = tcp_sk(sk);
+
+ if (BPF_SOCK_OPS_TEST_FLAG(tp, BPF_SOCK_OPS_RTT_CB_FLAG))
+ tcp_call_bpf(sk, BPF_SOCK_OPS_RTT_CB, 0, NULL);
+}
+
#if IS_ENABLED(CONFIG_SMC)
extern struct static_key_false tcp_have_smc;
#endif
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index cffea1826a1f..9cdd0aaeba06 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1770,6 +1770,7 @@ union bpf_attr {
* * **BPF_SOCK_OPS_RTO_CB_FLAG** (retransmission time out)
* * **BPF_SOCK_OPS_RETRANS_CB_FLAG** (retransmission)
* * **BPF_SOCK_OPS_STATE_CB_FLAG** (TCP state change)
+ * * **BPF_SOCK_OPS_RTT_CB_FLAG** (every RTT)
*
* Therefore, this function can be used to clear a callback flag by
* setting the appropriate bit to zero. e.g. to disable the RTO
@@ -3314,7 +3315,8 @@ struct bpf_sock_ops {
#define BPF_SOCK_OPS_RTO_CB_FLAG (1<<0)
#define BPF_SOCK_OPS_RETRANS_CB_FLAG (1<<1)
#define BPF_SOCK_OPS_STATE_CB_FLAG (1<<2)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS 0x7 /* Mask of all currently
+#define BPF_SOCK_OPS_RTT_CB_FLAG (1<<3)
+#define BPF_SOCK_OPS_ALL_CB_FLAGS 0xF /* Mask of all currently
* supported cb flags
*/
@@ -3369,6 +3371,8 @@ enum {
BPF_SOCK_OPS_TCP_LISTEN_CB, /* Called on listen(2), right after
* socket transition to LISTEN state.
*/
+ BPF_SOCK_OPS_RTT_CB, /* Called on every RTT.
+ */
};
/* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index b71efeb0ae5b..c21e8a22fb3b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -778,6 +778,8 @@ static void tcp_rtt_estimator(struct sock *sk, long mrtt_us)
tp->rttvar_us -= (tp->rttvar_us - tp->mdev_max_us) >> 2;
tp->rtt_seq = tp->snd_nxt;
tp->mdev_max_us = tcp_rto_min_us(sk);
+
+ tcp_bpf_rtt(sk);
}
} else {
/* no previous measure. */
@@ -786,6 +788,8 @@ static void tcp_rtt_estimator(struct sock *sk, long mrtt_us)
tp->rttvar_us = max(tp->mdev_us, tcp_rto_min_us(sk));
tp->mdev_max_us = tp->rttvar_us;
tp->rtt_seq = tp->snd_nxt;
+
+ tcp_bpf_rtt(sk);
}
tp->srtt_us = max(1U, srtt);
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 0/8] bpf: TCP RTT sock_ops bpf callback
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
Congestion control team would like to have a periodic callback to
track some TCP statistics. Let's add a sock_ops callback that can be
selectively enabled on a socket by socket basis and is executed for
every RTT. BPF program frequency can be further controlled by calling
bpf_ktime_get_ns and bailing out early.
I run neper tcp_stream and tcp_rr tests with the sample program
from the last patch and didn't observe any noticeable performance
difference.
Suggested-by: Eric Dumazet <edumazet@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Stanislav Fomichev (8):
bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
bpf: add icsk_retransmits to bpf_tcp_sock
bpf/tools: sync bpf.h
selftests/bpf: test BPF_SOCK_OPS_RTT_CB
samples/bpf: add sample program that periodically dumps TCP stats
samples/bpf: fix tcp_bpf.readme detach command
include/net/tcp.h | 8 +
include/uapi/linux/bpf.h | 12 +-
net/core/filter.c | 207 +++++++++++-----
net/ipv4/tcp_input.c | 4 +
samples/bpf/Makefile | 1 +
samples/bpf/tcp_bpf.readme | 2 +-
samples/bpf/tcp_dumpstats_kern.c | 65 +++++
tools/include/uapi/linux/bpf.h | 12 +-
tools/testing/selftests/bpf/Makefile | 3 +-
tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
tools/testing/selftests/bpf/test_tcp_rtt.c | 253 ++++++++++++++++++++
11 files changed, 570 insertions(+), 58 deletions(-)
create mode 100644 samples/bpf/tcp_dumpstats_kern.c
create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: santosh.shilimkar @ 2019-07-01 20:41 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <505e9af7-a0cd-bf75-4a72-5d883ee06bf1@oracle.com>
On 7/1/19 9:39 AM, Gerd Rausch wrote:
> In order to:
> 1) avoid a silly bouncing between "clean_list" and "drop_list"
> triggered by function "rds_ib_reg_frmr" as it is releases frmr
> regions whose state is not "FRMR_IS_FREE" right away.
>
> 2) prevent an invalid access error in a race from a pending
> "IB_WR_LOCAL_INV" operation with a teardown ("dma_unmap_sg", "put_page")
> and de-registration ("ib_dereg_mr") of the corresponding
> memory region.
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
> net/rds/ib_frmr.c | 89 ++++++++++++++++++++++++++++++-----------------
> net/rds/ib_mr.h | 2 ++
> 2 files changed, 59 insertions(+), 32 deletions(-)
>
> diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
> index 9f8aa310c27a..3c953034dca3 100644
> --- a/net/rds/ib_frmr.c
> +++ b/net/rds/ib_frmr.c
> @@ -76,6 +76,7 @@ static struct rds_ib_mr *rds_ib_alloc_frmr(struct rds_ib_device *rds_ibdev,
>
> frmr->fr_state = FRMR_IS_FREE;
> init_waitqueue_head(&frmr->fr_inv_done);
> + init_waitqueue_head(&frmr->fr_reg_done);
> return ibmr;
>
> out_no_cigar:
> @@ -124,6 +125,7 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
> */
> ib_update_fast_reg_key(frmr->mr, ibmr->remap_count++);
> frmr->fr_state = FRMR_IS_INUSE;
> + frmr->fr_reg = true;
>
> memset(®_wr, 0, sizeof(reg_wr));
> reg_wr.wr.wr_id = (unsigned long)(void *)ibmr;
> @@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
> if (printk_ratelimit())
> pr_warn("RDS/IB: %s returned error(%d)\n",
> __func__, ret);
> + goto out;
> + }
> +
> + if (!frmr->fr_reg)
> + goto out;
> +
> + /* Wait for the registration to complete in order to prevent an invalid
> + * access error resulting from a race between the memory region already
> + * being accessed while registration is still pending.
> + */
> + wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
> + msecs_to_jiffies(100));
> +
This arbitrary timeout in this patch as well as pacth 1/7 which
Dave pointed out has any logic ?
MR registration command issued to hardware can at times take as
much as command timeout(e.g 60 seconds in CX3) and upto that its still
legitimate operation and not necessary failure. We shouldn't add
arbitrary time outs in ULPs.
Regards,
Santosh
^ permalink raw reply
* Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings
From: Heiner Kallweit @ 2019-07-01 20:36 UTC (permalink / raw)
To: Al Viro; +Cc: Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20190701195621.GC17978@ZenIV.linux.org.uk>
On 01.07.2019 21:56, Al Viro wrote:
> On Mon, Jul 01, 2019 at 09:35:28PM +0200, Heiner Kallweit wrote:
>> Sparse complains about casting to/from restricted __be16. Fix this.
>
> Fix what, exactly? Force-cast is not a fix - it's "STFU, I know
> better, it's really correct" to sparse. Which may or may not
> match the reality, but it definitely requires more in way of
> commit message than "sparse says it's wrong; shut it up".
>
>> static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
>> @@ -1537,7 +1537,7 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
>>
>> if (opts2 & RxVlanTag)
>> __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
>> - ntohs(opts2 & 0xffff));
>> + ntohs((__force __be16)(opts2 & 0xffff)));
>> }
>
> Should that be ntohs at all? What behaviour is correct on big-endian host?
>
> AFAICS, in that code opts2 comes from little-endian 32bit. It's converted to
> host-endian, lower 16 bits (i.e. the first two octets in memory) are then
> fed to ntohs. Suppose we had in-core value stored as A0, A1, A2, A3.
> On little-endian that code will yield A0 * 256 + A1, treated as host-endian.
> On big-endian the same will yield A1 * 256 + A0. Is that actually correct?
>
I think you're right and the original patch should be reverted.
> The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> third argument treats it as a host-endian integer. So... Has anyone
> tested that code on b-e host? Should that ntohs() actually be swab16(),
> yielding (on any host) the same value we currently get for l-e hosts only?
>
I haven't seen any b-e host with a Realtek network chip yet.
^ permalink raw reply
* Re: [PATCH net-next 2/7] net/rds: Get rid of "wait_clean_list_grace" and add locking
From: santosh.shilimkar @ 2019-07-01 20:33 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <5c49f180-0dbf-88b9-965d-6cb88061f31b@oracle.com>
On 7/1/19 9:39 AM, Gerd Rausch wrote:
> Waiting for activity on the "clean_list" to quiesce is no substitute
> for proper locking.
>
> We can have multiple threads competing for "llist_del_first"
> via "rds_ib_reuse_mr", and a single thread competing
> for "llist_del_all" and "llist_del_first" via "rds_ib_flush_mr_pool".
>
> Since "llist_del_first" depends on "list->first->next" not to change
> in the midst of the operation, simply waiting for all current calls
> to "rds_ib_reuse_mr" to quiesce across all CPUs is woefully inadequate:
>
> By the time "wait_clean_list_grace" is done iterating over all CPUs to see
> that there is no concurrent caller to "rds_ib_reuse_mr", a new caller may
> have just shown up on the first CPU.
>
> Furthermore, <linux/llist.h> explicitly calls out the need for locking:
> * Cases where locking is needed:
> * If we have multiple consumers with llist_del_first used in one consumer,
> * and llist_del_first or llist_del_all used in other consumers,
> * then a lock is needed.
>
> Also, while at it, drop the unused "pool" parameter
> from "list_to_llist_nodes".
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
Looks good.
^ permalink raw reply
* Re: [PATCH 1/4] net: dsa: Change DT bindings for Vitesse VSC73xx switches
From: Linus Walleij @ 2019-07-01 20:23 UTC (permalink / raw)
To: Florian Fainelli
Cc: Pawel Dembicki, Andrew Lunn, Vivien Didelot, David S. Miller,
Rob Herring, Mark Rutland, netdev,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org
In-Reply-To: <45ff597a-5090-3874-b43d-5b5f45d2d2f6@gmail.com>
On Mon, Jul 1, 2019 at 6:44 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> Take b53 for instance which supports MDIO and SPI by default, and
> optionally memory mapped and SRAB (indirect memory map) accesses, they
> all have the same compatible strings. Whether the switches will appear
> as spi_device, platform_device, or something else is entirely based on
> how the Device Tree is laid out.
That's clever.
Pawel can you restructure the series around this observation?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH net-next 4/7] net/rds: Fix NULL/ERR_PTR inconsistency
From: santosh.shilimkar @ 2019-07-01 20:14 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <9f46098a-bcc7-bc0e-20db-2cbf05fefdee@oracle.com>
On 7/1/19 9:39 AM, Gerd Rausch wrote:
> Make function "rds_ib_try_reuse_ibmr" return NULL in case
> memory region could not be allocated, since callers
> simply check if the return value is not NULL.
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
Looks good to me. Will add this to other fixes.
^ permalink raw reply
* Re: [PATCH net-next 7/7] net/rds: Initialize ic->i_fastreg_wrs upon allocation
From: santosh.shilimkar @ 2019-07-01 20:15 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <a97a92d4-c443-12e1-6d82-1646f9584828@oracle.com>
On 7/1/19 9:40 AM, Gerd Rausch wrote:
> Otherwise, if an IB connection is torn down before "rds_ib_setup_qp"
> is called, the value of "ic->i_fastreg_wrs" is still at zero
> (as it wasn't initialized by "rds_ib_setup_qp").
> Consequently "rds_ib_conn_path_shutdown" will spin forever,
> waiting for it to go back to "RDS_IB_DEFAULT_FR_WR",
> which of course will never happen as there are no
> outstanding work requests.
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
Looks good to me. Will add this to other fixes.
^ permalink raw reply
* Re: [PATCH net-next 6/7] net/rds: Keep track of and wait for FRWR segments in use upon shutdown
From: santosh.shilimkar @ 2019-07-01 20:15 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <ad27abb5-b86a-1942-e2c8-2cba00812849@oracle.com>
On 7/1/19 9:40 AM, Gerd Rausch wrote:
> Since "rds_ib_free_frmr" and "rds_ib_free_frmr_list" simply put
> the FRMR memory segments on the "drop_list" or "free_list",
> and it is the job of "rds_ib_flush_mr_pool" to reap those entries
> by ultimately issuing a "IB_WR_LOCAL_INV" work-request,
> we need to trigger and then wait for all those memory segments
> attached to a particular connection to be fully released before
> we can move on to release the QP, CQ, etc.
>
> So we make "rds_ib_conn_path_shutdown" wait for one more
> atomic_t called "i_fastreg_inuse_count" that keeps track of how
> many FRWR memory segments are out there marked "FRMR_IS_INUSE"
> (and also wake_up rds_ib_ring_empty_wait, as they go away).
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
Looks good to me. Will add this to other fixes.
^ 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