* [PATCH v2 32/37] xfrm: Replace hrtimer tasklet with softirq hrtimer
From: Anna-Maria Gleixner @ 2017-10-22 21:40 UTC (permalink / raw)
To: LKML
Cc: Thomas Gleixner, Peter Zijlstra, Ingo Molnar, keescook,
Christoph Hellwig, John Stultz, Steffen Klassert, Herbert Xu,
David S. Miller, netdev
In-Reply-To: <20171022213938.940451689@linutronix.de>
[-- Attachment #1: xfrm_Replace_hrtimer_tasklet_with_softirq_hrtimer.patch --]
[-- Type: text/plain, Size: 4776 bytes --]
From: Thomas Gleixner <tglx@linutronix.de>
Switch the timer to HRTIMER_MODE_SOFT, which executed the timer
callback in softirq context and remove the hrtimer_tasklet.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
---
include/net/xfrm.h | 2 +-
net/xfrm/xfrm_state.c | 30 ++++++++++++++++++------------
2 files changed, 19 insertions(+), 13 deletions(-)
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -216,7 +216,7 @@ struct xfrm_state {
struct xfrm_stats stats;
struct xfrm_lifetime_cur curlft;
- struct tasklet_hrtimer mtimer;
+ struct hrtimer mtimer;
struct xfrm_state_offload xso;
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -426,7 +426,7 @@ static void xfrm_put_mode(struct xfrm_mo
static void xfrm_state_gc_destroy(struct xfrm_state *x)
{
- tasklet_hrtimer_cancel(&x->mtimer);
+ hrtimer_cancel(&x->mtimer);
del_timer_sync(&x->rtimer);
kfree(x->aead);
kfree(x->aalg);
@@ -471,8 +471,8 @@ static void xfrm_state_gc_task(struct wo
static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
{
- struct tasklet_hrtimer *thr = container_of(me, struct tasklet_hrtimer, timer);
- struct xfrm_state *x = container_of(thr, struct xfrm_state, mtimer);
+ struct xfrm_state *x = container_of(me, struct xfrm_state, mtimer);
+ enum hrtimer_restart ret = HRTIMER_NORESTART;
unsigned long now = get_seconds();
long next = LONG_MAX;
int warn = 0;
@@ -536,7 +536,8 @@ static enum hrtimer_restart xfrm_timer_h
km_state_expired(x, 0, 0);
resched:
if (next != LONG_MAX) {
- tasklet_hrtimer_start(&x->mtimer, ktime_set(next, 0), HRTIMER_MODE_REL);
+ hrtimer_forward_now(&x->mtimer, ktime_set(next, 0));
+ ret = HRTIMER_RESTART;
}
goto out;
@@ -553,7 +554,7 @@ static enum hrtimer_restart xfrm_timer_h
out:
spin_unlock(&x->lock);
- return HRTIMER_NORESTART;
+ return ret;
}
static void xfrm_replay_timer_handler(unsigned long data);
@@ -572,8 +573,8 @@ struct xfrm_state *xfrm_state_alloc(stru
INIT_HLIST_NODE(&x->bydst);
INIT_HLIST_NODE(&x->bysrc);
INIT_HLIST_NODE(&x->byspi);
- tasklet_hrtimer_init(&x->mtimer, xfrm_timer_handler,
- CLOCK_BOOTTIME, HRTIMER_MODE_ABS);
+ hrtimer_init(&x->mtimer, CLOCK_BOOTTIME, HRTIMER_MODE_ABS_SOFT);
+ x->mtimer.function = xfrm_timer_handler;
setup_timer(&x->rtimer, xfrm_replay_timer_handler,
(unsigned long)x);
x->curlft.add_time = get_seconds();
@@ -1030,7 +1031,9 @@ xfrm_state_find(const xfrm_address_t *da
hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
}
x->lft.hard_add_expires_seconds = net->xfrm.sysctl_acq_expires;
- tasklet_hrtimer_start(&x->mtimer, ktime_set(net->xfrm.sysctl_acq_expires, 0), HRTIMER_MODE_REL);
+ hrtimer_start(&x->mtimer,
+ ktime_set(net->xfrm.sysctl_acq_expires, 0),
+ HRTIMER_MODE_REL_SOFT);
net->xfrm.state_num++;
xfrm_hash_grow_check(net, x->bydst.next != NULL);
spin_unlock_bh(&net->xfrm.xfrm_state_lock);
@@ -1141,7 +1144,7 @@ static void __xfrm_state_insert(struct x
hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
}
- tasklet_hrtimer_start(&x->mtimer, ktime_set(1, 0), HRTIMER_MODE_REL);
+ hrtimer_start(&x->mtimer, ktime_set(1, 0), HRTIMER_MODE_REL_SOFT);
if (x->replay_maxage)
mod_timer(&x->rtimer, jiffies + x->replay_maxage);
@@ -1245,7 +1248,9 @@ static struct xfrm_state *__find_acq_cor
x->mark.m = m->m;
x->lft.hard_add_expires_seconds = net->xfrm.sysctl_acq_expires;
xfrm_state_hold(x);
- tasklet_hrtimer_start(&x->mtimer, ktime_set(net->xfrm.sysctl_acq_expires, 0), HRTIMER_MODE_REL);
+ hrtimer_start(&x->mtimer,
+ ktime_set(net->xfrm.sysctl_acq_expires, 0),
+ HRTIMER_MODE_REL_SOFT);
list_add(&x->km.all, &net->xfrm.state_all);
hlist_add_head_rcu(&x->bydst, net->xfrm.state_bydst + h);
h = xfrm_src_hash(net, daddr, saddr, family);
@@ -1544,7 +1549,8 @@ int xfrm_state_update(struct xfrm_state
memcpy(&x1->lft, &x->lft, sizeof(x1->lft));
x1->km.dying = 0;
- tasklet_hrtimer_start(&x1->mtimer, ktime_set(1, 0), HRTIMER_MODE_REL);
+ hrtimer_start(&x1->mtimer, ktime_set(1, 0),
+ HRTIMER_MODE_REL_SOFT);
if (x1->curlft.use_time)
xfrm_state_check_expire(x1);
@@ -1568,7 +1574,7 @@ int xfrm_state_check_expire(struct xfrm_
if (x->curlft.bytes >= x->lft.hard_byte_limit ||
x->curlft.packets >= x->lft.hard_packet_limit) {
x->km.state = XFRM_STATE_EXPIRED;
- tasklet_hrtimer_start(&x->mtimer, 0, HRTIMER_MODE_REL);
+ hrtimer_start(&x->mtimer, 0, HRTIMER_MODE_REL_SOFT);
return -EINVAL;
}
^ permalink raw reply
* Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
From: Richard Leitner @ 2017-10-22 22:30 UTC (permalink / raw)
To: Florian Fainelli, Richard Leitner, fugang.duan
Cc: netdev, linux-kernel, Richard Leitner, stable
In-Reply-To: <6656d8d0-874a-574e-9ea9-37f8238bd155@gmail.com>
On 10/22/2017 08:31 PM, Florian Fainelli wrote:
>
> On 10/22/2017 06:11 AM, Richard Leitner wrote:
>> From: Richard Leitner <richard.leitner@skidata.com>
>>
>> From: Richard Leitner <richard.leitner@skidata.com>
>>
>> Some PHYs (for example the LAN8710) doesn't allow turning the ethernet
>> ref clocks off and on again without reset (according to their datasheet).
>> Exactly this behaviour was introduced for power saving reasons by
>> commit e8fcfcd5684a ("net: fec: optimize the clock management to save...")
>
> Please don't truncate the commit subject, put it in full length.
Ok. So I can then ignore the checkpatch.pl error complaining about too
long lines?
>
>> Therefore remove those en/disables of the refclk during probe, open and
>> close of the fec.
>>
>> Generally speaking is this issue only be relevant if the ref clk for
>> the PHY is generated by the SoC. In our specific case (PCB) this problem
>> does occur at about every 10th to 50th POR of an LAN8710 connected to an
>> i.MX6 SoC. The typical symptom of this problem is a "swinging" ethernet
>> link. Similar issues were experienced by users of the NXP forum:
>> https://community.nxp.com/thread/389902
>> https://community.nxp.com/message/309354
>> With this patch applied the issue didn't occur for at least a few
>> thousand PORs of our board.
>
> FWIW, this is a common problem on most Ethernet systems attached to a
> PHY because of the clock dependencies between the MAC and PHY, in either
> direction really. We recently stumbled across a similar problem on the
> MAC side (using bcmgenet) where after shutting down the PHY in
> ndo_stop() we would not get enough clock cycles for the MAC to cleanly
> shutdown and upon subsequent ndo_open() we could get the MAC in an
> invalid state.
Hmm... So I'm not the only one struggling with this... Was there already
discussion/ideas on how to solve this issue once and for all across the
kernel?
Andrew Lunn suggested to make the PHY driver a clock driver and let it
export the refclk... But IMHO that "feels" a bit strange. Due to the
fact in our case the clock is generated by the SoC and the PHY is an
external chip which only consumes it.
But back to this patch: Is it OK the way it fixes the issue?
>
>>
>> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
>
> Likewise here, not truncation of the commit subject necessary.
>
>> Cc: stable@vger.kernel.org
>
> netdev patches are handled directly by David Miller, see netdev-FAQ for
> details.
Ok. So I shouldn't post them to stable@vger.kernel.org at all?
>
>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>> ---
>> drivers/net/ethernet/freescale/fec_main.c | 7 -------
>> 1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index 3dc2d771a222..8f696b53d8b8 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -2844,9 +2844,6 @@ fec_enet_open(struct net_device *ndev)
>> return ret;
>>
>> pinctrl_pm_select_default_state(&fep->pdev->dev);
>> - ret = fec_enet_clk_enable(ndev, true);
>> - if (ret)
>> - goto clk_enable;
>>
>> /* I should reset the ring buffers here, but I don't yet know
>> * a simple way to do that.
>> @@ -2879,8 +2876,6 @@ fec_enet_open(struct net_device *ndev)
>> err_enet_mii_probe:
>> fec_enet_free_buffers(ndev);
>> err_enet_alloc:
>> - fec_enet_clk_enable(ndev, false);
>> -clk_enable:
>> pm_runtime_mark_last_busy(&fep->pdev->dev);
>> pm_runtime_put_autosuspend(&fep->pdev->dev);
>> pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>> @@ -2907,7 +2902,6 @@ fec_enet_close(struct net_device *ndev)
>>
>> fec_enet_update_ethtool_stats(ndev);
>>
>> - fec_enet_clk_enable(ndev, false);
>> pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>> pm_runtime_mark_last_busy(&fep->pdev->dev);
>> pm_runtime_put_autosuspend(&fep->pdev->dev);
>> @@ -3495,7 +3489,6 @@ fec_probe(struct platform_device *pdev)
>>
>> /* Carrier starts down, phylib will bring it up */
>> netif_carrier_off(ndev);
>> - fec_enet_clk_enable(ndev, false);
>> pinctrl_pm_select_sleep_state(&pdev->dev);
>>
>> ret = register_netdev(ndev);
>>
>
^ permalink raw reply
* Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
From: Florian Fainelli @ 2017-10-22 22:48 UTC (permalink / raw)
To: Richard Leitner, Richard Leitner, fugang.duan
Cc: netdev, linux-kernel, Richard Leitner, Andrew Lunn
In-Reply-To: <1a54f090-be13-a82d-7d5c-1c76f45df4a8@g0hl1n.net>
On 10/22/2017 03:30 PM, Richard Leitner wrote:
>
> On 10/22/2017 08:31 PM, Florian Fainelli wrote:
>>
>> On 10/22/2017 06:11 AM, Richard Leitner wrote:
>>> From: Richard Leitner <richard.leitner@skidata.com>
>>>
>>> From: Richard Leitner <richard.leitner@skidata.com>
>>>
>>> Some PHYs (for example the LAN8710) doesn't allow turning the ethernet
>>> ref clocks off and on again without reset (according to their
>>> datasheet).
>>> Exactly this behaviour was introduced for power saving reasons by
>>> commit e8fcfcd5684a ("net: fec: optimize the clock management to
>>> save...")
>>
>> Please don't truncate the commit subject, put it in full length.
>
> Ok. So I can then ignore the checkpatch.pl error complaining about too
> long lines?
Yes absolutely it's just a tool, not the absolute reference.
>
>>
>>> Therefore remove those en/disables of the refclk during probe, open and
>>> close of the fec.
>>>
>>> Generally speaking is this issue only be relevant if the ref clk for
>>> the PHY is generated by the SoC. In our specific case (PCB) this problem
>>> does occur at about every 10th to 50th POR of an LAN8710 connected to an
>>> i.MX6 SoC. The typical symptom of this problem is a "swinging" ethernet
>>> link. Similar issues were experienced by users of the NXP forum:
>>> https://community.nxp.com/thread/389902
>>> https://community.nxp.com/message/309354
>>> With this patch applied the issue didn't occur for at least a few
>>> thousand PORs of our board.
>>
>> FWIW, this is a common problem on most Ethernet systems attached to a
>> PHY because of the clock dependencies between the MAC and PHY, in either
>> direction really. We recently stumbled across a similar problem on the
>> MAC side (using bcmgenet) where after shutting down the PHY in
>> ndo_stop() we would not get enough clock cycles for the MAC to cleanly
>> shutdown and upon subsequent ndo_open() we could get the MAC in an
>> invalid state.
>
> Hmm... So I'm not the only one struggling with this... Was there already
> discussion/ideas on how to solve this issue once and for all across the
> kernel?
Not really, there is not unfortunately a one size fits all solution.
>
> Andrew Lunn suggested to make the PHY driver a clock driver and let it
> export the refclk... But IMHO that "feels" a bit strange. Due to the
> fact in our case the clock is generated by the SoC and the PHY is an
> external chip which only consumes it.
It depends which *MII mode you use. Unless you are using reverse MII,
the PHY supplies the RX clock and the MAC supplies the RX clock, so
solving this for all modes becomes quite tricky. Andrew's suggestion
makes sense, but considering that the MAC and PHY need to be connected
to each other, it may be tricky to request the PHY's clock before you
actually attached to the PHY...
>
> But back to this patch: Is it OK the way it fixes the issue?
Fugang and Andrew probably know this hardware a lot better, I would have
to look at the code path a bit more to understand if an alternative
solution is possible. It sounds like your patch could create a power
consumption regression, so maybe add a check for the PHY ID that is
problematic by doing something like:
if (priv->phydev->drv && priv->phydev->drv->phy_id == XXXX_XXXX) where
XXXX_XXXX is the LAN870 PHY ID (obtained from MII register 2 & 3).
>
>>
>>>
>>> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
>>
>> Likewise here, not truncation of the commit subject necessary.
>>
>>> Cc: stable@vger.kernel.org
>>
>> netdev patches are handled directly by David Miller, see netdev-FAQ for
>> details.
>
> Ok. So I shouldn't post them to stable@vger.kernel.org at all?
Nope.
>
>>
>>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>>> ---
>>> drivers/net/ethernet/freescale/fec_main.c | 7 -------
>>> 1 file changed, 7 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>> b/drivers/net/ethernet/freescale/fec_main.c
>>> index 3dc2d771a222..8f696b53d8b8 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -2844,9 +2844,6 @@ fec_enet_open(struct net_device *ndev)
>>> return ret;
>>> pinctrl_pm_select_default_state(&fep->pdev->dev);
>>> - ret = fec_enet_clk_enable(ndev, true);
>>> - if (ret)
>>> - goto clk_enable;
>>> /* I should reset the ring buffers here, but I don't yet know
>>> * a simple way to do that.
>>> @@ -2879,8 +2876,6 @@ fec_enet_open(struct net_device *ndev)
>>> err_enet_mii_probe:
>>> fec_enet_free_buffers(ndev);
>>> err_enet_alloc:
>>> - fec_enet_clk_enable(ndev, false);
>>> -clk_enable:
>>> pm_runtime_mark_last_busy(&fep->pdev->dev);
>>> pm_runtime_put_autosuspend(&fep->pdev->dev);
>>> pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>>> @@ -2907,7 +2902,6 @@ fec_enet_close(struct net_device *ndev)
>>> fec_enet_update_ethtool_stats(ndev);
>>> - fec_enet_clk_enable(ndev, false);
>>> pinctrl_pm_select_sleep_state(&fep->pdev->dev);
>>> pm_runtime_mark_last_busy(&fep->pdev->dev);
>>> pm_runtime_put_autosuspend(&fep->pdev->dev);
>>> @@ -3495,7 +3489,6 @@ fec_probe(struct platform_device *pdev)
>>> /* Carrier starts down, phylib will bring it up */
>>> netif_carrier_off(ndev);
>>> - fec_enet_clk_enable(ndev, false);
>>> pinctrl_pm_select_sleep_state(&pdev->dev);
>>> ret = register_netdev(ndev);
>>>
>>
--
Florian
^ permalink raw reply
* Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close
From: Richard Leitner @ 2017-10-22 23:01 UTC (permalink / raw)
To: Florian Fainelli, Richard Leitner, fugang.duan
Cc: netdev, linux-kernel, Richard Leitner, Andrew Lunn
In-Reply-To: <f0bf60b0-b2fb-9ed3-7800-2a5c6bb6ead8@gmail.com>
On 10/23/2017 12:48 AM, Florian Fainelli wrote> On 10/22/2017 03:30 PM,
Richard Leitner wrote:
>> On 10/22/2017 08:31 PM, Florian Fainelli wrote:
>>> On 10/22/2017 06:11 AM, Richard Leitner wrote:
...
>> Andrew Lunn suggested to make the PHY driver a clock driver and let it
>> export the refclk... But IMHO that "feels" a bit strange. Due to the
>> fact in our case the clock is generated by the SoC and the PHY is an
>> external chip which only consumes it.
>
> It depends which *MII mode you use. Unless you are using reverse MII,
> the PHY supplies the RX clock and the MAC supplies the RX clock, so
> solving this for all modes becomes quite tricky. Andrew's suggestion
> makes sense, but considering that the MAC and PHY need to be connected
> to each other, it may be tricky to request the PHY's clock before you
> actually attached to the PHY...
>>
>> But back to this patch: Is it OK the way it fixes the issue?
>
> Fugang and Andrew probably know this hardware a lot better, I would have
> to look at the code path a bit more to understand if an alternative
> solution is possible. It sounds like your patch could create a power
> consumption regression, so maybe add a check for the PHY ID that is
> problematic by doing something like:
>
> if (priv->phydev->drv && priv->phydev->drv->phy_id == XXXX_XXXX) where
> XXXX_XXXX is the LAN870 PHY ID (obtained from MII register 2 & 3).
I already had a patch which does this check and triggers a reset of the
PHY after the refclk was enabled (in fec_enet_clk_enable) in case the
phy_id matches. This would IMHO avoid all power consumption
regressions... It was something like:
switch (priv->phydev->drv->phy_id) {
case XXXX:
case XXXX:
ret = fec_reset_phy(ndev);
if (ret)
goto failed_reset;
if (ndev->phydev) {
ret = phy_init_hw(ndev->phydev);
if (ret)
goto failed_reset;
}
}
Would that be better?
Basically I'm fine with any kind of solution... It's up to you guys...
You're the experts ;-)
^ permalink raw reply
* Re: [jkirsher/net-queue PATCH] i40e: Add programming descriptors to cleaned_count
From: Paweł Staszewski @ 2017-10-22 23:15 UTC (permalink / raw)
To: Anders K. Pedersen | Cohaesio, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org, alexander.duyck@gmail.com
In-Reply-To: <1508695307.4970.32.camel@cohaesio.com>
Yes can confirm that after adding patch:
[jkirsher/net-queue PATCH] i40e: Add programming descriptors to
cleaned_count
There is no memleak.
W dniu 2017-10-22 o 20:01, Anders K. Pedersen | Cohaesio pisze:
> On lør, 2017-10-21 at 18:12 -0700, Alexander Duyck wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> This patch updates the i40e driver to include programming descriptors
>> in
>> the cleaned_count. Without this change it becomes possible for us to
>> leak
>> memory as we don't trigger a large enough allocation when the time
>> comes to
>> allocate new buffers and we end up overwriting a number of rx_buffers
>> equal
>> to the number of programming descriptors we encountered.
>>
>> Fixes: 0e626ff7ccbf ("i40e: Fix support for flow director programming
>> status")
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> This patch solves the remaining memory leak we've seen, so
>
> Tested-by: Anders K. Pedersen <akp@cohaesio.com>
>
> Regards,
> Anders
^ permalink raw reply
* Re: [PATCH v2 net-next] selftests/bpf: fix broken build of test_maps
From: David Miller @ 2017-10-23 0:06 UTC (permalink / raw)
To: ast; +Cc: daniel, brakmo, netdev
In-Reply-To: <20171022172906.3131400-1-ast@fb.com>
From: Alexei Starovoitov <ast@fb.com>
Date: Sun, 22 Oct 2017 10:29:06 -0700
> fix multiple build errors and warnings
>
> 1.
> test_maps.c: In function ‘test_map_rdonly’:
> test_maps.c:1051:30: error: ‘BPF_F_RDONLY’ undeclared (first use in this function)
> MAP_SIZE, map_flags | BPF_F_RDONLY);
>
> 2.
> test_maps.c:1048:6: warning: unused variable ‘i’ [-Wunused-variable]
> int i, fd, key = 0, value = 0;
>
> 3.
> test_maps.c:1087:2: error: called object is not a function or function pointer
> assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == EPERM);
>
> 4.
> ./bpf_helpers.h:72:11: error: use of undeclared identifier 'BPF_FUNC_getsockopt'
> (void *) BPF_FUNC_getsockopt;
>
> Fixes: e043325b3087 ("bpf: Add tests for eBPF file mode")
> Fixes: 6e71b04a8224 ("bpf: Add file mode configuration into bpf maps")
> Fixes: cd86d1fd2102 ("bpf: Adding helper function bpf_getsockops")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Applied, thanks Alexei.
^ permalink raw reply
* Re: [MERGE README] net --> net-next
From: David Miller @ 2017-10-23 0:16 UTC (permalink / raw)
To: daniel; +Cc: netdev, ast, john.fastabend
In-Reply-To: <59ECF46D.30903@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Sun, 22 Oct 2017 21:41:33 +0200
> On 10/22/2017 03:09 PM, Daniel Borkmann wrote:
>> On 10/22/2017 02:57 PM, David Miller wrote:
>>>
>>> There were quite a few BPF conflicts during the merge I just did of
>>> 'net' into 'net-next'.
>>>
>>> In particular, all of the packet pointer branch tests in the verifier
>>> had to be resolved wrt. three different sets of changes.
>>>
>>> The off-by-one stuff. The allowance of the 'data_end > ptr + x' form
>>> of packet pointer checks. And finally, the metadata stuff.
>>>
>>> I would really appreciate an audit and double check of my merge work
>>> by the interested parties.
>>
>> I will do a review today in the evening, thanks David!
>
> Looks good overall, I notices two things (in find_good_pkt_pointers()
> in the second loop the max_t(u16, ...) still exists instead of just
> max() in -net and in test_verifier the test cases for 'XDP pkt read'
> are split in the middle with other test cases for bpf_exit). I'll send
> a cleanup on Monday for this along with the matches for metadata part.
Thanks for taking a look, I was slightly stumped by the max_t() and decided
to keep it in the merge. If both arms of the operation are u16 then indeed
it isn't necessary.
Thanks again!
^ permalink raw reply
* Re: [PATCH 2/2] net: netrom: refactor code in nr_add_node
From: Gustavo A. R. Silva @ 2017-10-23 0:41 UTC (permalink / raw)
To: walter harms; +Cc: Ralf Baechle, David S. Miller, linux-hams, netdev
In-Reply-To: <59EA2A2D.4040003@bfs.de>
Quoting walter harms <wharms@bfs.de>:
> Am 20.10.2017 18:06, schrieb Gustavo A. R. Silva:
>> Hi Walter,
>>
>> Quoting walter harms <wharms@bfs.de>:
>>
>>> Am 19.10.2017 19:27, schrieb Gustavo A. R. Silva:
>>>> Code refactoring in order to make the code easier to read and maintain.
>>>>
>>>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>>>> ---
>>>> This code was tested by compilation only (GCC 7.2.0 was used).
>>>>
>>>> net/netrom/nr_route.c | 63
>>>> ++++++++++++++++-----------------------------------
>>>> 1 file changed, 20 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
>>>> index fc9cadc..1e5165f 100644
>>>> --- a/net/netrom/nr_route.c
>>>> +++ b/net/netrom/nr_route.c
>>>> @@ -80,6 +80,23 @@ static struct nr_neigh
>>>> *nr_neigh_get_dev(ax25_address *callsign,
>>>>
>>>> static void nr_remove_neigh(struct nr_neigh *);
>>>>
>>>> +/* re-sort the routes in quality order. */
>>>> +static inline void re_sort_routes(struct nr_node *nr_node, int ix_x,
>>>> int ix_y)
>>>> +{
>>>> + struct nr_route nr_route;
>>>> +
>>>> + if (nr_node->routes[ix_y].quality >
>>>> nr_node->routes[ix_x].quality) {
>>>> + if (nr_node->which == ix_x)
>>>> + nr_node->which = ix_y;
>>>> + else if (nr_node->which == ix_y)
>>>> + nr_node->which = ix_x;
>>>> +
>>>> + nr_route = nr_node->routes[ix_x];
>>>> + nr_node->routes[ix_x] = nr_node->routes[ix_y];
>>>> + nr_node->routes[ix_y] = nr_route;
>>>> + }
>>>> +}
>>>> +
>>>
>>>
>>> Good idea, a bit of nit picking ..
>>> does ix_ has a special meaning ? otherwise x,y would be sufficient.
>>
>> ix typical stands for index, but I think just x and y are fine too.
>>
>>> From the code below i can see: y=x+1 Perhaps that can be used.
>>>
>>
>> So are you proposing to use two arguments instead of three?
>>
>> re_sort_routes(nr_node, 0);
>>
>
> I am not sure, i would wait a bit and see if what improves readability.
> as Kevin Dawson pointed out: this is a sort here.
> Maybe there a nice way to do something like that (i do not know):
>
> case 3:
> re_sort_routes(nr_node, 1,2)
> case 2:
> re_sort_routes(nr_node, 0,1)
> case 1:
> break;
>
> The question is: Is the sorted list needed or simply the maximum ?
>
> NTL is a good thing to chop down the function in smaller digestible
> peaces.
>
I'll use the swap macro as you suggested and leave the rest of this
patch as is for now.
Dawson:
Thanks for pointing out the thing about the inline keyword. I'll remove it.
Thanks
--
Gustavo A. R. Silva
> re,
> wh
>
>>
>>> kernel.h has a swap() macro. so you can
>>> swap(nr_node->routes[x],nr_node->routes[y]);
>>>
>>
>> Nice, I will use that macro.
>>
>>> hope that helps,
>>
>> Definitely. I appreciate your comments.
>>
>
>
>
>
>> Thanks!
>> --
>> Gustavo A. R. Silva
>>
>>
>>
>>
>>
>>
>>
^ permalink raw reply
* [PATCH v2 2/2] net: netrom: refactor code in nr_add_node
From: Gustavo A. R. Silva @ 2017-10-23 1:08 UTC (permalink / raw)
To: Ralf Baechle, David S. Miller, walter harms
Cc: linux-hams, netdev, linux-kernel, Gustavo A. R. Silva
In-Reply-To: <20171022194136.Horde.CG1baKiAVuDlzYkA6GaU9Xb@gator4166.hostgator.com>
Code refactoring in order to make it easier to read and maintain.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
This code was tested by compilation only.
Changes in v2:
Make use of the swap macro and remove inline keyword.
net/netrom/nr_route.c | 59 ++++++++++++++-------------------------------------
1 file changed, 16 insertions(+), 43 deletions(-)
diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index fc9cadc..505e142 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -80,6 +80,19 @@ static struct nr_neigh *nr_neigh_get_dev(ax25_address *callsign,
static void nr_remove_neigh(struct nr_neigh *);
+/* re-sort the routes in quality order. */
+static void re_sort_routes(struct nr_node *nr_node, int x, int y)
+{
+ if (nr_node->routes[y].quality > nr_node->routes[x].quality) {
+ if (nr_node->which == x)
+ nr_node->which = y;
+ else if (nr_node->which == y)
+ nr_node->which = x;
+
+ swap(nr_node->routes[x], nr_node->routes[y]);
+ }
+}
+
/*
* Add a new route to a node, and in the process add the node and the
* neighbour if it is new.
@@ -90,7 +103,6 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
{
struct nr_node *nr_node;
struct nr_neigh *nr_neigh;
- struct nr_route nr_route;
int i, found;
struct net_device *odev;
@@ -251,50 +263,11 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
/* Now re-sort the routes in quality order */
switch (nr_node->count) {
case 3:
- if (nr_node->routes[1].quality > nr_node->routes[0].quality) {
- switch (nr_node->which) {
- case 0:
- nr_node->which = 1;
- break;
- case 1:
- nr_node->which = 0;
- break;
- }
- nr_route = nr_node->routes[0];
- nr_node->routes[0] = nr_node->routes[1];
- nr_node->routes[1] = nr_route;
- }
- if (nr_node->routes[2].quality > nr_node->routes[1].quality) {
- switch (nr_node->which) {
- case 1: nr_node->which = 2;
- break;
-
- case 2: nr_node->which = 1;
- break;
-
- default:
- break;
- }
- nr_route = nr_node->routes[1];
- nr_node->routes[1] = nr_node->routes[2];
- nr_node->routes[2] = nr_route;
- }
+ re_sort_routes(nr_node, 0, 1);
+ re_sort_routes(nr_node, 1, 2);
/* fall through */
case 2:
- if (nr_node->routes[1].quality > nr_node->routes[0].quality) {
- switch (nr_node->which) {
- case 0: nr_node->which = 1;
- break;
-
- case 1: nr_node->which = 0;
- break;
-
- default: break;
- }
- nr_route = nr_node->routes[0];
- nr_node->routes[0] = nr_node->routes[1];
- nr_node->routes[1] = nr_route;
- }
+ re_sort_routes(nr_node, 0, 1);
case 1:
break;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 2/2] net: netrom: refactor code in nr_add_node
From: David Miller @ 2017-10-23 1:18 UTC (permalink / raw)
To: garsilva; +Cc: ralf, wharms, linux-hams, netdev, linux-kernel
In-Reply-To: <20171023010840.GA32460@embeddedor.com>
From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date: Sun, 22 Oct 2017 20:08:40 -0500
> Code refactoring in order to make it easier to read and maintain.
>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
Gustavo, always when reposting a new version of a patch that is part of
a series you must _always_ repost the entire patch series.
Also, a proper patch series must begine with a "[PATCH 0/2] ..."
header posting explaining at a high level what the patch series
is doing, how it is doing it, and why it is doing it that way.
Thank you.
^ permalink raw reply
* Re: [PATCH v2 2/2] net: netrom: refactor code in nr_add_node
From: Gustavo A. R. Silva @ 2017-10-23 1:39 UTC (permalink / raw)
To: David Miller; +Cc: ralf, wharms, linux-hams, netdev, linux-kernel
In-Reply-To: <20171023.021813.1166611788274727571.davem@davemloft.net>
Quoting David Miller <davem@davemloft.net>:
> From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
> Date: Sun, 22 Oct 2017 20:08:40 -0500
>
>> Code refactoring in order to make it easier to read and maintain.
>>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>
> Gustavo, always when reposting a new version of a patch that is part of
> a series you must _always_ repost the entire patch series.
>
OK. I got it.
> Also, a proper patch series must begine with a "[PATCH 0/2] ..."
> header posting explaining at a high level what the patch series
> is doing, how it is doing it, and why it is doing it that way.
>
Yeah, in this case I thought there was no need for this as both
patches are not actually related in terms of functionality. But now
that I'm writing this, maybe that is precisely the reason why I should
have posted such header...?
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply
* Re: Regression in throughput between kvm guests over virtual bridge
From: Jason Wang @ 2017-10-23 2:06 UTC (permalink / raw)
To: Matthew Rosato, Wei Xu, mst; +Cc: netdev, davem
In-Reply-To: <c0b42b27-56f6-c2f9-9476-28d25678808a@linux.vnet.ibm.com>
On 2017年10月19日 04:17, Matthew Rosato wrote:
>> 2. It might be useful to short the traffic path as a reference, What I am running
>> is briefly like:
>> pktgen(host kernel) -> tap(x) -> guest(DPDK testpmd)
>>
>> The bridge driver(br_forward(), etc) might impact performance due to my personal
>> experience, so eventually I settled down with this simplified testbed which fully
>> isolates the traffic from both userspace and host kernel stack(1 and 50 instances,
>> bridge driver, etc), therefore reduces potential interferences.
>>
>> The down side of this is that it needs DPDK support in guest, has this ever be
>> run on s390x guest? An alternative approach is to directly run XDP drop on
>> virtio-net nic in guest, while this requires compiling XDP inside guest which needs
>> a newer distro(Fedora 25+ in my case or Ubuntu 16.10, not sure).
>>
> I made an attempt at DPDK, but it has not been run on s390x as far as
> I'm aware and didn't seem trivial to get working.
>
> So instead I took your alternate suggestion & did:
> pktgen(host) -> tap(x) -> guest(xdp_drop)
>
> When running this setup, I am not able to reproduce the regression. As
> mentioned previously, I am also unable to reproduce when running one end
> of the uperf connection from the host - I have only ever been able to
> reproduce when both ends of the uperf connection are running within a guest.
>
Thanks for the test. Looking at the code, the only obvious difference
when BATCH is 1 is that one spinlock which was previously called by
tun_peek_len() was avoided since we can do it locally. I wonder whether
or not this speeds up handle_rx() a little more then leads more wakeups
during some rates/sizes of TCP stream. To prove this, maybe you can try:
- enable busy polling, using poll-us=1000, and to see if we can still
get the regression
- measure the pps pktgen(vm1) -> tap1 -> bridge -> tap2 -> vm2
Michael, any another possibility in your mind?
Thanks
^ permalink raw reply
* Re: [PATCH net-next v2 0/7] net: dsa: bcm_sf2: Add support for IPv6 CFP rules
From: David Miller @ 2017-10-23 2:11 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, andrew, vivien.didelot
In-Reply-To: <20171020213949.16485-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 20 Oct 2017 14:39:42 -0700
> This patch series adds support for matching IPv6 addresses to the existing CFP
> support code. Because IPv6 addresses are four times bigger than IPv4, we can
> fit them anymore in a single slice, so we need to chain two in order to have
> a complete match. This makes us require a second bitmap tracking unique rules
> so we don't over populate the TCAM.
>
> Finally, because the code had to be re-organized, it became a lot easier to
> support arbitrary prefix/mask lengths, so the last two patches do just that.
Series applied, thanks Florian.
^ permalink raw reply
* Re: [PATCH] isdn/gigaset: Provide cardstate context for bas timer callbacks
From: David Miller @ 2017-10-23 2:12 UTC (permalink / raw)
To: keescook; +Cc: pebolle, isdn, johan, linux-kernel, gigaset307x-common, netdev
In-Reply-To: <20171020204708.GA58150@beast>
From: Kees Cook <keescook@chromium.org>
Date: Fri, 20 Oct 2017 13:47:08 -0700
> While the work callback uses the urb to find cardstate from bas_cardstate,
> this may not be valid for timer callbacks. Instead, introduce a direct
> pointer back to the cardstate from bas_cardstate for use in timer
> callbacks.
>
> Reported-by: Paul Bolle <pebolle@tiscali.nl>
> Fixes: 4cfea08e6251 ("isdn/gigaset: Convert timers to use timer_setup()")
> Cc: Paul Bolle <pebolle@tiscali.nl>
> Cc: Karsten Keil <isdn@linux-pingi.de>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: gigaset307x-common@lists.sourceforge.net
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Applied to net-next.
^ permalink raw reply
* Re: Regression in throughput between kvm guests over virtual bridge
From: Michael S. Tsirkin @ 2017-10-23 2:13 UTC (permalink / raw)
To: Jason Wang; +Cc: Matthew Rosato, Wei Xu, netdev, davem
In-Reply-To: <376f8939-1990-abf6-1f5f-57b3822f94fe@redhat.com>
On Mon, Oct 23, 2017 at 10:06:36AM +0800, Jason Wang wrote:
>
>
> On 2017年10月19日 04:17, Matthew Rosato wrote:
> > > 2. It might be useful to short the traffic path as a reference, What I am running
> > > is briefly like:
> > > pktgen(host kernel) -> tap(x) -> guest(DPDK testpmd)
> > >
> > > The bridge driver(br_forward(), etc) might impact performance due to my personal
> > > experience, so eventually I settled down with this simplified testbed which fully
> > > isolates the traffic from both userspace and host kernel stack(1 and 50 instances,
> > > bridge driver, etc), therefore reduces potential interferences.
> > >
> > > The down side of this is that it needs DPDK support in guest, has this ever be
> > > run on s390x guest? An alternative approach is to directly run XDP drop on
> > > virtio-net nic in guest, while this requires compiling XDP inside guest which needs
> > > a newer distro(Fedora 25+ in my case or Ubuntu 16.10, not sure).
> > >
> > I made an attempt at DPDK, but it has not been run on s390x as far as
> > I'm aware and didn't seem trivial to get working.
> >
> > So instead I took your alternate suggestion & did:
> > pktgen(host) -> tap(x) -> guest(xdp_drop)
> >
> > When running this setup, I am not able to reproduce the regression. As
> > mentioned previously, I am also unable to reproduce when running one end
> > of the uperf connection from the host - I have only ever been able to
> > reproduce when both ends of the uperf connection are running within a guest.
> >
>
> Thanks for the test. Looking at the code, the only obvious difference when
> BATCH is 1 is that one spinlock which was previously called by
> tun_peek_len() was avoided since we can do it locally. I wonder whether or
> not this speeds up handle_rx() a little more then leads more wakeups during
> some rates/sizes of TCP stream. To prove this, maybe you can try:
>
> - enable busy polling, using poll-us=1000, and to see if we can still get
> the regression
> - measure the pps pktgen(vm1) -> tap1 -> bridge -> tap2 -> vm2
>
> Michael, any another possibility in your mind?
>
> Thanks
Not really. I still suspect since it's s390 only there's
some kind of race condition where we wake up a task repeatedly.
--
MST
^ permalink raw reply
* RE: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic
From: Chris Mi @ 2017-10-23 2:47 UTC (permalink / raw)
To: Cong Wang, Jamal Hadi Salim
Cc: Linux Kernel Network Developers, Lucas Bates, Jiri Pirko,
David Miller
In-Reply-To: <CAM_iQpVMLJjyuSgGHZycgzRz6KHusyTODOAMbw2dj7jTb7bbQQ@mail.gmail.com>
> -----Original Message-----
> From: Cong Wang [mailto:xiyou.wangcong@gmail.com]
> Sent: Friday, October 20, 2017 11:00 AM
> To: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Chris Mi <chrism@mellanox.com>; Linux Kernel Network Developers
> <netdev@vger.kernel.org>; Lucas Bates <lucasb@mojatatu.com>; Jiri Pirko
> <jiri@resnulli.us>; David Miller <davem@davemloft.net>
> Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and
> bindcnt to atomic
>
> On Thu, Oct 19, 2017 at 7:21 AM, Jamal Hadi Salim <jhs@mojatatu.com>
> wrote:
> > On 17-10-18 12:43 PM, Cong Wang wrote:
> >>
> >> On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi <chrism@mellanox.com> wrote:
> >>>>
> >>>> -----Original Message-----
> >
> >
> >>
> >> You listed 3 problems, and you think they are 3 different ones, here
> >> I argue problem 3 (using RCU callbacks) is the cause of problem 1
> >> (refcnt not atomic). This is why I mentioned I have been thinking
> >> about removing RCU callbacks, because it probably could fix all of them.
> >>
> >
> > Cong,
> > Given this is a known bug (the test case Chris presented crashes the
> > kernel) - would it make sense to have a patch that goes to -net to fix
> > this while your approach and discussion outcome goes into net-next?
>
> I am not sure. Because Chris' patchset is large too and I don't think it could fix
> all crashes, so it has little value to just apply them for -net.
It seems it is not easy to discard call_rcu(). I'm afraid even if we have a final solution
without call_rcu(), it is not mature at the beginning as well. I mean we also need time
to fix the possible bugs of the new design. And maybe to destroy the filters in parallel
is the right direction. If this bug is the last bug brought by call_rcu(), then changing it
may not be a good idea.
Patch 1 is straightforward to use atomic. Patch 2 is to convert the list to array.
I think there is no harm to the new design. Patch 3 and 4 are useful test case.
We also need it with new design to make sure there is no regression.
So I think my patch set should not be held so long time.
My two cents.
^ permalink raw reply
* Re: [net-next] tcp: do tcp_mstamp_refresh before retransmits on TSQ handler
From: Koichiro Den @ 2017-10-23 3:26 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem, ncardwell
In-Reply-To: <1508692270.30291.53.camel@edumazet-glaptop3.roam.corp.google.com>
On Sun, 2017-10-22 at 10:11 -0700, Eric Dumazet wrote:
> On Sun, 2017-10-22 at 09:49 -0700, Eric Dumazet wrote:
>
> > Here is the test I cooked.
> >
> > Note that it actually passes just fine, do I am wondering if your patch
> > is needed after all..
> >
>
> Oh this is because tcp_mstamp_refresh() is called from tcp_write_xmit()
> and tcp_write_xmit() is directly called from tcp_tsq_handler()
Thanks for all you help. I guess we should focus on retransmit call path
which comes just before that, or not?
>
> So it looks your patch has no observable effect.
>
> (Even with usec TS as we use here at Google, it has no effect since TCP
> stack does not consider RTT samples being valid if skb was ever
> retransmitted)
>
>
Is the point about tcp_rack_advance part or in general?
Btw, perf probe makes the original point described in my commit message apparent
as follows:
$ perf probe 'T1=tcp_write_xmit:25 tm=+1464(%bx):u64'
$ perf probe 'T2=tcp_xmit_retransmit_queue+0
l_o=+1664(%di):u32 # tp->lost_out
r_o=+1520(%di):u32 # tp->retrans_out
tm=+1464(%di):u64' # skb_mstamp will be set to this.
$ perf trace -aT --no-syscalls --call-graph dwarf \
-e probe:T1/max-stack=4/
-e probe:T2/max-stack=1/ |& \
sed 's/..kernel.kallsyms..//;s/probe://' # to make it a bit more readable :p
$ # run fq-pacing-tsq-rtx-ts.pkt Eric provided:
[before patch applied]
-----8<------8<-----
2560581.604 T2:(ffffffff817da5e0) l_o=25 r_o=0 tm=2560581404)
tcp_xmit_retransmit_queue.part.33
tcp_xmit_recovery.part.53
2560581.614 T1:(ffffffff817d7e74) tm=2560581612) # <--- (1)
tcp_write_xmit
__tcp_push_pending_frames
tcp_rcv_established
tcp_v4_do_rcv
/*
* ^
* | could be unexpectedly long, e.g. due to BLOCK_SOFTIRQ
* v
*/
2560582.215 T2:(ffffffff817da5e0) l_o=25 r_o=5 tm=2560581612)
/* <- same as (1) above */
tcp_xmit_retransmit_queue.part.33
tcp_tasklet_func
----->8------->8-----
[after patch applied]
-----8<------8<-----
97846.150 T2:(ffffffff817da5e0) l_o=25 r_o=0 tm=97846132)
tcp_xmit_retransmit_queue.part.33
tcp_xmit_recovery.part.53
97846.159 T1:(ffffffff817d7e74) tm=97846158) # <--- (1)
tcp_write_xmit
__tcp_push_pending_frames
tcp_rcv_established
tcp_v4_do_rcv
/*
* ^
* | could be unexpectedly long, e.g. due to BLOCK_SOFTIRQ
* v
*/
97847.144 T2:(ffffffff817da5e0) l_o=25 r_o=5 tm=97847134)
/* <- not the same as (1).
* never affected now that we ensure it's refreshed */
tcp_xmit_retransmit_queue.part.33
tcp_tasklet_func
----->8------->8-----
Now I wonder this is more of a theoretical one rather than a patch to fix one
specific bug.
^ permalink raw reply
* Re: [net-next] tcp: do tcp_mstamp_refresh before retransmits on TSQ handler
From: Eric Dumazet @ 2017-10-23 3:40 UTC (permalink / raw)
To: Koichiro Den; +Cc: netdev, davem, ncardwell
In-Reply-To: <1508729177.2898.5.camel@klaipeden.com>
On Mon, 2017-10-23 at 12:26 +0900, Koichiro Den wrote:
> Now I wonder this is more of a theoretical one rather than a patch to fix one
> specific bug.
Note that I said that your patch was fine and I added a 'Reviewed-by:'
tag.
What I meant is that it has no direct effect on correctness of TCP
stack. I could not cook a packetdrill test that shows the difference
before and after your patch.
BTW, in the following sequence :
A) Fetch high-res timestamp and store in X
B) Use X
B) Can use a quite old value of X, depending on scheduling (preempt
kernels or interrupt handling)
TCP really does not care of how accurate X is, it is a best effort.
For RTX packets, it is even more the case, since TCP does not take RTT
samples from packets that were retransmitted.
^ permalink raw reply
* Re: [PATCH net-next 0/6] add a set of tracepoints to tcp stack
From: David Miller @ 2017-10-23 4:08 UTC (permalink / raw)
To: songliubraving; +Cc: netdev, alexei.starovoitov, liu.song.a23
In-Reply-To: <20171020212046.577888-1-songliubraving@fb.com>
From: Song Liu <songliubraving@fb.com>
Date: Fri, 20 Oct 2017 14:20:40 -0700
> These patches add the following tracepoints to tcp stack.
>
> tcp_send_reset
> tcp_receive_reset
> tcp_destroy_sock
> tcp_set_state
Please fix this build error, it probably happens when IPV6 is built
modular:
ERROR: "__tracepoint_tcp_send_reset" [net/ipv6/ipv6.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
Thanks.
^ permalink raw reply
* Re: [patch net-next 0/8] mlxsw: Add support for non-equal-cost multi-path
From: David Miller @ 2017-10-23 4:27 UTC (permalink / raw)
To: jiri; +Cc: netdev, idosch, mlxsw
In-Reply-To: <20171022211150.2567-1-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Sun, 22 Oct 2017 23:11:42 +0200
> From: Jiri Pirko <jiri@mellanox.com>
>
> Ido says:
>
> In the device, nexthops are stored as adjacency entries in an array
> called the KVD linear (KVDL). When a multi-path route is hit the
> packet's headers are hashed and then converted to an index into KVDL
> based on the adjacency group's size and base index.
>
> Up until now the driver ignored the `weight` parameter for multi-path
> routes and allocated only one adjacency entry for each nexthop with a
> limit of 32 nexthops in a group. This set makes the driver take the
> `weight` parameter into account when allocating adjacency entries.
>
> First patch teaches dpipe to show the size of the adjacency group, so
> that users will be able to determine the actual weight of each nexthop.
> The second patch refactors the KVDL allocator, making it more receptive
> towards the addition of another partition later in the set.
>
> Patches 3-5 introduce small changes towards the actual change in the
> sixth patch that populates the adjacency entries according to their
> relative weight.
>
> Last two patches finally add another partition to the KVDL, which allows
> us to allocate more than 32 entries per-group and thus support more
> nexthops and also provide higher accuracy with regards to the requested
> weights.
Series applied, thank you.
^ permalink raw reply
* Re: [net-next] tcp: do tcp_mstamp_refresh before retransmits on TSQ handler
From: Koichiro Den @ 2017-10-23 4:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem, ncardwell
In-Reply-To: <1508730005.30291.70.camel@edumazet-glaptop3.roam.corp.google.com>
On Sun, 2017-10-22 at 20:40 -0700, Eric Dumazet wrote:
> On Mon, 2017-10-23 at 12:26 +0900, Koichiro Den wrote:
>
> > Now I wonder this is more of a theoretical one rather than a patch to fix
> > one
> > specific bug.
>
>
> Note that I said that your patch was fine and I added a 'Reviewed-by:'
> tag.
Sure, sorry about my confusing comment.
>
>
> What I meant is that it has no direct effect on correctness of TCP
> stack. I could not cook a packetdrill test that shows the difference
> before and after your patch.
>
> BTW, in the following sequence :
>
> A) Fetch high-res timestamp and store in X
> B) Use X
>
> B) Can use a quite old value of X, depending on scheduling (preempt
> kernels or interrupt handling)
>
> TCP really does not care of how accurate X is, it is a best effort.
>
I agreed. In e.g., hard interrupt storm, this extra refreshing is just
make the expected delay smaller under the same condition.
> For RTX packets, it is even more the case, since TCP does not take RTT
> samples from packets that were retransmitted.
Indeed, meaning that tcp_clean_rtx_queue implementation never takes.
But for me it seems that there is some possibility RACK algorithm will take it.
^ permalink raw reply
* Re: [PATCH net-next] net: systemport: Guard against unmapped TX ring
From: David Miller @ 2017-10-23 4:29 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev
In-Reply-To: <20171020225930.11907-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 20 Oct 2017 15:59:30 -0700
> Because SYSTEMPORT is a (semi) normal network device, the stack may attempt to
> queue packets on it oustide of the DSA slave transmit path. When that happens,
> the DSA layer has not had a chance to tag packets with the appropriate per-port
> and per-queue information, and if that happens and we don't have a port 0 queue
> 0 available (e.g: on boards where this does not exist), we will hit a NULL
> pointer de-reference in bcm_sysport_select_queue().
>
> Guard against such cases by testing for the TX ring validity.
>
> Fixes: 84ff33eeb23d ("net: systemport: Establish DSA network device queue mapping")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [net-next] tcp: do tcp_mstamp_refresh before retransmits on TSQ handler
From: Eric Dumazet @ 2017-10-23 4:31 UTC (permalink / raw)
To: Koichiro Den; +Cc: netdev, davem, ncardwell
In-Reply-To: <1508732925.2898.7.camel@klaipeden.com>
On Mon, 2017-10-23 at 13:28 +0900, Koichiro Den wrote:
> Indeed, meaning that tcp_clean_rtx_queue implementation never takes.
> But for me it seems that there is some possibility RACK algorithm will take it.
As long as tp->tcp_mstamp is monotonically increasing (and it is ;) ),
RACK will behave properly.
I am not sure we need to discuss this forever frankly ;)
^ permalink raw reply
* Re: [PATCH] net: core: rtnetlink: use BUG_ON instead of if condition followed by BUG
From: David Miller @ 2017-10-23 4:32 UTC (permalink / raw)
To: garsilva; +Cc: netdev, linux-kernel
In-Reply-To: <20171021004311.GA25170@embeddedor.com>
From: "Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date: Fri, 20 Oct 2017 19:43:11 -0500
> Use BUG_ON instead of if condition followed by BUG in do_setlink.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
Applied.
^ permalink raw reply
* Re: [net-next] tcp: do tcp_mstamp_refresh before retransmits on TSQ handler
From: Koichiro Den @ 2017-10-23 4:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem, ncardwell
In-Reply-To: <1508733063.30291.72.camel@edumazet-glaptop3.roam.corp.google.com>
On Sun, 2017-10-22 at 21:31 -0700, Eric Dumazet wrote:
> On Mon, 2017-10-23 at 13:28 +0900, Koichiro Den wrote:
>
> > Indeed, meaning that tcp_clean_rtx_queue implementation never takes.
> > But for me it seems that there is some possibility RACK algorithm will take
> > it.
>
> As long as tp->tcp_mstamp is monotonically increasing (and it is ;) ),
> RACK will behave properly.
>
> I am not sure we need to discuss this forever frankly ;)
>
>
>
Yeah, sure, thanks for your time and helpful comments.
^ 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