* Re: net/dccp: warning in dccp_feat_clone_sp_val/__might_sleep
From: Eric Dumazet @ 2016-10-31 18:40 UTC (permalink / raw)
To: Cong Wang
Cc: Andrey Konovalov, Peter Zijlstra, Gerrit Renker, David S. Miller,
dccp, netdev, LKML, Dmitry Vyukov, Eric Dumazet
In-Reply-To: <CAM_iQpWs1YvrZQF8JTzWR0u0V4t4f3b71WStStGWqDd7agHN_w@mail.gmail.com>
On Mon, 2016-10-31 at 11:00 -0700, Cong Wang wrote:
> On Sun, Oct 30, 2016 at 6:20 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Sun, 2016-10-30 at 05:41 +0100, Andrey Konovalov wrote:
> >> Sorry, the warning is still there.
> >>
> >> I'm not sure adding sched_annotate_sleep() does anything, since it's
> >> defined as (in case CONFIG_DEBUG_ATOMIC_SLEEP is not set):
> >> # define sched_annotate_sleep() do { } while (0)
> >
> > Thanks again for testing.
> >
> > But you do have CONFIG_DEBUG_ATOMIC_SLEEP set, which triggers a check in
> > __might_sleep() :
> >
> > WARN_ONCE(current->state != TASK_RUNNING && current->task_state_change,
> >
> > Relevant commit is 00845eb968ead28007338b2bb852b8beef816583
> > ("sched: don't cause task state changes in nested sleep debugging")
> >
> > Another relevant commit was 26cabd31259ba43f68026ce3f62b78094124333f
> > ("sched, net: Clean up sk_wait_event() vs. might_sleep()")
> >
> > Before release_sock() could process the backlog in process context, only
> > lock_sock() could trigger the issue, so my fix at that time was commit
> > cb7cf8a33ff73cf638481d1edf883d8968f934f8 ("inet: Clean up
> > inet_csk_wait_for_connect() vs. might_sleep()")
> >
>
> Thanks for the context, but isn't the original warning reported by Andrey is
> from inet_wait_for_connect()? You seem only patch some dccp function
> which is why it is still there?
>
> It should be the following, no?
>
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 9648c97..bbd8159 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -544,6 +544,7 @@ static long inet_wait_for_connect(struct sock *sk,
> long timeo, int writebias)
> * without closing the socket.
> */
> while ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) {
> + sched_annotate_sleep();
> release_sock(sk);
> timeo = schedule_timeout(timeo);
> lock_sock(sk);
Yes, this would be one of the locations needing this.
^ permalink raw reply
* [PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
From: John Heenan @ 2016-10-31 18:35 UTC (permalink / raw)
To: Jes Sorensen, Kalle Valo, linux-wireless, netdev; +Cc: linux-kernel
The rtl8723bu wireless IC shows evidence of a more agressive approach to
power saving, powering down its RF side when there is no wireless
interfacing but leaving USB interfacing intact. This makes the wireless
IC more suitable for use in devices which need to keep their power use
as low as practical, such as tablets and Surface Pro type devices.
In effect this means that a full initialisation must be performed
whenever a wireless interface is brought up. It also means that
interpretations of power status from general wireless registers should
not be relied on to influence an init sequence.
The patch works by forcing a fuller initialisation and forcing it to
occur more often in code paths (such as occurs during a low level
authentication that initiates wireless interfacing).
The initialisation sequence is now more consistent with code based
directly on vendor code. For example while the vendor derived code
interprets a register as indcating a particular powered state, it does
not use this information to influence its init sequence.
The rtl8723bu device has a unique USB VID and PID. This is taken
advantage of for the patch to ensure only rtl8723bu devices are affected
by this patch.
With this patch wpa_supplicant reliably and consistently connects with
an AP. Before a workaround such as executing rmmod and modprobe before
each call to wpa_supplicant worked with some distributions.
Signed-off-by: John Heenan <john@zgus.com>
---
.../net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 24 ++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 04141e5..f36e674 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -79,6 +79,8 @@ MODULE_PARM_DESC(dma_agg_pages, "Set DMA aggregation pages (range 1-127, 0 to di
#define RTL8XXXU_TX_URB_LOW_WATER 25
#define RTL8XXXU_TX_URB_HIGH_WATER 32
+#define USB_PRODUCT_ID_RTL8723BU 0xb720
+
static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
struct rtl8xxxu_rx_urb *rx_urb);
@@ -3892,6 +3894,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
u8 val8;
u16 val16;
u32 val32;
+ struct usb_device_descriptor *udesc = &priv->udev->descriptor;
/* Check if MAC is already powered on */
val8 = rtl8xxxu_read8(priv, REG_CR);
@@ -3900,7 +3903,9 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
* Fix 92DU-VC S3 hang with the reason is that secondary mac is not
* initialized. First MAC returns 0xea, second MAC returns 0x00
*/
- if (val8 == 0xea)
+ if (val8 == 0xea
+ || (udesc->idVendor == USB_VENDOR_ID_REALTEK
+ && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU))
macpower = false;
else
macpower = true;
@@ -5776,9 +5781,17 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
struct rtl8xxxu_tx_urb *tx_urb;
unsigned long flags;
int ret, i;
+ struct usb_device_descriptor *udesc = &priv->udev->descriptor;
ret = 0;
+ if(udesc->idVendor == USB_VENDOR_ID_REALTEK
+ && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU) {
+ ret = rtl8xxxu_init_device(hw);
+ if (ret)
+ goto error_out;
+ }
+
init_usb_anchor(&priv->rx_anchor);
init_usb_anchor(&priv->tx_anchor);
init_usb_anchor(&priv->int_anchor);
@@ -6080,9 +6093,12 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
goto exit;
}
- ret = rtl8xxxu_init_device(hw);
- if (ret)
+ if(!(id->idVendor == USB_VENDOR_ID_REALTEK
+ && id->idProduct == USB_PRODUCT_ID_RTL8723BU)) {
+ ret = rtl8xxxu_init_device(hw);
+ if (ret)
goto exit;
+ }
hw->wiphy->max_scan_ssids = 1;
hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN;
@@ -6191,7 +6207,7 @@ static struct usb_device_id dev_table[] = {
/* Tested by Myckel Habets */
{USB_DEVICE_AND_INTERFACE_INFO(0x2357, 0x0109, 0xff, 0xff, 0xff),
.driver_info = (unsigned long)&rtl8192eu_fops},
-{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDOR_ID_REALTEK, 0xb720, 0xff, 0xff, 0xff),
+{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDOR_ID_REALTEK, USB_PRODUCT_ID_RTL8723BU, 0xff, 0xff, 0xff),
.driver_info = (unsigned long)&rtl8723bu_fops},
#ifdef CONFIG_RTL8XXXU_UNTESTED
/* Still supported by rtlwifi */
--
2.10.1
^ permalink raw reply related
* Re: Let's do P4
From: Hannes Frederic Sowa @ 2016-10-31 18:32 UTC (permalink / raw)
To: Jiri Pirko, John Fastabend
Cc: Alexei Starovoitov, Thomas Graf, Jakub Kicinski, netdev, davem,
jhs, roopa, simon.horman, ast, daniel, prem, jbenc, tom, mattyk,
idosch, eladr, yotamg, nogahf, ogerlitz, linville, andy,
f.fainelli, dsa, vivien.didelot, andrew, ivecera,
Maciej Żenczykowski
In-Reply-To: <20161031171229.GB2895@nanopsycho.orion>
On 31.10.2016 18:12, Jiri Pirko wrote:
>> >
>> >In the naive implementation only pipelines that map 1:1 will work. Maybe
>> >this is what Alexei is noticing?
> P4 is ment to program programable hw, not fixed pipeline.
Is it realistic to assume that future hardware might be programmed with
a proprietary (FPGA-alike) bitstream where a generic API wouldn't fit
anymore? I could imagine vendors shipping a higher abstracted
VHDL/Verilog compiler in the future and expect the kernel just forward
it to the hardware as-is.
Bye,
Hannes
^ permalink raw reply
* XDP question - how much can BPF change in xdp_buff?
From: Mintz, Yuval @ 2016-10-31 18:31 UTC (permalink / raw)
To: netdev
So I've [finally] started looking into implementing XDP
for qede, and there's one thing I feel like I'm missing in
regard to XDP_TX - what's the guarantee/requirement
that the bpf program isn't going to transmute some fields
of the rx packet in a way that would prevent the forwarding?
E.g., can a BPF change the TCP payload of an incoming packet
without correcting its TCP checksum, and then expect the
driver to transmit it [via XDP_TX]? If not, how is this enforced [if at all]?
[Looked at samples/bpf/xdp2_kern.c which manipulates the
UDP header; so I'm not certain what prevents it from doing
the same when checksum modifications would be required]
^ permalink raw reply
* Re: [PATCH net] ipv6: add mtu lock check in __ip6_rt_update_pmtu
From: David Miller @ 2016-10-31 18:25 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, hannes
In-Reply-To: <0726e9078aea71d7f6332b4639985565c5e6491b.1477649881.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 28 Oct 2016 18:18:01 +0800
> Prior to this patch, ipv6 didn't do mtu lock check in ip6_update_pmtu.
> It leaded to that mtu lock doesn't really work when receiving the pkt
> of ICMPV6_PKT_TOOBIG.
>
> This patch is to add mtu lock check in __ip6_rt_update_pmtu just as ipv4
> did in __ip_rt_update_pmtu.
>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied, thank you.
^ permalink raw reply
* Re: [RFC PATCH 01/13] pinctrl: meson: Add GXL pinctrl definitions
From: Kevin Hilman @ 2016-10-31 18:23 UTC (permalink / raw)
To: Neil Armstrong
Cc: Linus Walleij, Carlo Caione, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
open list:ARM/Amlogic Meson..., linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
In-Reply-To: <b80010f7-196a-abeb-e4a2-5009f3b7f13b-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> writes:
> On 10/24/2016 03:03 AM, Linus Walleij wrote:
>> On Fri, Oct 21, 2016 at 4:40 PM, Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
>>
>>> Add support for the Amlogic Meson GXL SoC, this is a partially complete
>>> definition only based on the Amlogic Vendor tree.
>>>
>>> This definition differs a lot from the GXBB and needs a separate entry.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>
>> Looks good to me. Tell me when I may apply it, it looks orthogonal
>> to the rest of the patches.
>>
>> Yours,
>> Linus Walleij
>>
>
> Hi Linus,
>
> I'm ok to have it applied as soon as possible, but I can send a clean non-rfc patch if needed.
>
Acked-by: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
I was hoping to have a bit clearer info from the vendor, but based on
vendor BSP, I think we have most of the info, and can do the minor
fixups if/when we get detailed documentation.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: mv643xx_eth.c merge gone bad...
From: Andrew Lunn @ 2016-10-31 18:22 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, jgunthorpe, jarod, netdev
In-Reply-To: <efd31e03-7d18-a41e-bf7f-3c77faa6e825@gmail.com>
On Mon, Oct 31, 2016 at 11:18:54AM -0700, Florian Fainelli wrote:
> On 10/31/2016 07:34 AM, David Miller wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > Date: Mon, 31 Oct 2016 14:35:38 +0100
> >
> >> On Mon, Oct 31, 2016 at 09:12:06AM -0400, David Miller wrote:
> >>> From: Andrew Lunn <andrew@lunn.ch>
> >>> Date: Mon, 31 Oct 2016 12:33:49 +0100
> >>>
> >>>> The merge of commit 27058af401e49d88a905df000dd26f443fcfa8ce for
> >>>> mv643xx_eth.c has gone bad:
> >>>
> >>> Sorry this should be fixed now.
> >>
> >> Hi David
> >>
> >> I think the SET_NETDEV_DEV() should be removed, which is what cause the merged conflict:
> >>
> >> http://patchwork.ozlabs.org/patch/687294/
>
> The SET_NETDEV_DEV() is required for Jason's change to work, since he is
> doing this:
>
> +static int get_phy_mode(struct mv643xx_eth_private *mp)
> +{
> + struct device *dev = mp->dev->dev.parent;
Hi Florian
That requires the SET_NETDEV_DEV() which is in a different hunk of the
original patch, moving it earlier. This is the old one which is being
removed.
Andrew
^ permalink raw reply
* Re: mv643xx_eth.c merge gone bad...
From: Florian Fainelli @ 2016-10-31 18:18 UTC (permalink / raw)
To: David Miller, andrew; +Cc: jgunthorpe, jarod, netdev
In-Reply-To: <20161031.103403.293968855937308643.davem@davemloft.net>
On 10/31/2016 07:34 AM, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Mon, 31 Oct 2016 14:35:38 +0100
>
>> On Mon, Oct 31, 2016 at 09:12:06AM -0400, David Miller wrote:
>>> From: Andrew Lunn <andrew@lunn.ch>
>>> Date: Mon, 31 Oct 2016 12:33:49 +0100
>>>
>>>> The merge of commit 27058af401e49d88a905df000dd26f443fcfa8ce for
>>>> mv643xx_eth.c has gone bad:
>>>
>>> Sorry this should be fixed now.
>>
>> Hi David
>>
>> I think the SET_NETDEV_DEV() should be removed, which is what cause the merged conflict:
>>
>> http://patchwork.ozlabs.org/patch/687294/
The SET_NETDEV_DEV() is required for Jason's change to work, since he is
doing this:
+static int get_phy_mode(struct mv643xx_eth_private *mp)
+{
+ struct device *dev = mp->dev->dev.parent;
--
Florian
^ permalink raw reply
* Re: [PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after encapsulation
From: David Miller @ 2016-10-31 18:18 UTC (permalink / raw)
To: shmulik.ladkani; +Cc: elicooper, tom, netdev
In-Reply-To: <20161028190757.26be756c@halley>
From: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Date: Fri, 28 Oct 2016 19:07:57 +0300
> On Fri, 28 Oct 2016 13:13:45 +0800 Eli Cooper <elicooper@gmx.com> wrote:
>> Maybe we
>> should clear IP6CB in ip6tunnel_xmit(), rather than in every tunnel's codes?
>
> This seems reasonable.
>
> A potential issue might be whether it needs to be done earlier, although
> I've reviewed current versions of both 'ip6_tnl_xmit' and
> 'udp_tunnel6_xmit_skb' and it looks okay. But please verify.
>
>> By the way, I don't see any point in setting IPCB(skb)->flags in
>> udp_tunnel6_xmit_skb(). It will not be interpreted as IPCB any further
>> past ip6tunnel_xmit(), even if it were not cleared. Plus, nothing seems
>> to use these flags anyway.
>
> This seems right.
>
> It was introduced in 6a93cc9052 "udp-tunnel: Add a few more UDP tunnel APIs".
>
> If you checkout that tree, you'll notice same treatment to
> IPCB(skb)->opt and IPCB(skb)->flags in l2tp_xmit_skb... maybe it was
> just copied ;-)
I think for now, we should clear IP6CB in ip6tunnel_xmit()
unconditionally, and remove the IPCB() stuff from the UDP tunneling
code.
If we need some kind of preservation of some of the IP6CB(skb)->flag
bits for whatever reason, we can add that later. Right now that
code was producing essentially garbage.
^ permalink raw reply
* Re: Why do we need tasklet in IFB?
From: David Miller @ 2016-10-31 18:10 UTC (permalink / raw)
To: make0818; +Cc: stephen, netdev, jhs
In-Reply-To: <CAAmHdhyC4DT+Ch2jAhAeCXKoUBH+_q-j96VcZFMO=oBhikN6cg@mail.gmail.com>
From: Michael Ma <make0818@gmail.com>
Date: Mon, 31 Oct 2016 11:02:28 -0700
> 2016-10-28 14:52 GMT-07:00 Michael Ma <make0818@gmail.com>:
>> 2016-10-28 14:48 GMT-07:00 Stephen Hemminger <stephen@networkplumber.org>:
>>> On Fri, 28 Oct 2016 14:45:07 -0700
>>> Michael Ma <make0818@gmail.com> wrote:
>>>
>>>> 2016-10-28 14:38 GMT-07:00 Stephen Hemminger <stephen@networkplumber.org>:
>>>> > On Fri, 28 Oct 2016 14:36:27 -0700
>>>> > Michael Ma <make0818@gmail.com> wrote:
>>>> >
>>>> >> Hi -
>>>> >>
>>>> >> Currently IFB uses tasklet to process tx/rx on the interface that
>>>> >> forwarded the packet to IFB. My understanding on why we're doing this
>>>> >> is that since dev_queue_xmit() can be invoked in interrupt, we want to
>>>> >> defer the processing of original tx/rx in case ifb_xmit() is called
>>>> >> from interrupt.
>>>> >
>>>> > dev_queue_xmit is only called from interrupt if doing netconsole.
>>>>
>
> In fact this doesn't seem to explain since if the original path is tx
> and the context is interrupt, IFB will call dev_queue_xmit as well so
> the context can be interrupt in that case.
>
> Then tasklet is still unnecessary.
Perhaps it's necessary to deal with potential recursion, that's my only
guess at this point.
Jamal, do you remember what went through your mind back in 2005? :-)
^ permalink raw reply
* Re: Why do we need tasklet in IFB?
From: Michael Ma @ 2016-10-31 18:02 UTC (permalink / raw)
To: Stephen Hemminger, Linux Kernel Network Developers
In-Reply-To: <CAAmHdhyC4DT+Ch2jAhAeCXKoUBH+_q-j96VcZFMO=oBhikN6cg@mail.gmail.com>
2016-10-31 11:02 GMT-07:00 Michael Ma <make0818@gmail.com>:
> 2016-10-28 14:52 GMT-07:00 Michael Ma <make0818@gmail.com>:
>> 2016-10-28 14:48 GMT-07:00 Stephen Hemminger <stephen@networkplumber.org>:
>>> On Fri, 28 Oct 2016 14:45:07 -0700
>>> Michael Ma <make0818@gmail.com> wrote:
>>>
>>>> 2016-10-28 14:38 GMT-07:00 Stephen Hemminger <stephen@networkplumber.org>:
>>>> > On Fri, 28 Oct 2016 14:36:27 -0700
>>>> > Michael Ma <make0818@gmail.com> wrote:
>>>> >
>>>> >> Hi -
>>>> >>
>>>> >> Currently IFB uses tasklet to process tx/rx on the interface that
>>>> >> forwarded the packet to IFB. My understanding on why we're doing this
>>>> >> is that since dev_queue_xmit() can be invoked in interrupt, we want to
>>>> >> defer the processing of original tx/rx in case ifb_xmit() is called
>>>> >> from interrupt.
>>>> >
>>>> > dev_queue_xmit is only called from interrupt if doing netconsole.
>>>>
>
> In fact this doesn't seem to explain since if the original path is tx
> and the context is interrupt, IFB will call dev_queue_xmit as well so
> the context can be interrupt in that case.
>
> Then tasklet is still unnecessary.
>
>>>> OK - so the reason is that netif_receive_skb() can only be invoked
>>>> from softirq and we have to use tasklet in IFB to guarantee this.
>>>>
>>>> Then if the original path is rx, tasklet is unnecessary because
>>>> ifb_xmit() is invoked from netif_receive_skb() which is always in the
>>>> softirq context, right?
>>>
>>> The other reason is to avoid deep kernel callstacks
I see - this seems to be the ultimate reason but in case we know the
actual IFB configuration is simple then tasklet can still be avoided,
right?
^ permalink raw reply
* Re: Why do we need tasklet in IFB?
From: Michael Ma @ 2016-10-31 18:02 UTC (permalink / raw)
To: Stephen Hemminger, Linux Kernel Network Developers
In-Reply-To: <CAAmHdhwWGygKtcNR2Bazh-TMoAahEaAf6N5CPWjnvun0BfUdLA@mail.gmail.com>
2016-10-28 14:52 GMT-07:00 Michael Ma <make0818@gmail.com>:
> 2016-10-28 14:48 GMT-07:00 Stephen Hemminger <stephen@networkplumber.org>:
>> On Fri, 28 Oct 2016 14:45:07 -0700
>> Michael Ma <make0818@gmail.com> wrote:
>>
>>> 2016-10-28 14:38 GMT-07:00 Stephen Hemminger <stephen@networkplumber.org>:
>>> > On Fri, 28 Oct 2016 14:36:27 -0700
>>> > Michael Ma <make0818@gmail.com> wrote:
>>> >
>>> >> Hi -
>>> >>
>>> >> Currently IFB uses tasklet to process tx/rx on the interface that
>>> >> forwarded the packet to IFB. My understanding on why we're doing this
>>> >> is that since dev_queue_xmit() can be invoked in interrupt, we want to
>>> >> defer the processing of original tx/rx in case ifb_xmit() is called
>>> >> from interrupt.
>>> >
>>> > dev_queue_xmit is only called from interrupt if doing netconsole.
>>>
In fact this doesn't seem to explain since if the original path is tx
and the context is interrupt, IFB will call dev_queue_xmit as well so
the context can be interrupt in that case.
Then tasklet is still unnecessary.
>>> OK - so the reason is that netif_receive_skb() can only be invoked
>>> from softirq and we have to use tasklet in IFB to guarantee this.
>>>
>>> Then if the original path is rx, tasklet is unnecessary because
>>> ifb_xmit() is invoked from netif_receive_skb() which is always in the
>>> softirq context, right?
>>
>> The other reason is to avoid deep kernel callstacks
^ permalink raw reply
* Re: net/dccp: warning in dccp_feat_clone_sp_val/__might_sleep
From: Cong Wang @ 2016-10-31 18:00 UTC (permalink / raw)
To: Eric Dumazet
Cc: Andrey Konovalov, Peter Zijlstra, Gerrit Renker, David S. Miller,
dccp, netdev, LKML, Dmitry Vyukov, Eric Dumazet
In-Reply-To: <1477833601.7065.297.camel@edumazet-glaptop3.roam.corp.google.com>
On Sun, Oct 30, 2016 at 6:20 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2016-10-30 at 05:41 +0100, Andrey Konovalov wrote:
>> Sorry, the warning is still there.
>>
>> I'm not sure adding sched_annotate_sleep() does anything, since it's
>> defined as (in case CONFIG_DEBUG_ATOMIC_SLEEP is not set):
>> # define sched_annotate_sleep() do { } while (0)
>
> Thanks again for testing.
>
> But you do have CONFIG_DEBUG_ATOMIC_SLEEP set, which triggers a check in
> __might_sleep() :
>
> WARN_ONCE(current->state != TASK_RUNNING && current->task_state_change,
>
> Relevant commit is 00845eb968ead28007338b2bb852b8beef816583
> ("sched: don't cause task state changes in nested sleep debugging")
>
> Another relevant commit was 26cabd31259ba43f68026ce3f62b78094124333f
> ("sched, net: Clean up sk_wait_event() vs. might_sleep()")
>
> Before release_sock() could process the backlog in process context, only
> lock_sock() could trigger the issue, so my fix at that time was commit
> cb7cf8a33ff73cf638481d1edf883d8968f934f8 ("inet: Clean up
> inet_csk_wait_for_connect() vs. might_sleep()")
>
Thanks for the context, but isn't the original warning reported by Andrey is
from inet_wait_for_connect()? You seem only patch some dccp function
which is why it is still there?
It should be the following, no?
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 9648c97..bbd8159 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -544,6 +544,7 @@ static long inet_wait_for_connect(struct sock *sk,
long timeo, int writebias)
* without closing the socket.
*/
while ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV)) {
+ sched_annotate_sleep();
release_sock(sk);
timeo = schedule_timeout(timeo);
lock_sock(sk);
^ permalink raw reply related
* Re: [PATCH] net: phy: at803x: the Atheros 8031 supports pause frames
From: Florian Fainelli @ 2016-10-31 17:55 UTC (permalink / raw)
To: Timur Tabi, netdev, zefir.kurtisi, scampbel, alokc, shankerd,
andrew
In-Reply-To: <58177E17.6070704@codeaurora.org>
On 10/31/2016 10:23 AM, Timur Tabi wrote:
> Florian Fainelli wrote:
>
>> May I suggest reading about standards a bit more, or just looking at
>> other drivers, like tg3.c.
>
> I have been doing that for over six months now. There's only so much I
> can glean from reading source code and standards documents. The inner
> workings of our NIC are a mystery lost to time.
>
>>> Ok, to me that means that the PHY driver must tell phylib whether or not
>>> it supports pause frames. The question becomes:
>>>
>>> 1) Is my patch correct?
>>> 2) Is my patch necessary?
>>> 3) Is my patch sufficient?
>>
>> Your patch is correct but also insufficient, although, as indicated
>> before, there is an misconception with PHY drivers that they should be
>> setting SUPPORTED_*Pause* bits,
>
> If that's a misconception, then why is my patch correct? It modifies
> the PHY driver to set those bits. These drivers do the same thing:
Your patch is not correct nor incorrect, but results in PHYLIB to
advertise pause frames to your link partner, and since you also have
flow control enabled, this produces the desired effect of getting what
you need, and the state is consistend. But as indicated before, this is
not how you are supposed to fix that.
>
> bcm63xx.c
> bcm7xxx.c
> bcm-cygnus.c
> broadcom.c
> icplus.c
> micrel.c
> microchip.c
> national.c
> smsc.c
> ste10Xp.c
Ever heard of cargo cult programming?
>
>> the MAC should do that, based on what it
>> does, anyway, but more importantly you need to have your Ethernet MAC
>> react properly upon what the auto-negotiation and locally configured
>> pause/flow control settings are, and configure the Ethernet MAC
>> accordingly.
>
> Ok, so I should enable support for pause frames in my MAC if
> phydev->pause and/or phydev->asym_pause is set, and disable it otherwise?
You don't read my emails do you? By setting phydev->supported and
phydev->features to include SUPPORTED_Pause and SUPPORTED_AsymPause when
you have *connected* to the PHY.
>
> If I read the spec correct (tx=MAC sends pause frames, rx=MAC
> understands received pause frames),
>
> pause asym_pause enable tx? enable rx?
> ----- ---------- ---------- ----------
> 0 0 No No
> 0 1 Yes No
> 1 0 Yes Yes
> 1 1 No Yes
>
> The last two seem backwards. The internal driver enables RX and TX if
> pause and asym_pause is enabled.
>
>> When you want pause frames to be enabled, you need to advertise them,
>> and in order to do so, you need to set phydev->supported to have
>> SUPPORTED_Pause and SUPPORTED_AsymPause, and call genphy_restart_aneg()
>> to transfer that to a write to the MII_ADVERTISE register, resulting in
>> your link partner to see that you now support Pause frames. Since pause
>> frames are now in use, you want your Ethernet MAC, not to ignore pause
>> frames (have flow control enabled).
>
> Is there a way to enable pause frames without ethtool? I intend to add
> ethtool support to my driver, but I want to fix this last bug first.
Auto-negotiation, with a forced policy to have flow control enabled is
certainly one of these cases.
>
>>> 1) I believe my patch is correct, because many other PHY drivers do the
>>> same thing for the same reason. If the PHY supports register 4 bits 10
>>> and 11, then those SUPPORTED_xxx bits should be set.
>>>
>>> 2) I don't know why my patch appears to be necessary, because I don't
>>> understand why a 1Gb NIC on a 2Ghz processor drops frames. I suspect
>>> the program is not performance but rather a mis-programming.
>>>
>>> 4) Is my patch sufficient? The internal driver always enables pause
>>> frame support in the PHY if the PHY says that pause frames are enabled.
>>> In fact, I can't even turn off flow control in the internal driver:
>>
>> Please stop abusing this "PHY says", the PHY advertises what it is being
>> told to advertise, by the MAC...
>
> Then I need help understanding why there are 10 other PHY drivers that
> set the SUPPORTED_Pause and SUPPORTED_Asym_Pause bits in their .features
> flags.
Cargo cult programming, replicating something you thought was right, or
did the job at the time, did not look back on it, and nobody cared
fixing it.
>
>>> $ sudo ethtool -A eth1 tx off rx off
>>> $ sudo ethtool -a eth1
>>> Pause parameters for eth1:
>>> Autonegotiate: on
>>> RX: on
>>> TX: on
>>>
>>> The driver insists on leaving flow control enabled if autonegotiation is
>>> enabled.
>>
>> Actually, the driver forces the enabling of flow control in the MAC,
>> period, but does not do anything with the auto-negotiation results, or I
>> could not spot it.
>
> Sorry, the internal driver is very different that the external one. It
> lacks phylib support. You can see the internal driver here:
>
> https://source.codeaurora.org/external/thundersoft/kernel/msm-3.10/tree/drivers/net/ethernet/msm/emac?h=caf/LA.BF.2.1.2_rb1.2
I have no interest in looking into something that is not upstream.
>
>
> Flow control is enabled or disabled via the TXFC and RXFC bits in
> function emac_hw_config_fc():
>
> https://source.codeaurora.org/external/thundersoft/kernel/msm-3.10/tree/drivers/net/ethernet/msm/emac/emac_hw.c?h=caf/LA.BF.2.1.2_rb1.2#n1306
>
>
>> This was spotted in the review, but not addressed, but your adjust_link
>> callback seems completely bogus, it does a full MAC stop,
>> reconfiguration and restart,
>
> I've been struggling to figure out what exactly the driver should do
> during adjust_link, although I don't see where it's doing a full
> restart. If the link is up, it just calls emac_mac_start(), which just
> starts the MAC.
"just starts" the MAC is not quite what emac_mac_start() does, it does a
lot more than just programming the duplex and flow control settings, it
also deals with receiver/transmitter enable, touching some queue
configuration, interrupt etc., programs a handful of MAC bits relating
to packet processing etc.. all of this should be unnecessary at the time
you react to a link change, and presumably also needs to be done in a
controlled manner, as the hardware may be acting on a packet boundary
(you may have to wait for a full packet to drain before doing this or that).
All of this is much more than what is expected for a link up/down event.
>
>> that is at best unnecessary and costly, but
>> it also misses a few things and does not act upon reading phydev->pause
>> and phydev->asym_pause to set or clear TXFC and RXFC, that really seems
>> to be your problem here.
>
> Now that I understand (barely) what phydev->pause and phydev->asym_pause
> do, I can modify emac_mac_start() to use those values to set TXFC and RXFC.
>
> Unfortunately, that won't explain why my driver needs the PHY to pass
> those frames to avoid 10% packet loss. You would think a 1Gb NIC on a
> 24-core 2Ghz SOC could handle 900 Mbps.
>
> Note that if I throttle my bandwidth to 90 Mbps, I still get packet
> loss. However, if I switch the link from gigabit to 100Mbps, then I
> don't get packet loss. So it's not the actual throughput that's the
> problem. I get CRC errors in gigabit mode no matter what, if the pause
> frames are blocked by the PHY.
Then really this needs more investigation from your side instead of just
enabling pause frames and making the problem go away.
>
>> Here is what could possibly go wrong right now:
>>
>> - your Ethernet MAC unconditionally enables flow control at the MAC
>> level, but does not advertise support for that in MII_ADVERTISE until
>> you set SUPPORTED_Pause and SUPPORTED_Asym_Pause in the PHY driver, fair
>> enough
>>
>> - the link partner you are testing against does not keep up well with
>> your transmit rates, but supports flow control, so Pause frames that it
>> sends back at you to tell you to stop transmitting so quickly just get
>> ignored, because not being advertised, you experience packet loss
>
> The link partner is an e1000 NIC on an 8-core 3.6GHz PC . Is it
> conceivable that I'm overloading it?
Yes it is, and if you are not directly connected to it, maybe it is the
switch in between, depending on how it deals with flow control you could
run into a problem with the switch. Not all switches support end-to-end
pause frames, and will terminate it locally, and then try to also apply
flow control with the otehr link partner: your e1000 NIC on the other PC.
Flow control issues are kind of difficult to resolve for sure, but
David's reply from a few minutes ago is also how I would approach the
problem.
--
Florian
^ permalink raw reply
* Re: [PATCH v2 net-next 1/5] bpf: Refactor cgroups code in prep for new type
From: Thomas Graf @ 2016-10-31 17:49 UTC (permalink / raw)
To: Daniel Mack; +Cc: David Ahern, David Miller, netdev, ast, daniel, maheshb
In-Reply-To: <7d2a099d-1039-ef2d-86e6-2fca151d316c@zonque.org>
On 10/31/16 at 06:16pm, Daniel Mack wrote:
> On 10/31/2016 06:05 PM, David Ahern wrote:
> > On 10/31/16 11:00 AM, Daniel Mack wrote:
> >> Yeah, I'm confused too. I changed that name in my v7 from
> >> BPF_PROG_TYPE_CGROUP_SOCK to BPF_PROG_TYPE_CGROUP_SKB on David's
> >> (Ahern) request. Why is it now renamed again?
> >
> > Thomas pushed back on adding another program type in favor of using
> > subtypes. So this makes the program type generic to CGROUP and patch
> > 2 in this v2 set added Mickaël's subtype patch with the socket
> > mangling done that way in patch 3.
> >
>
> Fine for me. I can change it around again.
I would like to hear from Daniel B and Alexei as well. We need to
decide whether to use subtypes consistently and treat prog types as
something more high level or whether to bluntly introduce a new prog
type for every distinct set of verifier limits. I will change lwt_bpf
as well accordingly.
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: the Atheros 8031 supports pause frames
From: David Miller @ 2016-10-31 17:48 UTC (permalink / raw)
To: timur; +Cc: netdev, zefir.kurtisi, scampbel, alokc, shankerd, andrew,
f.fainelli
In-Reply-To: <1477605901-30906-1-git-send-email-timur@codeaurora.org>
From: Timur Tabi <timur@codeaurora.org>
Date: Thu, 27 Oct 2016 17:05:01 -0500
> The Atheros 8031 PHY supports the 802.3 extension for symmetric and
> asymmetric pause frames, so set that to the list of features supported
> by the phy.
>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
It looks like Florian and you need to discuss this a little further
but here are some comments on my part.
First of all the PHY state for pause is merely a control for what gets
advertised in negotiation and the result after negotiation completes,
and that's about it. Maybe is has an influence upon whether PAUSE
frames are passed to/from the MAC, but that would be the largest
extent of it even if so.
The MAC does all of the actual PAUSE processing. When the MAC sees a
PAUSE frame is backs off it's transmitter. When the amount of unused
RX buffers in it's ring gets very low, the MAC emits a PAUSE frame.
You also mentioned that you were surprised that getting 900MBit on a
multi-core 2GHZ ARM without drops isn't happening. Well, this is a
very complex issue to analyze. I can only give a few pointers after
taking a quick look at this out-of-tree driver.
Are you testing single-flow performance? If so, even though this is a
multi-queue NIC the traffic will be going over only one of the queues
and thus all of those other cores are basically wasted, because only
one core will be processing this flow's packets.
Next, there are probably a lot of batching optimizations missing from
the driver. For example, unconditionally always posting replenished
RX buffers ever time you process the RX ring is expensive. Especially
expensive is the MMIO write to post the new RX buffers. You should
batch them and only perform the MMIO write when say 8 or more new
RX buffers have been posted.
This is a pretty common optimization if you look at other drivers.
Next, the DMA map/unmap operations could be (relatively) expensive
on this platform and contribute to what packet rates are possible
without drops.
But all of this is speculation, you really need to look at "perf"
output to see if the kernel is spending an excessive amount of time in
one place or another during your tests. At least this way you'll have
some hard data to work with and have some kind of idea what might be
the reason.
^ permalink raw reply
* Re: [PATCH v2 net-next 0/5] Add bpf support to set sk_bound_dev_if
From: Thomas Graf @ 2016-10-31 17:46 UTC (permalink / raw)
To: David Ahern; +Cc: David Miller, netdev, daniel, ast, daniel, maheshb
In-Reply-To: <69b9c20b-9566-5f75-cebd-a0bd243c2d65@cumulusnetworks.com>
On 10/31/16 at 11:16am, David Ahern wrote:
> On 10/31/16 11:01 AM, David Miller wrote:
> > Also, any reason why you don't allow the cgroup bpf sk filter to return
> > an error code so that the sock creation could be cancelled if the eBPF
> > program desires that? It could be useful, I suppose.
>
> My first draft at this feature had that but I removed it for simplicity now. Can certainly add it back.
We're trying to standardize on common return codes for all program
types. The lwt bpf series defines BPF_ codes which are compatible
with TC_ACT_* values to make lwt_bpf and cls_bpf compatible. Would
be great to use the same return codes and implement the ones that
make sense.
^ permalink raw reply
* [PATCH net] bgmac: stop clearing DMA receive control register right after it is set
From: Andy Gospodarek @ 2016-10-31 17:32 UTC (permalink / raw)
To: netdev; +Cc: Andy Gospodarek, Hauke Mehrtens
Current bgmac code initializes some DMA settings in the receive control
register for some hardware and then immediately clears those settings.
Not clearing those settings results in ~420Mbps *improvement* in
throughput; this system can now receive frames at line-rate on Broadcom
5871x hardware compared to ~520Mbps today. I also tested a few other
values but found there to be no discernible difference in CPU
utilization even if burst size and prefetching values are different.
On the hardware tested there was no need to keep the code that cleared
all but bits 16-17, but since there is a wide variety of hardware that
used this driver (I did not look at all hardware docs for hardware using
this IP block), I find it wise to move this call up and clear bits just
after reading the default value from the hardware rather than completely
removing it.
This is a good candidate for -stable >=3.14 since that is when the code
that was supposed to improve performance (but did not) was introduced.
Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
Fixes: 56ceecde1f29 ("bgmac: initialize the DMA controller of core...")
Cc: Hauke Mehrtens <hauke@hauke-m.de>
---
drivers/net/ethernet/broadcom/bgmac.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 31ca204..91cbf92 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -307,6 +307,10 @@ static void bgmac_dma_rx_enable(struct bgmac *bgmac,
u32 ctl;
ctl = bgmac_read(bgmac, ring->mmio_base + BGMAC_DMA_RX_CTL);
+
+ /* preserve ONLY bits 16-17 from current hardware value */
+ ctl &= BGMAC_DMA_RX_ADDREXT_MASK;
+
if (bgmac->feature_flags & BGMAC_FEAT_RX_MASK_SETUP) {
ctl &= ~BGMAC_DMA_RX_BL_MASK;
ctl |= BGMAC_DMA_RX_BL_128 << BGMAC_DMA_RX_BL_SHIFT;
@@ -317,7 +321,6 @@ static void bgmac_dma_rx_enable(struct bgmac *bgmac,
ctl &= ~BGMAC_DMA_RX_PT_MASK;
ctl |= BGMAC_DMA_RX_PT_1 << BGMAC_DMA_RX_PT_SHIFT;
}
- ctl &= BGMAC_DMA_RX_ADDREXT_MASK;
ctl |= BGMAC_DMA_RX_ENABLE;
ctl |= BGMAC_DMA_RX_PARITY_DISABLE;
ctl |= BGMAC_DMA_RX_OVERFLOW_CONT;
--
2.1.0
^ permalink raw reply related
* Re: [PATCH net-next 3/4] bpf: BPF for lightweight tunnel encapsulation
From: Thomas Graf @ 2016-10-31 17:35 UTC (permalink / raw)
To: Tom Herbert
Cc: David S. Miller, Alexei Starovoitov, Daniel Borkmann,
Linux Kernel Network Developers, roopa
In-Reply-To: <CALx6S34hjr9-OV1qiyh+J=zhFdUtmMeC+7S=gj3BR7Z=1vaitA@mail.gmail.com>
On 10/31/16 at 09:07am, Tom Herbert wrote:
> I guess this leads to a more general question I have about the effects
> of allowing userspace to insert code in the kernel that modifies
> packets. If we allow BPF programs to arbitrarily modify packets in
> LWT, how do we ensure that there are no insidious effects later in the
> path? For instance, what someone uses BPF to convert an IPv6 packet
> to IPv4, or maybe convert packet to something that isn't even IP, or
> what if someone just decides to overwrite every byte in a packet with
> 0xff?
This is why modifying packets is not allowed on input at all as it
would invalidate the IP parsing that has already been done.
Writing is allowed for dst_output() on the basis that it is the
equivalent of a raw socket with header inclusion. If you look at
rawv6_send_hdrinc(), it does not perform any validation and calls into
dst_output() directly. I agree though that this must be made water
proof.
Pushing additional headers is only allowed at xmit, this is the
equivalent LWT MPLS.
> Are these thing allowed, and if so what is the effect? I would
> assume a policy that these can't cause any insidious effects to
> unrelated traffic or the rest of the system, in particular such things
> should not cause the kernel to crash (based on the principle that
> user space code should never cause kernel to crash). I think XDP might
Agreed. Although it's already possible to hook a kernel module at LWT
or Netfilter to do arbitrary packet modifications, BPF must be held
at a higher standard even in privileged mode.
^ permalink raw reply
* Re: [PATCH/RFC] ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W
From: David Miller @ 2016-10-31 17:24 UTC (permalink / raw)
To: geert+renesas
Cc: sergei.shtylyov, horms+renesas, arnd, netdev, linux-renesas-soc
In-Reply-To: <1477934018-13968-1-git-send-email-geert+renesas@glider.be>
From: Geert Uytterhoeven <geert+renesas@glider.be>
Date: Mon, 31 Oct 2016 18:13:38 +0100
> The limitation to 10/100Mbit speeds on R-Car Gen3 is valid for R-Car H3
> ES1.0 only. Check for the exact SoC model to allow 1Gbps on newer
> revisions of R-Car H3, and on R-Car M3-W.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Tested on:
> - r8a7795/salvator-x with R-Car H3 ES1.0 (limited to 100Mbps),
> - r8a7795/salvator-x with R-Car H3 ES1.1 (1Gbps),
> - r8a7796/salvator-x with R-Car M3-W ES1.0 (1Gbps).
>
> This is marked as an RFC because it depends on:
> A) the soc_device_match() infrastructure,
> B) Renesas SoC core ESx.y handling.
> Hence I think the best merge strategy is to let this patch go in through
> Simon's Renesas tree.
>
> David: If you agree, can you please provide your ack? Thanks!
Sure, no problem:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: the Atheros 8031 supports pause frames
From: Timur Tabi @ 2016-10-31 17:23 UTC (permalink / raw)
To: Florian Fainelli, netdev, zefir.kurtisi, scampbel, alokc,
shankerd, andrew
In-Reply-To: <144cc092-f04d-f5eb-503c-51e87c214b66@gmail.com>
Florian Fainelli wrote:
> May I suggest reading about standards a bit more, or just looking at
> other drivers, like tg3.c.
I have been doing that for over six months now. There's only so much I
can glean from reading source code and standards documents. The inner
workings of our NIC are a mystery lost to time.
>> Ok, to me that means that the PHY driver must tell phylib whether or not
>> it supports pause frames. The question becomes:
>>
>> 1) Is my patch correct?
>> 2) Is my patch necessary?
>> 3) Is my patch sufficient?
>
> Your patch is correct but also insufficient, although, as indicated
> before, there is an misconception with PHY drivers that they should be
> setting SUPPORTED_*Pause* bits,
If that's a misconception, then why is my patch correct? It modifies
the PHY driver to set those bits. These drivers do the same thing:
bcm63xx.c
bcm7xxx.c
bcm-cygnus.c
broadcom.c
icplus.c
micrel.c
microchip.c
national.c
smsc.c
ste10Xp.c
> the MAC should do that, based on what it
> does, anyway, but more importantly you need to have your Ethernet MAC
> react properly upon what the auto-negotiation and locally configured
> pause/flow control settings are, and configure the Ethernet MAC accordingly.
Ok, so I should enable support for pause frames in my MAC if
phydev->pause and/or phydev->asym_pause is set, and disable it otherwise?
If I read the spec correct (tx=MAC sends pause frames, rx=MAC
understands received pause frames),
pause asym_pause enable tx? enable rx?
----- ---------- ---------- ----------
0 0 No No
0 1 Yes No
1 0 Yes Yes
1 1 No Yes
The last two seem backwards. The internal driver enables RX and TX if
pause and asym_pause is enabled.
> When you want pause frames to be enabled, you need to advertise them,
> and in order to do so, you need to set phydev->supported to have
> SUPPORTED_Pause and SUPPORTED_AsymPause, and call genphy_restart_aneg()
> to transfer that to a write to the MII_ADVERTISE register, resulting in
> your link partner to see that you now support Pause frames. Since pause
> frames are now in use, you want your Ethernet MAC, not to ignore pause
> frames (have flow control enabled).
Is there a way to enable pause frames without ethtool? I intend to add
ethtool support to my driver, but I want to fix this last bug first.
>> 1) I believe my patch is correct, because many other PHY drivers do the
>> same thing for the same reason. If the PHY supports register 4 bits 10
>> and 11, then those SUPPORTED_xxx bits should be set.
>>
>> 2) I don't know why my patch appears to be necessary, because I don't
>> understand why a 1Gb NIC on a 2Ghz processor drops frames. I suspect
>> the program is not performance but rather a mis-programming.
>>
>> 4) Is my patch sufficient? The internal driver always enables pause
>> frame support in the PHY if the PHY says that pause frames are enabled.
>> In fact, I can't even turn off flow control in the internal driver:
>
> Please stop abusing this "PHY says", the PHY advertises what it is being
> told to advertise, by the MAC...
Then I need help understanding why there are 10 other PHY drivers that
set the SUPPORTED_Pause and SUPPORTED_Asym_Pause bits in their .features
flags.
>> $ sudo ethtool -A eth1 tx off rx off
>> $ sudo ethtool -a eth1
>> Pause parameters for eth1:
>> Autonegotiate: on
>> RX: on
>> TX: on
>>
>> The driver insists on leaving flow control enabled if autonegotiation is
>> enabled.
>
> Actually, the driver forces the enabling of flow control in the MAC,
> period, but does not do anything with the auto-negotiation results, or I
> could not spot it.
Sorry, the internal driver is very different that the external one. It
lacks phylib support. You can see the internal driver here:
https://source.codeaurora.org/external/thundersoft/kernel/msm-3.10/tree/drivers/net/ethernet/msm/emac?h=caf/LA.BF.2.1.2_rb1.2
Flow control is enabled or disabled via the TXFC and RXFC bits in
function emac_hw_config_fc():
https://source.codeaurora.org/external/thundersoft/kernel/msm-3.10/tree/drivers/net/ethernet/msm/emac/emac_hw.c?h=caf/LA.BF.2.1.2_rb1.2#n1306
> This was spotted in the review, but not addressed, but your adjust_link
> callback seems completely bogus, it does a full MAC stop,
> reconfiguration and restart,
I've been struggling to figure out what exactly the driver should do
during adjust_link, although I don't see where it's doing a full
restart. If the link is up, it just calls emac_mac_start(), which just
starts the MAC.
> that is at best unnecessary and costly, but
> it also misses a few things and does not act upon reading phydev->pause
> and phydev->asym_pause to set or clear TXFC and RXFC, that really seems
> to be your problem here.
Now that I understand (barely) what phydev->pause and phydev->asym_pause
do, I can modify emac_mac_start() to use those values to set TXFC and RXFC.
Unfortunately, that won't explain why my driver needs the PHY to pass
those frames to avoid 10% packet loss. You would think a 1Gb NIC on a
24-core 2Ghz SOC could handle 900 Mbps.
Note that if I throttle my bandwidth to 90 Mbps, I still get packet
loss. However, if I switch the link from gigabit to 100Mbps, then I
don't get packet loss. So it's not the actual throughput that's the
problem. I get CRC errors in gigabit mode no matter what, if the pause
frames are blocked by the PHY.
> Here is what could possibly go wrong right now:
>
> - your Ethernet MAC unconditionally enables flow control at the MAC
> level, but does not advertise support for that in MII_ADVERTISE until
> you set SUPPORTED_Pause and SUPPORTED_Asym_Pause in the PHY driver, fair
> enough
>
> - the link partner you are testing against does not keep up well with
> your transmit rates, but supports flow control, so Pause frames that it
> sends back at you to tell you to stop transmitting so quickly just get
> ignored, because not being advertised, you experience packet loss
The link partner is an e1000 NIC on an 8-core 3.6GHz PC . Is it
conceivable that I'm overloading it?
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH v2 net-next 0/5] Add bpf support to set sk_bound_dev_if
From: David Ahern @ 2016-10-31 17:16 UTC (permalink / raw)
To: David Miller; +Cc: netdev, daniel, ast, daniel, maheshb, tgraf
In-Reply-To: <20161031.130104.2206412054220058623.davem@davemloft.net>
On 10/31/16 11:01 AM, David Miller wrote:
> From: David Ahern <dsa@cumulusnetworks.com>
> Date: Wed, 26 Oct 2016 17:58:37 -0700
>
>> The recently added VRF support in Linux leverages the bind-to-device
>> API for programs to specify an L3 domain for a socket. While
>> SO_BINDTODEVICE has been around for ages, not every ipv4/ipv6 capable
>> program has support for it. Even for those programs that do support it,
>> the API requires processes to be started as root (CAP_NET_RAW) which
>> is not desirable from a general security perspective.
>>
>> This patch set leverages Daniel Mack's work to attach bpf programs to
>> a cgroup:
>>
>> https://www.mail-archive.com/netdev@vger.kernel.org/msg134028.html
>>
>> to provide a capability to set sk_bound_dev_if for all AF_INET{6}
>> sockets opened by a process in a cgroup when the sockets are allocated.
>>
>> This capability enables running any program in a VRF context and is key
>> to deploying Management VRF, a fundamental configuration for networking
>> gear, with any Linux OS installation.
>
> Ok, after some review I think I understand what's going on here.
>
> It would initially seem simpler to just support forced sk_bound_dev_if
> in cgroups. But I think I understand why you may have gone this way:
That's what the l3mdev cgroup patch does -- force the sk_bound_dev_if for sockets. Tejun pushed back on adding new controllers. The cgroup+bpf is another way to accomplish the end goal. The key is using the cgroup infra for parent-child inheritance of the policy, holder of the policy "data" to be applied, tracking what processes are in a group, what the group is for a specific process, and on. No need to reinvent that part.
>
> 1) The cgroup-bpf code always has the cgroup hierarchy propagation
> logic.
>
> 2) The may be use cases for doing things with other sock members.
>
> With respect to #2, do you know of any such planned use cases already?
One suggestion is the local port binding limitations that Mahesh and Anoop were looking into.
>
> Also, any reason why you don't allow the cgroup bpf sk filter to return
> an error code so that the sock creation could be cancelled if the eBPF
> program desires that? It could be useful, I suppose.
My first draft at this feature had that but I removed it for simplicity now. Can certainly add it back.
^ permalink raw reply
* Re: [PATCH v2 net-next 1/5] bpf: Refactor cgroups code in prep for new type
From: Daniel Mack @ 2016-10-31 17:16 UTC (permalink / raw)
To: David Ahern, David Miller; +Cc: netdev, ast, daniel, maheshb, tgraf
In-Reply-To: <44b37825-d001-c708-963f-cbdedb559710@cumulusnetworks.com>
On 10/31/2016 06:05 PM, David Ahern wrote:
> On 10/31/16 11:00 AM, Daniel Mack wrote:
>> On 10/31/2016 05:58 PM, David Miller wrote:
>>> From: David Ahern <dsa@cumulusnetworks.com> Date: Wed, 26 Oct
>>> 2016 17:58:38 -0700
>>>
>>>> diff --git a/include/uapi/linux/bpf.h
>>>> b/include/uapi/linux/bpf.h index 6b62ee9a2f78..73da296c2125
>>>> 100644 --- a/include/uapi/linux/bpf.h +++
>>>> b/include/uapi/linux/bpf.h @@ -98,7 +98,7 @@ enum bpf_prog_type
>>>> { BPF_PROG_TYPE_TRACEPOINT, BPF_PROG_TYPE_XDP,
>>>> BPF_PROG_TYPE_PERF_EVENT, - BPF_PROG_TYPE_CGROUP_SKB, +
>>>> BPF_PROG_TYPE_CGROUP, };
>>>>
>>>> enum bpf_attach_type {
>>>
>>> If we do this then the cgroup-bpf series should use this value
>>> rather than changing it after-the-fact in your series here.
>>>
>>
>> Yeah, I'm confused too. I changed that name in my v7 from
>> BPF_PROG_TYPE_CGROUP_SOCK to BPF_PROG_TYPE_CGROUP_SKB on David's
>> (Ahern) request. Why is it now renamed again?
>
> Thomas pushed back on adding another program type in favor of using
> subtypes. So this makes the program type generic to CGROUP and patch
> 2 in this v2 set added Mickaël's subtype patch with the socket
> mangling done that way in patch 3.
>
Fine for me. I can change it around again.
Thanks,
Daniel
^ permalink raw reply
* [PATCH/RFC] ravb: Support 1Gbps on R-Car H3 ES1.1+ and R-Car M3-W
From: Geert Uytterhoeven @ 2016-10-31 17:13 UTC (permalink / raw)
To: David S. Miller, Sergei Shtylyov
Cc: Simon Horman, Arnd Bergmann, netdev, linux-renesas-soc,
Geert Uytterhoeven
The limitation to 10/100Mbit speeds on R-Car Gen3 is valid for R-Car H3
ES1.0 only. Check for the exact SoC model to allow 1Gbps on newer
revisions of R-Car H3, and on R-Car M3-W.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Tested on:
- r8a7795/salvator-x with R-Car H3 ES1.0 (limited to 100Mbps),
- r8a7795/salvator-x with R-Car H3 ES1.1 (1Gbps),
- r8a7796/salvator-x with R-Car M3-W ES1.0 (1Gbps).
This is marked as an RFC because it depends on:
A) the soc_device_match() infrastructure,
B) Renesas SoC core ESx.y handling.
Hence I think the best merge strategy is to let this patch go in through
Simon's Renesas tree.
David: If you agree, can you please provide your ack? Thanks!
---
drivers/net/ethernet/renesas/ravb_main.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 27cfec3154c8e0f2..bf3109a413ac5721 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -31,6 +31,7 @@
#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
+#include <linux/sys_soc.h>
#include <asm/div64.h>
@@ -977,6 +978,11 @@ static void ravb_adjust_link(struct net_device *ndev)
phy_print_status(phydev);
}
+static const struct soc_device_attribute r8a7795es10[] = {
+ { .soc_id = "r8a7795", .revision = "ES1.0", },
+ { /* sentinel */ }
+};
+
/* PHY init function */
static int ravb_phy_init(struct net_device *ndev)
{
@@ -1011,10 +1017,10 @@ static int ravb_phy_init(struct net_device *ndev)
return -ENOENT;
}
- /* This driver only support 10/100Mbit speeds on Gen3
+ /* This driver only support 10/100Mbit speeds on R-Car H3 ES1.0
* at this time.
*/
- if (priv->chip_id == RCAR_GEN3) {
+ if (soc_device_match(r8a7795es10)) {
int err;
err = phy_set_max_speed(phydev, SPEED_100);
--
1.9.1
^ permalink raw reply related
* Re: Let's do P4
From: Jiri Pirko @ 2016-10-31 17:12 UTC (permalink / raw)
To: John Fastabend
Cc: Alexei Starovoitov, Thomas Graf, Jakub Kicinski, netdev, davem,
jhs, roopa, simon.horman, ast, daniel, prem, hannes, jbenc, tom,
mattyk, idosch, eladr, yotamg, nogahf, ogerlitz, linville, andy,
f.fainelli, dsa, vivien.didelot, andrew, ivecera,
Maciej Żenczykowski
In-Reply-To: <58177712.4000208@gmail.com>
Mon, Oct 31, 2016 at 05:53:38PM CET, john.fastabend@gmail.com wrote:
>On 16-10-31 02:39 AM, Jiri Pirko wrote:
>> Sun, Oct 30, 2016 at 11:39:05PM CET, alexei.starovoitov@gmail.com wrote:
>>> On Sun, Oct 30, 2016 at 05:38:36PM +0100, Jiri Pirko wrote:
>>>> Sun, Oct 30, 2016 at 11:26:49AM CET, tgraf@suug.ch wrote:
>>>>> On 10/30/16 at 08:44am, Jiri Pirko wrote:
>>>>>> Sat, Oct 29, 2016 at 06:46:21PM CEST, john.fastabend@gmail.com wrote:
>>>>>>> On 16-10-29 07:49 AM, Jakub Kicinski wrote:
>>>>>>>> On Sat, 29 Oct 2016 09:53:28 +0200, Jiri Pirko wrote:
>>>>>>>>> Hi all.
>>>>>>>>>
>>>
>>> sorry for delay. travelling to KS, so probably missed something in
>>> this thread and comments can be totally off...
>>>
>>> the subject "let's do P4" is imo misleading, since it reads like
>>> we don't do P4 at the moment, whereas the opposite is true.
>>> Several p4->bpf compilers is a proof.
>>
>> We don't do p4 in kernel now, we don't do p4 offloading now. That is
>> the reason I started this discussion.
>>
>
>The point here is P4 is a high level language likely we will never "do"
>P4 in the kernel nor offload it. P4 translates to eBPF and runs in
>kernel just fine. This can be offloaded to some devices but as you
>point out is challenging for a class of architecture to offload.
>
>Also simple P4 programs can be offloaded into 'tc' cls_u32 for example
>and even cls_flower.
>
>>
>>>
>>>> The network world is divided into 2 general types of hw:
>>>> 1) network ASICs - network specific silicon, containing things like TCAM
>>>> These ASICs are suitable to be programmed by P4.
>>>
>>> i think the opposite is the case in case of P4.
>>> when hw asic has tcam it's still far far away from being usable with P4
>>> which requires fully programmable protocol parser, arbitrary tables and so on.
>>> P4 doesn't even define TCAM as a table type. The p4 program can declare
>>> a desired algorithm of search in the table and compiler has to figure out
>>> what HW resources to use to satisfy such p4 program.
>>>
>>>> 2) network processors - basically a general purpose CPUs
>>>> These processors are suitable to be programmed by eBPF.
>>>
>>> I think this statement is also misleading, since it positions
>>> p4 and bpf as competitors whereas that's not the case.
>>> p4 is the language. bpf is an instruction set.
>>
>> I wanted to say that we are having 2 approaches in silicon, 2 different
>> paradigms. Sure you can do p4>bpf. But hard to do it the opposite way.
>>
>>
>>>
>>>> Exactly. Following drawing shows p4 pipeline setup for SW and Hw:
>>>>
>>>> |
>>>> | +--> ebpf engine
>>>> | |
>>>> | |
>>>> | compilerB
>>>> | ^
>>>> | |
>>>> p4src --> compilerA --> p4ast --TCNL--> cls_p4 --+-> driver -> compilerC -> HW
>>>> |
>>>> userspace | kernel
>>>> |
>>>
>>> frankly this diagram smells very much like kernel bypass to me,
>>
>> what? That is well defined kernel API, in-kernel sw consumer and offload
>> in driver. Same API for both.
>>
>> Alex, you have very odd sense about what's bypassing kernel. That kind
>> of freaks me out...
>>
>
>I think the issue with offloading a P4-AST will be how much work goes
>into mapping this onto any particular hardware instance. And how much
>of the P4 language feature set is exposed.
>
>For example I suspect MLX switch has a different pipeline than MLX NIC
>and even different variations of the product lines. The same goes for
>Intel pipeline in NIC and switch and different products in same line.
>
>If P4-ast describes the exact instance of the hardware its an easy task
>the map is 1:1 but isn't exactly portable. Taking an N table onto a M
>table pipeline on the other hand is a bit more work and requires various
>transformations to occur in the runtime API. I'm guessing the class of
>devices we are talking about here can not reconfigure themselves to
>match the P4-ast.
I believe we can assume that. the p4ast has to be generic as the
original p4source is. It would be a terrible mistake to couple it with
some specific hardware. I only want to use p4ast because it would be easy
parse in kernel, unlike p4source.
>
>In the naive implementation only pipelines that map 1:1 will work. Maybe
>this is what Alexei is noticing?
P4 is ment to program programable hw, not fixed pipeline.
>
>>
>>> since I cannot see how one can put the whole p4 language compiler
>>> into the driver, so this last step of p4ast->hw, I presume, will be
>>> done by firmware, which will be running full compiler in an embedded cpu
>>
>> In case of mlxsw, that compiler would be in driver.
>>
>>
>>> on the switch. To me that's precisely the kernel bypass, since we won't
>>> have a clue what HW capabilities actually are and won't be able to fine
>>> grain control them.
>>> Please correct me if I'm wrong.
>>
>> You are wrong. By your definition, everything has to be figured out in
>> driver and FW does nothing. Otherwise it could do "something else" and
>> that would be a bypass? Does not make any sense to me whatsoever.
>>
>>
>>>
>>>> Plus the thing I cannot imagine in the model you propose is table fillup.
>>>> For ebpf, you use maps. For p4 you would have to have a separate HW-only
>>>> API. This is very similar to the original John's Flow-API. And therefore
>>>> a kernel bypass.
>>>
>>> I think John's flow api is a better way to expose mellanox switch capabilities.
>>
>> We are under impression that p4 suits us nicely. But it is not about
>> us, it is about finding the common way to do this.
>>
>
>I'll just poke at my FlowAPI question again. For fixed ASICS what is
>the Flow-API missing. We have a few proof points that show it is both
>sufficient and usable for the handful of use cases we care about.
Yeah, it is most probably fine. Even for flex ASICs to some point. The
question is how it stands comparing to other alternatives, like p4
>
>>
>>> I also think it's not fair to call it 'bypass'. I see nothing in it
>>> that justify such 'swear word' ;)
>>
>> John's Flow-API was a kernel bypass. Why? It was a API specifically
>> designed to directly work with HW tables, without kernel being involved.
>
>I don't think that is a fair definition of HW bypass. The SKIP_SW flag
>does exactly that for 'tc' based offloads and it was not rejected.
No, no, no. You still have possibility to do the same thing in kernel,
same functionality, with the same API. That is a big difference.
>
>The _real_ reason that seems to have fallen out of this and other
>discussion is the Flow-API didn't provide an in-kernel translation into
>an emulated patch. Note we always had a usermode translation to eBPF.
>A secondary reason appears to be overhead of adding yet another netlink
>family.
Yeah. Maybe you remember, back then when Flow-API was being discussed,
I suggested to wrap it under TC as cls_xflows and cls_xflowsaction of
some sort and do in-kernel datapath implementation. I believe that after
that, it would be acceptable.
>
>>
>>
>>> The goal of flow api was to expose HW features to user space, so that
>>> user space can program it. For something simple as mellanox switch
>>> asic it fits perfectly well.
>>
>> Again, this is not mlx-asic-specific. And again, that is a kernel bypass.
>>
>>
>>> Unless I misunderstand the bigger goal of this discussion and it's
>>> about programming ezchip devices.
>>
>> No. For network processors, I believe that BPF is nicely offloadable, no
>> need to do the excercise for that.
>>
>>
>>>
>>> If the goal is to model hw tcam in the linux kernel then just introduce
>>> tcam bpf map type. It will be dog slow in user space, but it will
>>> match exactly what is happnening in the HW and user space can make
>>> sensible trade-offs.
>>
>> No, you got me completely wrong. This is not about the TCAM. This is
>> about differences in the 2 words (p4/bpf).
>> Again, for "p4-ish" devices, you have to translate BPF. And as you
>> noted, it's an instruction set. Very hard if not impossible to parse in
>> order to get back the original semantics.
>>
>
>I think in this discussion "p4-ish" devices means devices with multiple
>tables in a pipeline? Not devices that have programmable/configurable
>pipelines right? And if we get to talking about reconfigurable devices
>I believe this should be done out of band as it typically means
>reloading some ucode, etc.
I'm talking about both. But I think we should focus on reconfigurable
ones, as we probably won't see that much fixed ones in the future.
^ 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