Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On 04/23/2015 04:46 AM, Alexei Starovoitov wrote: ... The other two threads degenerated into non-technical comments. Yep. :-/ Anyway, this set was RFC to answer my main question whether I should continue with tc cleanup or stop right here. I got my answer. I think it's worth proceeding with this, it's long overdue. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch added to the 3.12 stable tree] net: ethernet: pcnet32: Setup the SRAM and NOUFLO on Am79C97{3, 5}
From: Markos Chandras This patch has been added to the 3.12 stable tree. If you have any objections, please let us know. === commit 87f966d97b89774162df04d2106c6350c8fe4cb3 upstream. On a MIPS Malta board, tons of fifo underflow errors have been observed when using u-boot as bootloader instead of YAMON. The reason for that is that YAMON used to set the pcnet device to SRAM mode but u-boot does not. As a result, the default Tx threshold (64 bytes) is now too small to keep the fifo relatively used and it can result to Tx fifo underflow errors. As a result of which, it's best to setup the SRAM on supported controllers so we can always use the NOUFLO bit. Cc: Cc: Cc: Don Fry Signed-off-by: Markos Chandras Signed-off-by: David S. Miller Signed-off-by: Jiri Slaby --- drivers/net/ethernet/amd/pcnet32.c | 31 +-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/amd/pcnet32.c b/drivers/net/ethernet/amd/pcnet32.c index 2d8e28819779..048743573230 100644 --- a/drivers/net/ethernet/amd/pcnet32.c +++ b/drivers/net/ethernet/amd/pcnet32.c @@ -1516,7 +1516,7 @@ pcnet32_probe1(unsigned long ioaddr, int shared, struct pci_dev *pdev) { struct pcnet32_private *lp; int i, media; - int fdx, mii, fset, dxsuflo; + int fdx, mii, fset, dxsuflo, sram; int chip_version; char *chipname; struct net_device *dev; @@ -1553,7 +1553,7 @@ pcnet32_probe1(unsigned long ioaddr, int shared, struct pci_dev *pdev) } /* initialize variables */ - fdx = mii = fset = dxsuflo = 0; + fdx = mii = fset = dxsuflo = sram = 0; chip_version = (chip_version >> 12) & 0x; switch (chip_version) { @@ -1586,6 +1586,7 @@ pcnet32_probe1(unsigned long ioaddr, int shared, struct pci_dev *pdev) chipname = "PCnet/FAST III 79C973"; /* PCI */ fdx = 1; mii = 1; + sram = 1; break; case 0x2626: chipname = "PCnet/Home 79C978"; /* PCI */ @@ -1609,6 +1610,7 @@ pcnet32_probe1(unsigned long ioaddr, int shared, struct pci_dev *pdev) chipname = "PCnet/FAST III 79C975"; /* PCI */ fdx = 1; mii = 1; + sram = 1; break; case 0x2628: chipname = "PCnet/PRO 79C976"; @@ -1637,6 +1639,31 @@ pcnet32_probe1(unsigned long ioaddr, int shared, struct pci_dev *pdev) dxsuflo = 1; } + /* +* The Am79C973/Am79C975 controllers come with 12K of SRAM +* which we can use for the Tx/Rx buffers but most importantly, +* the use of SRAM allow us to use the BCR18:NOUFLO bit to avoid +* Tx fifo underflows. +*/ + if (sram) { + /* +* The SRAM is being configured in two steps. First we +* set the SRAM size in the BCR25:SRAM_SIZE bits. According +* to the datasheet, each bit corresponds to a 512-byte +* page so we can have at most 24 pages. The SRAM_SIZE +* holds the value of the upper 8 bits of the 16-bit SRAM size. +* The low 8-bits start at 0x00 and end at 0xff. So the +* address range is from 0x up to 0x17ff. Therefore, +* the SRAM_SIZE is set to 0x17. The next step is to set +* the BCR26:SRAM_BND midway through so the Tx and Rx +* buffers can share the SRAM equally. +*/ + a->write_bcr(ioaddr, 25, 0x17); + a->write_bcr(ioaddr, 26, 0xc); + /* And finally enable the NOUFLO bit */ + a->write_bcr(ioaddr, 18, a->read_bcr(ioaddr, 18) | (1 << 11)); + } + dev = alloc_etherdev(sizeof(*lp)); if (!dev) { ret = -ENOMEM; -- 2.3.5 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net-next] be2net: log link status
> -Original Message- > From: Ivan Vecera [mailto:ivec...@redhat.com] > > The driver unlike other drivers does not log link state changes. > > Cc: Sathya Perla > Cc: Subbu Seetharaman > Cc: Ajit Khaparde > Signed-off-by: Ivan Vecera > --- > drivers/net/ethernet/emulex/benet/be_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/emulex/benet/be_main.c > b/drivers/net/ethernet/emulex/benet/be_main.c > index fb0bc3c..e349131 100644 > --- a/drivers/net/ethernet/emulex/benet/be_main.c > +++ b/drivers/net/ethernet/emulex/benet/be_main.c > @@ -662,6 +662,8 @@ void be_link_status_update(struct be_adapter *adapter, u8 > link_status) > netif_carrier_on(netdev); > else > netif_carrier_off(netdev); > + > + netdev_info(netdev, "Link is %s\n", link_status ? "Up" : "Down"); > } > Acked-by: Sathya Perla -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/3] tc: fix return values of ingress qdisc
On 04/22/15 at 04:29pm, Cong Wang wrote: > On Wed, Apr 22, 2015 at 3:04 PM, Alexei Starovoitov wrote: > > On 4/21/15 9:59 PM, Cong Wang wrote: > >> > >> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov > >> wrote: > >>> > >>> ingress qdisc should return NET_XMIT_* values just like all other qdiscs. > >>> > >> > >> XMIT already means egress... > > > > > > may be then it should be renamed as well. > > from include/linux/netdevice.h: > > /* qdisc ->enqueue() return codes. */ > > #define NET_XMIT_SUCCESS0x00 > > ... > > > > the point is that qdisc->enqeue() must return NET_XMIT_* values. > > ingress qdisc is violating this and therefore should be fixed. > > XMIT is non-sense for ingress, you really need to pick another > name for it if TC_ACT_OK isn't okay for you (it is okay for me). You transmit into a qdisc. If that terminology doesn't suit you then rename it to NET_QUEUE_* but moving away from returning TC_ACT_* is definitely the right thing to do here. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 3/3] tc: cleanup tc_classify
On 04/22/15 at 04:38pm, Cong Wang wrote: > On Wed, Apr 22, 2015 at 3:27 PM, Alexei Starovoitov wrote: > > On 4/21/15 10:05 PM, Cong Wang wrote: > >> > >> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov > >> wrote: > >>> > >>> introduce tc_classify_act() and qdisc_drop_bypass() helper functions to > >>> reduce > >>> copy-paste among different qdiscs I like this cleanup. It aligns all skb dropping in qdiscs to a qdisc_drop*() function. > >> I don't think qdisc_drop_bypass() is more readable than without it, > >> maybe you need a better name, or just leave the code as it is. > > > > > > what would be a better name? I'm open to suggestions. > > My reading for "qdisc_drop_bypass()" is it bypasses packet > dropping for some case, apparently doesn't match its definition. > > I can't think out a better name therefore I don't think it deserves > a function, just leave as it is. Interesting logic ;-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
On 4/23/2015 6:26 AM, Herbert Xu wrote: > Hi: > > It looks like our IPsec implementations of CCM and GCM are buggy This applies also to GMAC (rfc4543), right? > in that they don't include the IV in the authentication calculation. > > This definitely breaks interoperability with anyone who implements > them correctly. The fact that there have been no reports on this > probably means that nobody has run into this in the field yet. Does this mean that even the test vectors (crypto/testmgr.h) are broken? Thanks, Horia -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
On Thu, Apr 23, 2015 at 12:03:38PM +0300, Horia Geantă wrote: > This applies also to GMAC (rfc4543), right? No RFC4543 appears to be correctly implemented. > Does this mean that even the test vectors (crypto/testmgr.h) are broken? Indeed. The test vectors appear to be generated either through our implementation or by one that is identical to us. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Resource usages in Linux drivers
Thank you! On 04/23/2015 05:19 PM, Sergei Shtylyov wrote: Hello. On 4/23/2015 9:45 AM, Jia-Ju Bai wrote: Dear Sir, I am very sorry to trouble you. I find that resource management is error-prone when writing Linux drivers, and many problems may occur, such as resource leaks. Meanwhile, I find that many applied patches in the kernel mailing list focus on releasing allocated resources, especially in error-handling paths. Therefore, I have a question: is it possible to automatically release allocated resources in drivers before unloading and in error-handling paths? Yes, there's managed device API, look for functions starting with devm_. There's one limitation though: it can be used only in the driver's probe() method, so can't be used when e.g. network device is being opened. I think many APIs, such as kmalloc, can also be managed like garbage collection in Java. Maybe the performance is a matter. I am looking forward to your reply, thanks! Such questions should actually be asked on the mailing lists, not personally. I am sorry for that, and I will cc to the mailing lists and other maintainers. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
On Thu, Apr 23, 2015 at 11:58:52AM +0200, Martin Willi wrote: > > I'm not sure about that. RFC4106 refers to [1] for test vectors, which > is still available at web.archive.org [2]. > > When looking for example at Test Case 3, this is the same as in a newer > revision of the document [3]. That looks exactly the same as > aes_gcm_enc_tv_template[2] from testmgr.h. These are GCM test vectors, not RFC4106 test vectors so they are of no use when you're testing whether the IPsec IV (which is not the same thing as the GCM IV) is included in the authentication or not. AFAIK GCM itself is implemented correctly. It's only the IPsec wrapper around it (rfc4106 in particular) that's broken. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] netdev_alloc_pcpu_stats: use less common iterator variable
From: Johannes Berg With the CPU iteration variable called 'i', it's relatively easy to have variable shadowing which sparse will warn about. Avoid that by renaming the variable to __cpu which is less likely to be used in the surrounding context. Signed-off-by: Johannes Berg --- include/linux/netdevice.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index bcbde799ec69..6719041d8851 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2021,10 +2021,10 @@ struct pcpu_sw_netstats { ({ \ typeof(type) __percpu *pcpu_stats = alloc_percpu(type); \ if (pcpu_stats) { \ - int i; \ - for_each_possible_cpu(i) { \ + int __cpu; \ + for_each_possible_cpu(__cpu) { \ typeof(type) *stat; \ - stat = per_cpu_ptr(pcpu_stats, i); \ + stat = per_cpu_ptr(pcpu_stats, __cpu); \ u64_stats_init(&stat->syncp); \ } \ } \ -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
Hi Herbert, > > Does this mean that even the test vectors (crypto/testmgr.h) are broken? > > Indeed. The test vectors appear to be generated either through > our implementation or by one that is identical to us. I'm not sure about that. RFC4106 refers to [1] for test vectors, which is still available at web.archive.org [2]. When looking for example at Test Case 3, this is the same as in a newer revision of the document [3]. That looks exactly the same as aes_gcm_enc_tv_template[2] from testmgr.h. We by the way use test vectors in userland from the same document to verify our own GCM backend, our OpenSSL backend and an AESNI/PCLMULQD backend. And I've never heard of any incompatibilities. Regards Martin [1]http://csrc.nist.gov/CryptoToolkit/modes/proposedmodes/gcm/gcm-spec.pdf [2]http://web.archive.org/web/20070712195408/http://csrc.nist.gov/CryptoToolkit/modes/proposedmodes/gcm/gcm-spec.pdf [3]http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-revised-spec.pdf -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Resource usages in Linux drivers
On 04/23/2015 05:29 PM, Jia-Ju Bai wrote: Thank you! On 04/23/2015 05:19 PM, Sergei Shtylyov wrote: Hello. On 4/23/2015 9:45 AM, Jia-Ju Bai wrote: Dear Sir, I am very sorry to trouble you. I find that resource management is error-prone when writing Linux drivers, and many problems may occur, such as resource leaks. Meanwhile, I find that many applied patches in the kernel mailing list focus on releasing allocated resources, especially in error-handling paths. Therefore, I have a question: is it possible to automatically release allocated resources in drivers before unloading and in error-handling paths? Yes, there's managed device API, look for functions starting with devm_. There's one limitation though: it can be used only in the driver's probe() method, so can't be used when e.g. network device is being opened. I think many APIs, such as kmalloc, can also be managed like garbage collection in Java. Maybe the performance is a matter. I am looking forward to your reply, thanks! Such questions should actually be asked on the mailing lists, not personally. I am sorry for that, and I will cc to the mailing lists and other maintainers. I find that some common APIs are not managed, such as napi_enable and napi_start_queue. Is it possible to provide managed APIs for them? I also find many drivers do not use these managed APIs, especially in ethernet card drivers (like e100, r8169). Is it possible to change them? Best wishes, Jia-Ju Bai -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Resource usages in Linux drivers
On 4/23/2015 1:08 PM, Jia-Ju Bai wrote: I am very sorry to trouble you. I find that resource management is error-prone when writing Linux drivers, and many problems may occur, such as resource leaks. Meanwhile, I find that many applied patches in the kernel mailing list focus on releasing allocated resources, especially in error-handling paths. Therefore, I have a question: is it possible to automatically release allocated resources in drivers before unloading and in error-handling paths? Yes, there's managed device API, look for functions starting with devm_. There's one limitation though: it can be used only in the driver's probe() method, so can't be used when e.g. network device is being opened. I think many APIs, such as kmalloc, can also be managed like garbage collection in Java. Maybe the performance is a matter. I am looking forward to your reply, thanks! Such questions should actually be asked on the mailing lists, not personally. I am sorry for that, and I will cc to the mailing lists and other maintainers. I find that some common APIs are not managed, such as napi_enable and napi_start_queue. Is it possible to provide managed APIs for them? No, they're only called from ndo_open() method IIRC. The device managed APIs can only be called at the device probing time. I also find many drivers do not use these managed APIs, especially in ethernet card drivers (like e100, r8169). Is it possible to change them? Patches welcome. :-) Best wishes, Jia-Ju Bai WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
On Thu, Apr 23, 2015 at 11:26:20AM +0800, Herbert Xu wrote: > Hi: > > It looks like our IPsec implementations of CCM and GCM are buggy > in that they don't include the IV in the authentication calculation. Seems like crypto_rfc4106_crypt() passes the associated data it got from ESP directly to gcm, without chaining with the IV. > > This definitely breaks interoperability with anyone who implements > them correctly. The fact that there have been no reports on this > probably means that nobody has run into this in the field yet. > > On the security side, this is probably not a big deal for CCM > because it always verifies the authentication tag after decryption. > But for GCM this may be a DoS issue as an attacker could modify > the IV without triggering the authentication check and thus cause > an unnecessary decryption. For both CCM and GCM this will result > in random data injected as a packet into the network stack which > hopefully will be dropped. > > In order to fix this without breaking backwards compatibility, > my plan is to introduce new templates such as rfc4106v2 which > implement the RFC correctly. The existing templates will be > retained so that current users aren't broken by the fix. Adding a second template for the correct implementation is probaply the only thing that we can do if we don't want to break backwards compatibility. But maybe we can add a warning to the old implementation, such that users notice that they use a broken version. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
Hi Steffen, > > It looks like our IPsec implementations of CCM and GCM are buggy > > in that they don't include the IV in the authentication calculation. > > Seems like crypto_rfc4106_crypt() passes the associated data it > got from ESP directly to gcm, without chaining with the IV. Do you have any pointer for me where this is defined? Why is it needed, given that GCM implicitly authenticates the IV by using it in Y0? Also, I've just verified that a tunnel between the Windows Filtering Platform and Linux 3.13 using AES128GCM16 works just fine. So if we do something wrong, the problem does not only affect Linux. Regards Martin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net: phy: micrel: don't do clock-mode-select if we got NULL clock
Signed-off-by: Niklas Cassel --- drivers/net/phy/micrel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 1190fd8..a422036 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -548,7 +548,7 @@ static int kszphy_probe(struct phy_device *phydev) } clk = devm_clk_get(&phydev->dev, "rmii-ref"); - if (!IS_ERR(clk)) { + if (!IS_ERR_OR_NULL(clk)) { unsigned long rate = clk_get_rate(clk); bool rmii_ref_clk_sel_25_mhz; -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net 0/3] tipc: three bug fixes
A set of unrelated corrections; one for the tipc netns implementation, one regarding problems with random link resets, and one removing a an erroneous refcount decrement when reading link statistsics via netlink. Erik Hugne (2): tipc: fix random link reset problem tipc: fix node refcount issue Ying Xue (1): tipc: fix topology server broken issue net/tipc/link.c | 1 - net/tipc/server.c | 9 +++-- net/tipc/socket.c | 3 ++- 3 files changed, 5 insertions(+), 8 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net 3/3] tipc: fix node refcount issue
From: Erik Hugne When link statistics is dumped over netlink, we iterate over the list of peer nodes and append each links statistics to the netlink msg. In the case where the dump is resumed after filling up a nlmsg, the node refcnt is decremented without having been incremented previously which may cause the node reference to be freed. When this happens, the following info/stacktrace will be generated, followed by a crash or undefined behavior. We fix this by removing the erroneous call to tipc_node_put inside the loop that iterates over nodes. [ 384.312303] INFO: trying to register non-static key. [ 384.313110] the code is fine but needs lockdep annotation. [ 384.313290] turning off the locking correctness validator. [ 384.313290] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.0.0+ #13 [ 384.313290] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 384.313290] 88003c6d0290 88003cc03ca8 8170adf1 0007 [ 384.313290] 82728730 88003cc03d38 810a6a6d 001d7200 [ 384.313290] 88003c6d0ab0 88003cc03ce8 0285 0001 [ 384.313290] Call Trace: [ 384.313290][] dump_stack+0x4c/0x65 [ 384.313290] [] __lock_acquire+0xf3d/0xf50 [ 384.313290] [] lock_acquire+0xd5/0x290 [ 384.313290] [] ? link_timeout+0x1c/0x170 [tipc] [ 384.313290] [] ? link_state_event+0x4e0/0x4e0 [tipc] [ 384.313290] [] _raw_spin_lock_bh+0x40/0x80 [ 384.313290] [] ? link_timeout+0x1c/0x170 [tipc] [ 384.313290] [] link_timeout+0x1c/0x170 [tipc] [ 384.313290] [] call_timer_fn+0xb8/0x490 [ 384.313290] [] ? process_timeout+0x10/0x10 [ 384.313290] [] run_timer_softirq+0x21c/0x420 [ 384.313290] [] ? link_state_event+0x4e0/0x4e0 [tipc] [ 384.313290] [] __do_softirq+0xf4/0x630 [ 384.313290] [] irq_exit+0x5d/0x60 [ 384.313290] [] smp_apic_timer_interrupt+0x41/0x50 [ 384.313290] [] apic_timer_interrupt+0x70/0x80 [ 384.313290][] ? default_idle+0x20/0x210 [ 384.313290] [] ? default_idle+0x1e/0x210 [ 384.313290] [] arch_cpu_idle+0xa/0x10 [ 384.313290] [] cpu_startup_entry+0x2c3/0x530 [ 384.313290] [] ? clockevents_register_device+0x113/0x200 [ 384.313290] [] start_secondary+0x13f/0x170 Fixes: 8a0f6ebe8494 ("tipc: involve reference counter for node structure") Signed-off-by: Erik Hugne Signed-off-by: Jon Maloy --- net/tipc/link.c | 1 - 1 file changed, 1 deletion(-) diff --git a/net/tipc/link.c b/net/tipc/link.c index a6b30df..57be6e6 100644 --- a/net/tipc/link.c +++ b/net/tipc/link.c @@ -2143,7 +2143,6 @@ int tipc_nl_link_dump(struct sk_buff *skb, struct netlink_callback *cb) err = __tipc_nl_add_node_links(net, &msg, node, &prev_link); tipc_node_unlock(node); - tipc_node_put(node); if (err) goto out; -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net 1/3] tipc: fix topology server broken issue
From: Ying Xue When a new topology server is launched in a new namespace, its listening socket is inserted into the "init ns" namespace's socket hash table rather than the one owned by the new namespace. Although the socket's namespace is forcedly changed to the new namespace later, the socket is still stored in the socket hash table of "init ns" namespace. When a client created in the new namespace connects its own topology server, the connection is failed as its server's socket could not be found from its own namespace's socket table. If __sock_create() instead of original sock_create_kern() is used to create the server's socket through specifying an expected namesapce, the socket will be inserted into the specified namespace's socket table, thereby avoiding to the topology server broken issue. Fixes: 76100a8a64bc ("tipc: fix netns refcnt leak") Reported-by: Erik Hugne Signed-off-by: Ying Xue Signed-off-by: Jon Maloy --- net/tipc/server.c | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/net/tipc/server.c b/net/tipc/server.c index ab6183c..77ff03e 100644 --- a/net/tipc/server.c +++ b/net/tipc/server.c @@ -102,7 +102,7 @@ static void tipc_conn_kref_release(struct kref *kref) } saddr->scope = -TIPC_NODE_SCOPE; kernel_bind(sock, (struct sockaddr *)saddr, sizeof(*saddr)); - sk_release_kernel(sk); + sock_release(sock); con->sock = NULL; } @@ -321,12 +321,9 @@ static struct socket *tipc_create_listen_sock(struct tipc_conn *con) struct socket *sock = NULL; int ret; - ret = sock_create_kern(AF_TIPC, SOCK_SEQPACKET, 0, &sock); + ret = __sock_create(s->net, AF_TIPC, SOCK_SEQPACKET, 0, &sock, 1); if (ret < 0) return NULL; - - sk_change_net(sock->sk, s->net); - ret = kernel_setsockopt(sock, SOL_TIPC, TIPC_IMPORTANCE, (char *)&s->imp, sizeof(s->imp)); if (ret < 0) @@ -376,7 +373,7 @@ static struct socket *tipc_create_listen_sock(struct tipc_conn *con) create_err: kernel_sock_shutdown(sock, SHUT_RDWR); - sk_release_kernel(sock->sk); + sock_release(sock); return NULL; } -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net 2/3] tipc: fix random link reset problem
From: Erik Hugne In the function tipc_sk_rcv(), the stack variable 'err' is only initialized to TIPC_ERR_NO_PORT for the first iteration over the link input queue. If a chain of messages are received from a link, failure to lookup the socket for any but the first message will cause the message to bounce back out on a random link. We fix this by properly initializing err. Signed-off-by: Erik Hugne Signed-off-by: Jon Maloy --- net/tipc/socket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/tipc/socket.c b/net/tipc/socket.c index ee90d74..9074b5c 100644 --- a/net/tipc/socket.c +++ b/net/tipc/socket.c @@ -1764,13 +1764,14 @@ static int tipc_sk_enqueue(struct sk_buff_head *inputq, struct sock *sk, int tipc_sk_rcv(struct net *net, struct sk_buff_head *inputq) { u32 dnode, dport = 0; - int err = -TIPC_ERR_NO_PORT; + int err; struct sk_buff *skb; struct tipc_sock *tsk; struct tipc_net *tn; struct sock *sk; while (skb_queue_len(inputq)) { + err = -TIPC_ERR_NO_PORT; skb = NULL; dport = tipc_skb_peek_port(inputq, dport); tsk = tipc_sk_lookup(net, dport); -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix tcp fin memory accounting
On 04/21/2015 07:09 PM, Eric Dumazet wrote: Note that this patch adds a deadlock possibility in some stress situations. If a process owning some tcp socket dies, and tcp_mem[2] is already hit, all sk_stream_alloc_skb() can return NULL and we loop in tcp_send_fin(), making no progress because we can not free any tcp memory. Ugh. Thanks for fixing this Eric! Josh -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix tcp fin memory accounting
On Thu, 2015-04-23 at 08:48 -0500, Josh Hunt wrote: > On 04/21/2015 07:09 PM, Eric Dumazet wrote: > > > > Note that this patch adds a deadlock possibility in some stress > > situations. > > > > If a process owning some tcp socket dies, and tcp_mem[2] is already hit, > > all sk_stream_alloc_skb() can return NULL and we loop in tcp_send_fin(), > > making no progress because we can not free any tcp memory. > > Ugh. Thanks for fixing this Eric! No problem ;) For the record, I've tested this followup patch that I'll formally submit when net-next reopens : If there is one already sent skb in write queue (we look at the tail of course), and : - We are under TCP memory pressure, - or allocation of a fresh skb using GFP_KERNEL failed, -> we add the FIN flag that will eventually be sent later after a timeout. net/ipv4/tcp_output.c | 41 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 2ade67b7cdb0..fe6558eb64f3 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -2827,33 +2827,34 @@ static void sk_forced_wmem_schedule(struct sock *sk, int size) sk_memory_allocated_add(sk, amt, &status); } -/* Send a fin. The caller locks the socket for us. This cannot be - * allowed to fail queueing a FIN frame under any circumstances. +/* Send a FIN. The caller locks the socket for us. + * We should try to send a FIN packet really hard, but eventually give up. */ void tcp_send_fin(struct sock *sk) { + struct sk_buff *skb, *tskb = tcp_write_queue_tail(sk); struct tcp_sock *tp = tcp_sk(sk); - struct sk_buff *skb = tcp_write_queue_tail(sk); - int mss_now; - /* Optimization, tack on the FIN if we have a queue of -* unsent frames. But be careful about outgoing SACKS -* and IP options. + /* Optimization, tack on the FIN if we have one skb in write queue and +* this skb was not yet sent, or we are under memory pressure. +* Note: in the latter case, FIN packet will be sent after a timeout, +* as TCP stack thinks it has been transmitted once. */ - mss_now = tcp_current_mss(sk); - - if (tcp_send_head(sk)) { - TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_FIN; - TCP_SKB_CB(skb)->end_seq++; + if (tskb && (tcp_send_head(sk) || sk_under_memory_pressure(sk))) { +coalesce: + TCP_SKB_CB(tskb)->tcp_flags |= TCPHDR_FIN; + TCP_SKB_CB(tskb)->end_seq++; tp->write_seq++; + if (!tcp_send_head(sk)) { + tp->snd_nxt++; + return; + } } else { - /* Socket is locked, keep trying until memory is available. */ - for (;;) { - skb = alloc_skb_fclone(MAX_TCP_HEADER, - sk->sk_allocation); - if (skb) - break; - yield(); + skb = alloc_skb_fclone(MAX_TCP_HEADER, sk->sk_allocation); + if (unlikely(!skb)) { + if (tskb) + goto coalesce; + return; } skb_reserve(skb, MAX_TCP_HEADER); sk_forced_wmem_schedule(sk, skb->truesize); @@ -2862,7 +2863,7 @@ void tcp_send_fin(struct sock *sk) TCPHDR_ACK | TCPHDR_FIN); tcp_queue_skb(sk, skb); } - __tcp_push_pending_frames(sk, mss_now, TCP_NAGLE_OFF); + __tcp_push_pending_frames(sk, tcp_current_mss(sk), TCP_NAGLE_OFF); } /* We get here when a process closes a file descriptor (either due to -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] rhashtable: don't attempt to grow when at max_size
From: Johannes Berg The conversion of mac80211's station table to rhashtable had a bug that I found by accident in code review, that hadn't been found as rhashtable apparently managed to have a maximum hash chain length of one (!) in all our testing. In order to test the bug and verify the fix I set my rhashtable's max_size very low (4) in order to force getting hash collisions. At that point, rhashtable WARNed in rhashtable_insert_rehash() but didn't actually reject the hash table insertion. This caused it to lose insertions - my master list of stations would have 9 entries, but the rhashtable only had 5. This may warrant a deeper look, but that WARN_ON() just shouldn't happen. Fix this by not returning true from rht_grow_above_100() when the rhashtable's max_size has been reached - in this case the user is explicitly configuring it to be at most that big, so even if it's now above 100% it shouldn't attempt to resize. This fixes the "lost insertion" issue and consequently allows my code to display its error (and verify my fix for it.) Signed-off-by: Johannes Berg --- include/linux/rhashtable.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h index e23d242d1230..dbcbcc59aa92 100644 --- a/include/linux/rhashtable.h +++ b/include/linux/rhashtable.h @@ -282,7 +282,8 @@ static inline bool rht_shrink_below_30(const struct rhashtable *ht, static inline bool rht_grow_above_100(const struct rhashtable *ht, const struct bucket_table *tbl) { - return atomic_read(&ht->nelems) > tbl->size; + return atomic_read(&ht->nelems) > tbl->size && + (!ht->p.max_size || tbl->size < ht->p.max_size); } /* The bucket lock is selected based on the hash and protects mutations -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC] openvswitch: add support for netpoll
This patch simply forwards unicast netpoll packets via one of physical interface in datapath depending on source mac address from the skb. It seems possible to use common net flow classification for netpoll but there is no way to guarantee presence of right flow in kernel cache. Signed-off-by: Konstantin Khlebnikov --- net/openvswitch/vport-internal_dev.c | 74 ++ net/openvswitch/vport-netdev.c | 63 - net/openvswitch/vport-netdev.h | 15 +++ 3 files changed, 148 insertions(+), 4 deletions(-) diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c index 6a55f7105505..d1eb09ac09e8 100644 --- a/net/openvswitch/vport-internal_dev.c +++ b/net/openvswitch/vport-internal_dev.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -66,11 +67,77 @@ static struct rtnl_link_stats64 *internal_dev_get_stats(struct net_device *netde return stats; } +#ifdef CONFIG_NET_POLL_CONTROLLER + +static void internal_dev_poll_controller(struct net_device *dev) +{ +} + +static struct netdev_vport *get_local_netdev_vport(struct net_device *dev) +{ + struct datapath *dp = internal_dev_priv(dev)->vport->dp; + + return netdev_vport_priv(ovs_lookup_vport(dp, OVSP_LOCAL)); +} + +static int internal_dev_netpoll_setup(struct net_device *internal_dev, + struct netpoll_info *info) +{ + struct netdev_vport *local = get_local_netdev_vport(internal_dev); + struct netdev_vport *lower; + struct list_head *iter; + int ret = -EOPNOTSUPP; + + ASSERT_RTNL(); + /* succeed if at least one lower device can handle netpoll */ + netdev_for_each_lower_private(local->dev, lower, iter) + if (!ovs_netdev_netpoll_enable(lower)) + ret = 0; + /* enable netpoll on local device as mark for new devices */ + if (!ret && local->dev != internal_dev) + ovs_netdev_netpoll_enable(local); + return ret; +} + +static void internal_dev_netpoll_cleanup(struct net_device *internal_dev) +{ + struct netdev_vport *local = get_local_netdev_vport(internal_dev); + struct netdev_vport *lower; + struct list_head *iter; + + /* FIXME needs reference counting for more than one netpoll in dp */ + + ASSERT_RTNL(); + netdev_for_each_lower_private(local->dev, lower, iter) + ovs_netdev_netpoll_disable(lower); + ovs_netdev_netpoll_disable(local); +} + +static void internal_dev_netpoll_xmit(struct sk_buff *skb, + struct net_device *internal_dev) +{ + struct netdev_vport *local = get_local_netdev_vport(internal_dev); + struct netdev_vport *lower; + struct list_head *iter; + + netdev_for_each_lower_private_rcu(local->dev, lower, iter) + if (!ovs_netdev_netpoll_send(lower, skb)) + return; /* Unicast only, first gets all. */ + dev_kfree_skb_irq(skb); +} + +#endif /* CONFIG_NET_POLL_CONTROLLER */ + /* Called with rcu_read_lock_bh. */ static int internal_dev_xmit(struct sk_buff *skb, struct net_device *netdev) { rcu_read_lock(); - ovs_vport_receive(internal_dev_priv(netdev)->vport, skb, NULL); +#ifdef CONFIG_NET_POLL_CONTROLLER + if (unlikely(netpoll_tx_running(netdev))) + internal_dev_netpoll_xmit(skb, netdev); + else +#endif + ovs_vport_receive(internal_dev_priv(netdev)->vport, skb, NULL); rcu_read_unlock(); return 0; } @@ -122,6 +189,11 @@ static const struct net_device_ops internal_dev_netdev_ops = { .ndo_set_mac_address = eth_mac_addr, .ndo_change_mtu = internal_dev_change_mtu, .ndo_get_stats64 = internal_dev_get_stats, +#ifdef CONFIG_NET_POLL_CONTROLLER + .ndo_poll_controller = internal_dev_poll_controller, + .ndo_netpoll_setup = internal_dev_netpoll_setup, + .ndo_netpoll_cleanup = internal_dev_netpoll_cleanup, +#endif }; static struct rtnl_link_ops internal_dev_link_ops __read_mostly = { diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c index 4776282c6417..324fb078d32a 100644 --- a/net/openvswitch/vport-netdev.c +++ b/net/openvswitch/vport-netdev.c @@ -26,6 +26,7 @@ #include #include #include +#include #include @@ -86,10 +87,59 @@ static struct net_device *get_dpdev(const struct datapath *dp) return netdev_vport_priv(local)->dev; } +#ifdef CONFIG_NET_POLL_CONTROLLER +int ovs_netdev_netpoll_enable(struct netdev_vport *netdev_vport) +{ + struct netpoll *np; + int err; + + if (netdev_vport->np) + return 0; + + np = kzalloc(sizeof(*np), GFP_KERNEL); + if (!np) + return -ENOMEM; + + err = __netpoll_setup(np, netdev_vport->dev); + if (err) + kfree(np); + else +
Re: CCM/GCM implementation defect
On 04/23/2015 07:45 AM, Steffen Klassert wrote: > On Thu, Apr 23, 2015 at 11:26:20AM +0800, Herbert Xu wrote: >> Hi: >> >> It looks like our IPsec implementations of CCM and GCM are buggy >> in that they don't include the IV in the authentication calculation. > > Seems like crypto_rfc4106_crypt() passes the associated data it > got from ESP directly to gcm, without chaining with the IV. > >> >> This definitely breaks interoperability with anyone who implements >> them correctly. The fact that there have been no reports on this >> probably means that nobody has run into this in the field yet. >> >> On the security side, this is probably not a big deal for CCM >> because it always verifies the authentication tag after decryption. >> But for GCM this may be a DoS issue as an attacker could modify >> the IV without triggering the authentication check and thus cause >> an unnecessary decryption. For both CCM and GCM this will result >> in random data injected as a packet into the network stack which >> hopefully will be dropped. >> >> In order to fix this without breaking backwards compatibility, >> my plan is to introduce new templates such as rfc4106v2 which >> implement the RFC correctly. The existing templates will be >> retained so that current users aren't broken by the fix. > > Adding a second template for the correct implementation is > probaply the only thing that we can do if we don't want to > break backwards compatibility. But maybe we can add a warning > to the old implementation, such that users notice that they > use a broken version. Unless we have a cryptographer indicate to us how that this mistake does not significantly reduce or break the confidentiality and authentication, I do not think we should keep a known broken implementation around. Lets say this brings GCM security down to ROT13, then it just needs to die without keeping interoperability. Additionally, looking at how long we suffered and still suffer from defaulting to something broken (sha2_256 truncation) I'm really not in favour of keeping broken crypto implementations around. Paul -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] netdev_alloc_pcpu_stats: use less common iterator variable
From: Johannes Berg Date: Thu, 23 Apr 2015 12:06:30 +0200 > From: Johannes Berg > > With the CPU iteration variable called 'i', it's relatively easy > to have variable shadowing which sparse will warn about. Avoid > that by renaming the variable to __cpu which is less likely to > be used in the surrounding context. > > Signed-off-by: Johannes Berg Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: phy: micrel: don't do clock-mode-select if we got NULL clock
From: Niklas Cassel Date: Thu, 23 Apr 2015 15:37:11 +0200 > Signed-off-by: Niklas Cassel > --- > drivers/net/phy/micrel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 1190fd8..a422036 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -548,7 +548,7 @@ static int kszphy_probe(struct phy_device *phydev) > } > > clk = devm_clk_get(&phydev->dev, "rmii-ref"); > - if (!IS_ERR(clk)) { > + if (!IS_ERR_OR_NULL(clk)) { > unsigned long rate = clk_get_rate(clk); > bool rmii_ref_clk_sel_25_mhz; > I do not see anyone in any other networking driver checking for a NULL return from devm_clk_get(). So either everyone else is wrong, or your change is. I want to find out which before applying anything. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] ibmveth: Fix off-by-one error in ibmveth_change_mtu()
From: David Gibson Date: Thu, 23 Apr 2015 14:43:05 +1000 > AFAIK the PAPR document which defines the virtual device interface used by > the ibmveth driver doesn't specify a specific maximum MTU. So, in the > ibmveth driver, the maximum allowed MTU is determined by the maximum > allocated buffer size of 64k (corresponding to one page in the common case) > minus the per-buffer overhead IBMVETH_BUFF_OH (which has value 22 for 14 > bytes of ethernet header, plus 8 bytes for an opaque handle). > > This suggests a maximum allowable MTU of 65514 bytes, but in fact the > driver only permits a maximum MTU of 65513. This is because there is a < > instead of an <= in ibmveth_change_mtu(), which only permits an MTU which > is strictly smaller than the buffer size, rather than allowing the buffer > to be completely filled. > > This patch fixes the buglet. > > Signed-off-by: David Gibson > Acked-by: Thomas Falcon Applied, thank you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net 0/3] tipc: three bug fixes
From: Jon Maloy Date: Thu, 23 Apr 2015 09:37:37 -0400 > A set of unrelated corrections; one for the tipc netns implementation, > one regarding problems with random link resets, and one removing a > an erroneous refcount decrement when reading link statistsics via > netlink. Series applied, thanks Jon. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix tcp fin memory accounting
From: Eric Dumazet Date: Thu, 23 Apr 2015 07:02:47 -0700 > + if (!tcp_send_head(sk)) { > + tp->snd_nxt++; > + return; > + } I'm not so sure about this. Why is this needed? Otherwise patch looks fine to me. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rhashtable: don't attempt to grow when at max_size
From: Johannes Berg Date: Thu, 23 Apr 2015 16:38:43 +0200 > From: Johannes Berg > > The conversion of mac80211's station table to rhashtable had a bug > that I found by accident in code review, that hadn't been found as > rhashtable apparently managed to have a maximum hash chain length > of one (!) in all our testing. > > In order to test the bug and verify the fix I set my rhashtable's > max_size very low (4) in order to force getting hash collisions. > > At that point, rhashtable WARNed in rhashtable_insert_rehash() but > didn't actually reject the hash table insertion. This caused it to > lose insertions - my master list of stations would have 9 entries, > but the rhashtable only had 5. This may warrant a deeper look, but > that WARN_ON() just shouldn't happen. > > Fix this by not returning true from rht_grow_above_100() when the > rhashtable's max_size has been reached - in this case the user is > explicitly configuring it to be at most that big, so even if it's > now above 100% it shouldn't attempt to resize. > > This fixes the "lost insertion" issue and consequently allows my > code to display its error (and verify my fix for it.) > > Signed-off-by: Johannes Berg It looks fine to me, but I'll let Herbert and Thomas review this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rhashtable: don't attempt to grow when at max_size
On Thu, 2015-04-23 at 11:59 -0400, David Miller wrote: > > This fixes the "lost insertion" issue and consequently allows my > > code to display its error (and verify my fix for it.) > It looks fine to me, but I'll let Herbert and Thomas review this. Oh, sorry, didn't know Herbert was also involved with this. Anyway, that's a good idea. I really went for "this looks obvious" and patched it, and it started working for me thereafter. I don't know anything about side effects here. johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rhashtable: don't attempt to grow when at max_size
On 04/23/2015 06:09 PM, Johannes Berg wrote: On Thu, 2015-04-23 at 11:59 -0400, David Miller wrote: This fixes the "lost insertion" issue and consequently allows my code to display its error (and verify my fix for it.) It looks fine to me, but I'll let Herbert and Thomas review this. Oh, sorry, didn't know Herbert was also involved with this. Would be good to add Fixes tags, commit in question seems to be: ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion") Anyway, that's a good idea. I really went for "this looks obvious" and patched it, and it started working for me thereafter. I don't know anything about side effects here. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix tcp fin memory accounting
On Thu, 2015-04-23 at 11:54 -0400, David Miller wrote: > From: Eric Dumazet > Date: Thu, 23 Apr 2015 07:02:47 -0700 > > > + if (!tcp_send_head(sk)) { > > + tp->snd_nxt++; > > + return; > > + } > > I'm not so sure about this. Why is this needed? > > Otherwise patch looks fine to me. > I guess I need to add a comment then ;) If we want to pretend FIN was sent, we also need to tweak tp->snd_nxt to match new tskb->end_seq (or tp->write_seq). I tested following packetdrill script and confirmed that if I do not tweak snd_nxt, last packet sent is incorrect : > . 5001:5001(0) ack 2 This might be because our stack relies that we never coalesce something on one already sent skb (we do this check in tcp_sendmsg() for example) Also note the funny thing : The FIN that is finally sent also has a Push flag. I believe its fine to leave this, as it is not incorrect at protocol perspective, and removing the push would add some logic when we split the packet to remove the ACKed part. // Initialize a server socket. 0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3 +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0 +0 bind(3, ..., ...) = 0 +0 listen(3, 1) = 0 +0< S 0:0(0) win 32792 +0> S. 0:0(0) ack 1 +.010 < . 1:1(0) ack 1 win 257 +0 accept(3, ..., ...) = 4 +0.010 write(4, ..., 5000) = 5000 +0> P. 1:5001(5000) ack 1 // this assumes kernel is not able to allocate fresh skb to // hold FIN +.010 shutdown(4, SHUT_RDWR) = 0 +0< . 1:1(0) ack 5001 win 257 // Yeah ! note the FIN that we 'retransmit' also gets a Push +0.209> FP. 5001:5001(0) ack 1 +0 read(4, ..., 1000) = 0 +0 write(4, ..., 1000) = -1 EPIPE (Broken pipe) +.010 close(4) = 0 // TLP at 2*RTT after the original send of FIN. +.201 > FP. 5001:5001(0) ack 1 // Receive an ACK for the remaining outstanding data. +.010 < . 1:1(0) ack 5002 win 257 // Receive a FIN. +.010 < F. 1:1(0) ack 5002 win 257 // Make sure final packet is correct +0> . 5002:5002(0) ack 2 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix tcp fin memory accounting
On Thu, 2015-04-23 at 09:16 -0700, Eric Dumazet wrote: > On Thu, 2015-04-23 at 11:54 -0400, David Miller wrote: > > From: Eric Dumazet > > Date: Thu, 23 Apr 2015 07:02:47 -0700 > > > > > + if (!tcp_send_head(sk)) { > > > + tp->snd_nxt++; > > > + return; > > > + } > > > > I'm not so sure about this. Why is this needed? > > > > Otherwise patch looks fine to me. > > > > I guess I need to add a comment then ;) > > If we want to pretend FIN was sent, we also need to tweak tp->snd_nxt to > match new tskb->end_seq (or tp->write_seq). > > I tested following packetdrill script and confirmed that if I do not > tweak snd_nxt, last packet sent is incorrect : > > > . 5001:5001(0) ack 2 > > This might be because our stack relies that we never coalesce something > on one already sent skb (we do this check in tcp_sendmsg() for example) Well, real reason is that tp->snd_nxt is not touched in retransmit paths, but when new data is sent (tcp_event_new_data_sent()) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Suspicious RCU usage in bridge with Linux v4.0-9362-g1fc149933fd4
Hi All, We've had a user report the following backtrace from the bridge module with a recent Linus' tree. Has anything like this been reported yet? If you have any questions on setup, the user is CC'd. josh [ 29.382235] br0: port 1(tap0) entered forwarding state [ 29.382286] === [ 29.382315] [ INFO: suspicious RCU usage. ] [ 29.382344] 4.1.0-0.rc0.git11.1.fc23.x86_64 #1 Not tainted [ 29.382380] --- [ 29.382409] net/bridge/br_private.h:626 suspicious rcu_dereference_check() usage! [ 29.382455] other info that might help us debug this: [ 29.382507] rcu_scheduler_active = 1, debug_locks = 0 [ 29.382549] 2 locks held by swapper/0/0: [ 29.382576] #0: (((&p->forward_delay_timer))){+.-...}, at: [] call_timer_fn+0x5/0x4f0 [ 29.382660] #1: (&(&br->lock)->rlock){+.-...}, at: [] br_forward_delay_timer_expired+0x31/0x140 [bridge] [ 29.382754] stack backtrace: [ 29.382787] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.0-0.rc0.git11.1.fc23.x86_64 #1 [ 29.382838] Hardware name: LENOVO 422916G/LENOVO, BIOS A1KT53AUS 04/07/2015 [ 29.382882] 3ebfc20364115825 88003c48 81892d4b [ 29.382943] 81e124e0 88003c78 8110bcd7 [ 29.383004] 8800785c9d00 88065485ac58 880c62002800 880c5fc88ac0 [ 29.383065] Call Trace: [ 29.383084][] dump_stack+0x4c/0x65 [ 29.383130] [] lockdep_rcu_suspicious+0xe7/0x120 [ 29.383178] [] br_fill_ifinfo+0x4a9/0x6a0 [bridge] [ 29.383225] [] br_ifinfo_notify+0x11b/0x4b0 [bridge] [ 29.383271] [] ? br_hold_timer_expired+0x70/0x70 [bridge] [ 29.383320] [] br_forward_delay_timer_expired+0x58/0x140 [bridge] [ 29.383371] [] ? br_hold_timer_expired+0x70/0x70 [bridge] [ 29.383416] [] call_timer_fn+0xc3/0x4f0 [ 29.383454] [] ? call_timer_fn+0x5/0x4f0 [ 29.383493] [] ? lock_release_holdtime.part.29+0xf/0x200 [ 29.383541] [] ? br_hold_timer_expired+0x70/0x70 [bridge] [ 29.383587] [] run_timer_softirq+0x244/0x490 [ 29.383629] [] __do_softirq+0xec/0x670 [ 29.383666] [] irq_exit+0x145/0x150 [ 29.383703] [] smp_apic_timer_interrupt+0x46/0x60 [ 29.383744] [] apic_timer_interrupt+0x73/0x80 [ 29.383782][] ? cpuidle_enter_state+0x5f/0x2f0 [ 29.383832] [] ? cpuidle_enter_state+0x5b/0x2f0 [ 29.383873] [] cpuidle_enter+0x17/0x20 [ 29.383908] [] cpu_startup_entry+0x36f/0x5f0 [ 29.383949] [] rest_init+0x13d/0x150 [ 29.383986] [] start_kernel+0x4d2/0x4f3 [ 29.384023] [] ? early_idt_handlers+0x120/0x120 [ 29.384064] [] x86_64_start_reservations+0x2a/0x2c [ 29.384105] [] x86_64_start_kernel+0x14a/0x16d [ 36.100347] IN=br0 OUT= MAC=33:33:00:00:00:02:52:54:00:12:01:00:86:dd SRC=fe80::::5054:00ff:fe12:0100 DST=ff02:::::::0002 LEN=56 TC=0 HOPLIMIT=255 FLOWLBL=0 PROTO=ICMPv6 TYPE=133 CODE=0 [ 40.113744] IN=br0 OUT= MAC=33:33:00:00:00:02:52:54:00:12:01:00:86:dd SRC=fe80::::5054:00ff:fe12:0100 DST=ff02:::::::0002 LEN=56 TC=0 HOPLIMIT=255 FLOWLBL=0 PROTO=ICMPv6 TYPE=133 CODE=0 [ 44.119727] IN=br0 OUT= MAC=33:33:00:00:00:02:52:54:00:12:01:00:86:dd SRC=fe80::::5054:00ff:fe12:0100 DST=ff02:::::::0002 LEN=56 TC=0 HOPLIMIT=255 FLOWLBL=0 PROTO=ICMPv6 TYPE=133 CODE=0 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: net: unix: garbage: fixed several comment and whitespace style issues
From: Jason Eastman Date: Wed, 22 Apr 2015 00:56:42 -0600 > fixed several comment and whitespace style issues > > Signed-off-by: Jason Eastman Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fix tcp fin memory accounting
From: Eric Dumazet Date: Thu, 23 Apr 2015 09:48:12 -0700 > On Thu, 2015-04-23 at 09:16 -0700, Eric Dumazet wrote: >> On Thu, 2015-04-23 at 11:54 -0400, David Miller wrote: >> > From: Eric Dumazet >> > Date: Thu, 23 Apr 2015 07:02:47 -0700 >> > >> > > +if (!tcp_send_head(sk)) { >> > > +tp->snd_nxt++; >> > > +return; >> > > +} >> > >> > I'm not so sure about this. Why is this needed? >> > >> > Otherwise patch looks fine to me. >> > >> >> I guess I need to add a comment then ;) >> >> If we want to pretend FIN was sent, we also need to tweak tp->snd_nxt to >> match new tskb->end_seq (or tp->write_seq). >> >> I tested following packetdrill script and confirmed that if I do not >> tweak snd_nxt, last packet sent is incorrect : >> >> > . 5001:5001(0) ack 2 >> >> This might be because our stack relies that we never coalesce something >> on one already sent skb (we do this check in tcp_sendmsg() for example) > > Well, real reason is that tp->snd_nxt is not touched in retransmit > paths, but when new data is sent (tcp_event_new_data_sent()) That makes sense, thanks for explaining. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Suspicious RCU usage in bridge with Linux v4.0-9362-g1fc149933fd4
On Thu, Apr 23, 2015 at 6:07 PM, Josh Boyer wrote: > Hi All, > > We've had a user report the following backtrace from the bridge module > with a recent Linus' tree. Has anything like this been reported yet? > If you have any questions on setup, the user is CC'd. > I too observed similar backtrace once(not able to reproduce it again) while I was trying to check inconsistent lock state[1] with lockdep enabled. Regards, Sudeep [1] https://lkml.org/lkml/2015/4/23/329 --->8 === [ INFO: suspicious RCU usage. ] 4.0.0 #269 Not tainted --- include/trace/events/ipi.h:68 suspicious rcu_dereference_check() usage! other info that might help us debug this: RCU used illegally from idle CPU! rcu_scheduler_active = 1, debug_locks = 1 RCU used illegally from extended quiescent state! no locks held by swapper/0/0. stack backtrace: CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.0.0 #269 Hardware name: ARM-Versatile Express [] (unwind_backtrace) from [] (show_stack+0x11/0x14) [] (show_stack) from [] (dump_stack+0x73/0x8c) [] (dump_stack) from [] (handle_IPI+0x257/0x410) [] (handle_IPI) from [] (gic_handle_irq+0x4f/0x50) [] (gic_handle_irq) from [] (__irq_svc+0x3f/0x64) Exception stack(0xc083bf30 to 0xc083bf78) bf20: 0001 0001 c0841938 bf40: c083a000 c083d5fc c08ba5c8 c083d598 c08b938f c056afc8 bf60: 0008 c083bf78 c005d157 c000f288 4133 [] (__irq_svc) from [] (arch_cpu_idle+0x30/0x34) [] (arch_cpu_idle) from [] (cpu_startup_entry+0x339/0x404) [] (cpu_startup_entry) from [] (start_kernel+0x32f/0x338) [] (start_kernel) from [<8000807f>] (0x8000807f) === [ INFO: suspicious RCU usage. ] 4.0.0 #269 Not tainted --- include/trace/events/ipi.h:84 suspicious rcu_dereference_check() usage! other info that might help us debug this: RCU used illegally from idle CPU! rcu_scheduler_active = 1, debug_locks = 1 RCU used illegally from extended quiescent state! no locks held by swapper/0/0. stack backtrace: CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.0.0 #269 Hardware name: ARM-Versatile Express [] (unwind_backtrace) from [] (show_stack+0x11/0x14) [] (show_stack) from [] (dump_stack+0x73/0x8c) [] (dump_stack) from [] (handle_IPI+0x379/0x410) [] (handle_IPI) from [] (gic_handle_irq+0x4f/0x50) [] (gic_handle_irq) from [] (__irq_svc+0x3f/0x64) Exception stack(0xc083bf30 to 0xc083bf78) bf20: 0001 0001 c0841938 bf40: c083a000 c083d5fc c08ba5c8 c083d598 c08b938f c056afc8 bf60: 0008 c083bf78 c005d157 c000f288 4133 [] (__irq_svc) from [] (arch_cpu_idle+0x30/0x34) [] (arch_cpu_idle) from [] (cpu_startup_entry+0x339/0x404) [] (cpu_startup_entry) from [] (start_kernel+0x32f/0x338) [] (start_kernel) from [<8000807f>] (0x8000807f) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net] tcp: avoid looping in tcp_send_fin()
From: Eric Dumazet Presence of an unbound loop in tcp_send_fin() had always been hard to explain when analyzing crash dumps involving gigantic dying processes with millions of sockets. Lets try a different strategy : In case of memory pressure, try to add the FIN flag to last packet in write queue, even if packet was already sent. TCP stack will be able to deliver this FIN after a timeout event. Note that this FIN being delivered by a retransmit, it also carries a Push flag given our current implementation. By checking sk_under_memory_pressure(), we anticipate that cooking many FIN packets might deplete tcp memory. In the case we could not allocate a packet, even with __GFP_WAIT allocation, then not sending a FIN seems quite reasonable if it allows to get rid of this socket, free memory, and not block the process from eventually doing other useful work. Signed-off-by: Eric Dumazet --- net/ipv4/tcp_output.c | 50 +++- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 2ade67b7cdb0..a369e8a70b2c 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -2814,7 +2814,8 @@ begin_fwd: /* We allow to exceed memory limits for FIN packets to expedite * connection tear down and (memory) recovery. - * Otherwise tcp_send_fin() could loop forever. + * Otherwise tcp_send_fin() could be tempted to either delay FIN + * or even be forced to close flow without any FIN. */ static void sk_forced_wmem_schedule(struct sock *sk, int size) { @@ -2827,33 +2828,40 @@ static void sk_forced_wmem_schedule(struct sock *sk, int size) sk_memory_allocated_add(sk, amt, &status); } -/* Send a fin. The caller locks the socket for us. This cannot be - * allowed to fail queueing a FIN frame under any circumstances. +/* Send a FIN. The caller locks the socket for us. + * We should try to send a FIN packet really hard, but eventually give up. */ void tcp_send_fin(struct sock *sk) { + struct sk_buff *skb, *tskb = tcp_write_queue_tail(sk); struct tcp_sock *tp = tcp_sk(sk); - struct sk_buff *skb = tcp_write_queue_tail(sk); - int mss_now; - /* Optimization, tack on the FIN if we have a queue of -* unsent frames. But be careful about outgoing SACKS -* and IP options. + /* Optimization, tack on the FIN if we have one skb in write queue and +* this skb was not yet sent, or we are under memory pressure. +* Note: in the latter case, FIN packet will be sent after a timeout, +* as TCP stack thinks it has already been transmitted. */ - mss_now = tcp_current_mss(sk); - - if (tcp_send_head(sk)) { - TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_FIN; - TCP_SKB_CB(skb)->end_seq++; + if (tskb && (tcp_send_head(sk) || sk_under_memory_pressure(sk))) { +coalesce: + TCP_SKB_CB(tskb)->tcp_flags |= TCPHDR_FIN; + TCP_SKB_CB(tskb)->end_seq++; tp->write_seq++; + if (!tcp_send_head(sk)) { + /* This means tskb was already sent. +* Pretend we included the FIN on previous transmit. +* We need to set tp->snd_nxt to the value it would have +* if FIN had been sent. This is because retransmit path +* does not change tp->snd_nxt. +*/ + tp->snd_nxt++; + return; + } } else { - /* Socket is locked, keep trying until memory is available. */ - for (;;) { - skb = alloc_skb_fclone(MAX_TCP_HEADER, - sk->sk_allocation); - if (skb) - break; - yield(); + skb = alloc_skb_fclone(MAX_TCP_HEADER, sk->sk_allocation); + if (unlikely(!skb)) { + if (tskb) + goto coalesce; + return; } skb_reserve(skb, MAX_TCP_HEADER); sk_forced_wmem_schedule(sk, skb->truesize); @@ -2862,7 +2870,7 @@ void tcp_send_fin(struct sock *sk) TCPHDR_ACK | TCPHDR_FIN); tcp_queue_skb(sk, skb); } - __tcp_push_pending_frames(sk, mss_now, TCP_NAGLE_OFF); + __tcp_push_pending_frames(sk, tcp_current_mss(sk), TCP_NAGLE_OFF); } /* We get here when a process closes a file descriptor (either due to -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] ethernet: amd: AMD_XGBE should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `xgbe_probe': xgbe-main.c:(.text+0x2def0a): undefined reference to `dma_set_mask' xgbe-main.c:(.text+0x2def20): undefined reference to `dma_supported' drivers/built-in.o: In function `xgbe_rx_poll': xgbe-drv.c:(.text+0x2e0320): undefined reference to `dma_sync_single_for_cpu' xgbe-drv.c:(.text+0x2e035e): undefined reference to `dma_sync_single_for_cpu' drivers/built-in.o: In function `xgbe_unmap_rdata': xgbe-desc.c:(.text+0x2e5fe4): undefined reference to `dma_unmap_page' xgbe-desc.c:(.text+0x2e5ffa): undefined reference to `dma_unmap_single' xgbe-desc.c:(.text+0x2e604a): undefined reference to `dma_unmap_page' xgbe-desc.c:(.text+0x2e6084): undefined reference to `dma_unmap_page' drivers/built-in.o: In function `xgbe_alloc_pages': xgbe-desc.c:(.text+0x2e6156): undefined reference to `dma_map_page' xgbe-desc.c:(.text+0x2e6164): undefined reference to `dma_mapping_error' drivers/built-in.o: In function `xgbe_free_ring': xgbe-desc.c:(.text+0x2e63d4): undefined reference to `dma_unmap_page' xgbe-desc.c:(.text+0x2e640e): undefined reference to `dma_unmap_page' xgbe-desc.c:(.text+0x2e644a): undefined reference to `dma_free_coherent' drivers/built-in.o: In function `xgbe_init_ring': xgbe-desc.c:(.text+0x2e64d4): undefined reference to `dma_alloc_coherent' drivers/built-in.o: In function `xgbe_map_tx_skb': xgbe-desc.c:(.text+0x2e6628): undefined reference to `dma_map_single' xgbe-desc.c:(.text+0x2e6638): undefined reference to `dma_mapping_error' xgbe-desc.c:(.text+0x2e66b2): undefined reference to `dma_map_single' xgbe-desc.c:(.text+0x2e66c2): undefined reference to `dma_mapping_error' xgbe-desc.c:(.text+0x2e6762): undefined reference to `dma_map_page' xgbe-desc.c:(.text+0x2e6772): undefined reference to `dma_mapping_error' Signed-off-by: Geert Uytterhoeven --- drivers/net/ethernet/amd/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/amd/Kconfig b/drivers/net/ethernet/amd/Kconfig index c638c85f3954bc68..089c269637b725da 100644 --- a/drivers/net/ethernet/amd/Kconfig +++ b/drivers/net/ethernet/amd/Kconfig @@ -179,7 +179,7 @@ config SUNLANCE config AMD_XGBE tristate "AMD 10GbE Ethernet driver" - depends on (OF_NET || ACPI) && HAS_IOMEM + depends on (OF_NET || ACPI) && HAS_IOMEM && HAS_DMA select PHYLIB select AMD_XGBE_PHY select BITREVERSE -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] ethernet: arc: ARC_EMAC and EMAC_ROCKCHIP should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `arc_emac_tx_clean': emac_main.c:(.text+0x2decde): undefined reference to `dma_unmap_single' drivers/built-in.o: In function `arc_emac_rx': emac_main.c:(.text+0x2dee1c): undefined reference to `dma_unmap_single' emac_main.c:(.text+0x2dee72): undefined reference to `dma_map_single' emac_main.c:(.text+0x2dee7e): undefined reference to `dma_mapping_error' drivers/built-in.o: In function `arc_emac_probe': (.text+0x2df2ee): undefined reference to `dmam_alloc_coherent' drivers/built-in.o: In function `arc_emac_open': emac_main.c:(.text+0x2df6d8): undefined reference to `dma_map_single' emac_main.c:(.text+0x2df6e4): undefined reference to `dma_mapping_error' drivers/built-in.o: In function `arc_emac_tx': emac_main.c:(.text+0x2df9e4): undefined reference to `dma_map_single' emac_main.c:(.text+0x2df9f0): undefined reference to `dma_mapping_error' Signed-off-by: Geert Uytterhoeven --- drivers/net/ethernet/arc/Kconfig | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/arc/Kconfig b/drivers/net/ethernet/arc/Kconfig index 8e262e2b39b63fc5..dea29ee24da4a28c 100644 --- a/drivers/net/ethernet/arc/Kconfig +++ b/drivers/net/ethernet/arc/Kconfig @@ -25,8 +25,7 @@ config ARC_EMAC_CORE config ARC_EMAC tristate "ARC EMAC support" select ARC_EMAC_CORE - depends on OF_IRQ - depends on OF_NET + depends on OF_IRQ && OF_NET && HAS_DMA ---help--- On some legacy ARC (Synopsys) FPGA boards such as ARCAngel4/ML50x non-standard on-chip ethernet device ARC EMAC 10/100 is used. @@ -35,7 +34,7 @@ config ARC_EMAC config EMAC_ROCKCHIP tristate "Rockchip EMAC support" select ARC_EMAC_CORE - depends on OF_IRQ && OF_NET && REGULATOR + depends on OF_IRQ && OF_NET && REGULATOR && HAS_DMA ---help--- Support for Rockchip RK3066/RK3188 EMAC ethernet controllers. This selects Rockchip SoC glue layer support for the -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] can: CAN_GRCAN should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `grcan_free_dma_buffers': grcan.c:(.text+0x2d7716): undefined reference to `dma_free_coherent' drivers/built-in.o: In function `grcan_allocate_dma_buffers': grcan.c:(.text+0x2d779c): undefined reference to `dma_alloc_coherent' Signed-off-by: Geert Uytterhoeven --- drivers/net/can/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index 58808f6514520c86..e8c96b8e86f48e66 100644 --- a/drivers/net/can/Kconfig +++ b/drivers/net/can/Kconfig @@ -112,7 +112,7 @@ config PCH_CAN config CAN_GRCAN tristate "Aeroflex Gaisler GRCAN and GRHCAN CAN devices" - depends on OF + depends on OF && HAS_DMA ---help--- Say Y here if you want to use Aeroflex Gaisler GRCAN or GRHCAN. Note that the driver supports little endian, even though little -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch net] igb: pass the correct maxlen for eth_get_headlen()
On Wed, Apr 22, 2015 at 8:40 PM, Alexander Duyck wrote: > On 04/22/2015 04:23 PM, Cong Wang wrote: >> On Wed, Apr 22, 2015 at 3:34 PM, Alexander Duyck >> wrote: >>> On 04/22/2015 02:56 PM, Cong Wang wrote: On Wed, Apr 22, 2015 at 2:42 PM, Alexander Duyck wrote: > On 04/22/2015 01:33 PM, Cong Wang wrote: >> First, make sure you don't miss the TSIP case right above: >> >> The frag starting pointer and its size are advanced by: >> >> skb_frag_size_sub(frag, IGB_TS_HDR_LEN); >> ... >> va += IGB_TS_HDR_LEN; >> >> even though we unlikely pull header longer than >> IGB_RX_HDR_LEN - IGB_TS_HDR_LEN either. > So I believe this is a possible bug, one heck of a corner case to get > into though. It requires timestamp in packet, size 240 - 256, and a > malformed header. > > The proper fix would probably be to pull the timestamp out of the packet > before we add it to the frame. I'll submit a patch to address this. > Huh? Doesn't my patch already fix this? skb_frag_size() is always up to date. Or you mean another different problem? >>> Your patch has other issues. I am still NAKing your patch, but there is >>> an issue with igb that you have pointed out. The proper fix would be to >>> deal with the timestamp before we add the page fragment to the skb. >>> >> If the first frag is always 2K, then this is not a problem either. >> IGB_TS_HDR_LEN + IGB_RX_HDR_LEN < 2K. > > The problem is skb->tail will get screwed up. > Why? We don't copy timestamp into skb header, instead it is saved in shared_info, why skb->tail matters here in TSIP case? [...] > > The first frag will be 2K in size only if there are multiple frags. The Or it doesn't have frags at all since we just copy it to skb header, right? > problem in the TSIP case is that we will put the size reported by the > descriptor into the fragment, and then try to pull the size we That size is saved when adding the frag, in TSIP case we just sub it by IGB_TS_HDR_LEN, which seems correct. > determined via eth_get_headlen. So there is a window where that value > can be wrong and result in data_len going negative and tail being moved > beyond the end of the actual data. This sounds like a different problem if you are saying we should sub the size in the descriptor too. Therefore I don't see why your patch could replace mine. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ethernet: arc: ARC_EMAC and EMAC_ROCKCHIP should depend on HAS_DMA
From: Geert Uytterhoeven Date: Thu, 23 Apr 2015 19:59:34 +0200 > If NO_DMA=y: ... > Signed-off-by: Geert Uytterhoeven Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] can: CAN_GRCAN should depend on HAS_DMA
From: Geert Uytterhoeven Date: Thu, 23 Apr 2015 20:04:51 +0200 > If NO_DMA=y: ... > Signed-off-by: Geert Uytterhoeven Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On Wed, Apr 22, 2015 at 7:46 PM, Alexei Starovoitov wrote: > > TC_ACT_QUEUED cannot be removed. > Only deprecated with backwards compatibility the way this patch did it. > That should have been obvious. > It is at least the third time I have to repeat that: we really don't care about out-of-tree modules. Everyone MUST read Documentation/stable_api_nonsense.txt. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] ethernet: amd: AMD_XGBE should depend on HAS_DMA
From: Geert Uytterhoeven Date: Thu, 23 Apr 2015 19:59:33 +0200 > If NO_DMA=y: ... > Signed-off-by: Geert Uytterhoeven Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
NAPI: Polling function requesting extra calls - difference between 3.18.11 and 4.0.0
Hi, Recently Felix improved bgmac driver to be smarter in situation where new packets were Tx/Rx-ed during bgmac_poll execution. It was handled in: eb64e29 bgmac: leave interrupts disabled as long as there is work to do http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=eb64e2923a886441c7b322f138b36029f3fa6a36 With above change, after handling all skb-s in bgmac_poll, bgmac checks if there are new sbk-s to be handled. If so, it doesn't call napi_complete & it doesn't enable interrupts. Above commit was merged for 4.1 kernel, but I work with OpenWrt based on older releases, so I have all bgmac changes backported to 3.8.11 and 4.0. With 3.8.11 Felix's change seems to be working fine, I did some debugging: [ 44.97] [bgmac_interrupt] int_status & int_mask = 0x0100 [ 44.98] [bgmac_poll] START weight:64 [ 44.99] [bgmac_poll] Read handled:0 skb-s [ 44.99] [bgmac_poll] Read IRQ status again BGMAC_IS_TX0:0 BGMAC_IS_RX:0 [ 45.00] [bgmac_poll] Read skb-s < weight, calling napi_complete, en IRQs [ 45.01] [bgmac_poll] END returning handled:0 [ 48.04] [bgmac_interrupt] int_status & int_mask = 0x0100 [ 48.05] [bgmac_poll] START weight:64 [ 48.06] [bgmac_poll] Read handled:0 skb-s [ 48.06] [bgmac_poll] Read IRQ status again BGMAC_IS_TX0:0 BGMAC_IS_RX:0 [ 48.07] [bgmac_poll] Read skb-s < weight, calling napi_complete, en IRQs [ 48.08] [bgmac_poll] END returning handled:0 [ 48.10] [bgmac_poll] START weight:64 [ 48.11] [bgmac_poll] Read handled:1 skb-s [ 48.11] [bgmac_poll] Read IRQ status again BGMAC_IS_TX0:1 BGMAC_IS_RX:1 [ 48.12] [bgmac_poll] END more skb-s to handle, returning handled:1 [ 48.13] [bgmac_poll] START weight:64 [ 48.13] [bgmac_poll] Read handled:1 skb-s [ 48.14] [bgmac_poll] Read IRQ status again BGMAC_IS_TX0:0 BGMAC_IS_RX:0 [ 48.15] [bgmac_poll] Read skb-s < weight, calling napi_complete, en IRQs [ 48.16] [bgmac_poll] END returning handled:1 Please note that at 48.12 bgmac decided bgmac_poll should be called again and it didn't call napi_complete & didn't enable IRQs. It simply returned 1. NAPI behaved just like we expected, bgmac_poll was called again a moment later. It handled skb-s that appeared meanwhile and ended calling napi_complete and returning 1. Now, it fails badly for me at all when using kernel 4.0.0: [ 52.80] [bgmac_interrupt] int_status & int_mask = 0x0100 [ 52.80] [bgmac_poll] START weight:64 [ 52.81] [bgmac_poll] Read handled:0 skb-s [ 52.81] [bgmac_poll] Read IRQ status again BGMAC_IS_TX0:0 BGMAC_IS_RX:0 [ 52.82] [bgmac_poll] Read skb-s < weight, calling napi_complete, en IRQs [ 52.83] [bgmac_poll] END returning handled:0 [ 54.61] [bgmac_interrupt] int_status & int_mask = 0x0100 [ 54.62] [bgmac_poll] START weight:64 [ 54.63] [bgmac_poll] Read handled:0 skb-s [ 54.63] [bgmac_poll] Read IRQ status again BGMAC_IS_TX0:0 BGMAC_IS_RX:0 [ 54.64] [bgmac_poll] Read skb-s < weight, calling napi_complete, en IRQs [ 54.65] [bgmac_poll] END returning handled:0 [ 55.90] [bgmac_interrupt] int_status & int_mask = 0x0100 [ 55.91] [bgmac_poll] START weight:64 [ 55.92] [bgmac_poll] Read handled:1 skb-s [ 55.92] [bgmac_poll] Read IRQ status again BGMAC_IS_TX0:1 BGMAC_IS_RX:1 [ 55.93] [bgmac_poll] END more skb-s to handle, returning handled:1 As you can see, at 55.93 bgmac also decided bgmac_poll should be called again. It didn't call napi_complete, didn't enable IRQs. It simply returned 1. Just like with kernel 3.8.11. It this case however, NAPI never called bgmac_poll again. It resulted in bgmac hanging (IRQs are off, bgmac expects bgmac_poll to be called). Can you help us with this, please? Does bgmac use NAPI incorrectly? Were there some important changes in 3.19 or 4.0? -- Rafał diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c index 4929932..40ce34d 100644 --- a/drivers/net/ethernet/broadcom/bgmac.c +++ b/drivers/net/ethernet/broadcom/bgmac.c @@ -1219,6 +1219,7 @@ static irqreturn_t bgmac_interrupt(int irq, void *dev_id) struct bgmac *bgmac = netdev_priv(dev_id); u32 int_status = bgmac_read(bgmac, BGMAC_INT_STATUS); + pr_info("[%s] int_status:0x%08x & int_mask:0x%08x = 0x%08x\n", __FUNCTION__, int_status, bgmac->int_mask, int_status & bgmac->int_mask); int_status &= bgmac->int_mask; if (!int_status) @@ -1240,22 +1241,31 @@ static int bgmac_poll(struct napi_struct *napi, int weight) { struct bgmac *bgmac = container_of(napi, struct bgmac, napi); int handled = 0; + u32 int_status; + pr_info("[%s] START weight:%d\n", __FUNCTION__, weight); /* Ack */ bgmac_write(bgmac, BGMAC_INT_STATUS, ~0); bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0]); handled += bgmac_dma_rx_re
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On 04/23/2015 08:12 PM, Cong Wang wrote: On Wed, Apr 22, 2015 at 7:46 PM, Alexei Starovoitov wrote: ... TC_ACT_QUEUED cannot be removed. Only deprecated with backwards compatibility the way this patch did it. That should have been obvious. It is at least the third time I have to repeat that: we really don't care about out-of-tree modules. Everyone MUST read Documentation/stable_api_nonsense.txt. Seriously, what are you talking about ??? $ git grep -n TC_ACT_QUEUED include/uapi/ include/uapi/linux/pkt_cls.h:105:#define TC_ACT_QUEUED 5 This is *UAPI*, no-one is even remotely talking about out-of-tree kernel modules. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: NAPI: Polling function requesting extra calls - difference between 3.18.11 and 4.0.0
On Thu, 2015-04-23 at 20:18 +0200, Rafał Miłecki wrote: > > Can you help us with this, please? Does bgmac use NAPI incorrectly? > Were there some important changes in 3.19 or 4.0? > Right they were important changes in NAPI indeed : 3b47d30396ba net: gro: add a per device gro flush timer d75b1ade567f net: less interrupt masking in NAPI bc9ad166e38a net: introduce napi_schedule_irqoff() This requested some fixes in various drivers : commit f31ec95fa19e07a8beebcc0297284f23aa57967e commit 24e579c8898aa641ede3149234906982290934e5 commit 6088beef3f7517717bd21d90b379714dd0837079 commit f104fedc0da126abe93dd0f4a9fa13e5133bf9df commit 7a05dc64e2e4c611d89007b125b20c0d2a4d31a5 commit 001ce546bb537bb5b7821f05633556a0c9787e32 commit 3079c652141f9d6377417a7e8fd650c9948df65e commit 8acdf999accfd95093db17f33a58429a38782060 commit 6a6dc08ff6395f58be3ee568cb970ea956f16819 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ethernet: myri10ge: use arch_phys_wc_add()
From: "Luis R. Rodriguez" Date: Tue, 21 Apr 2015 13:09:45 -0700 > From: "Luis R. Rodriguez" > > This driver already uses ioremap_wc() on the same range > so when write-combining is available that will be used > instead. > ... > Signed-off-by: Luis R. Rodriguez I can't wait forever for the driver maintainers to review this, so I'm applying it. Thanks Luis. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On Thu, Apr 23, 2015 at 11:21 AM, Daniel Borkmann wrote: > On 04/23/2015 08:12 PM, Cong Wang wrote: >> >> On Wed, Apr 22, 2015 at 7:46 PM, Alexei Starovoitov >> wrote: > > ... >>> >>> TC_ACT_QUEUED cannot be removed. >>> Only deprecated with backwards compatibility the way this patch did it. >>> That should have been obvious. >> >> >> It is at least the third time I have to repeat that: we really don't care >> about out-of-tree modules. >> >> Everyone MUST read Documentation/stable_api_nonsense.txt. > > > Seriously, what are you talking about ??? > > $ git grep -n TC_ACT_QUEUED include/uapi/ > include/uapi/linux/pkt_cls.h:105:#define TC_ACT_QUEUED 5 > > This is *UAPI*, no-one is even remotely talking about out-of-tree > kernel modules. I am not stupid. You should figure out we can just leave its definition by removing it, leaving the default as stolen. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: NAPI: Polling function requesting extra calls - difference between 3.18.11 and 4.0.0
On Thu, 2015-04-23 at 11:28 -0700, Eric Dumazet wrote: > On Thu, 2015-04-23 at 20:18 +0200, Rafał Miłecki wrote: > > > > > Can you help us with this, please? Does bgmac use NAPI incorrectly? > > Were there some important changes in 3.19 or 4.0? > > > > Right they were important changes in NAPI indeed : > > 3b47d30396ba net: gro: add a per device gro flush timer > d75b1ade567f net: less interrupt masking in NAPI > bc9ad166e38a net: introduce napi_schedule_irqoff() > > This requested some fixes in various drivers : > > commit f31ec95fa19e07a8beebcc0297284f23aa57967e > commit 24e579c8898aa641ede3149234906982290934e5 > commit 6088beef3f7517717bd21d90b379714dd0837079 > commit f104fedc0da126abe93dd0f4a9fa13e5133bf9df > commit 7a05dc64e2e4c611d89007b125b20c0d2a4d31a5 > commit 001ce546bb537bb5b7821f05633556a0c9787e32 > commit 3079c652141f9d6377417a7e8fd650c9948df65e > commit 8acdf999accfd95093db17f33a58429a38782060 > commit 6a6dc08ff6395f58be3ee568cb970ea956f16819 > It looks like following fix is needed : diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c index de77d3a74abc..21e3c38c7c75 100644 --- a/drivers/net/ethernet/broadcom/bgmac.c +++ b/drivers/net/ethernet/broadcom/bgmac.c @@ -1260,7 +1260,7 @@ static int bgmac_poll(struct napi_struct *napi, int weight) /* Poll again if more events arrived in the meantime */ if (bgmac_read(bgmac, BGMAC_INT_STATUS) & (BGMAC_IS_TX0 | BGMAC_IS_RX)) - return handled; + return weight; if (handled < weight) { napi_complete(napi); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: NAPI: Polling function requesting extra calls - difference between 3.18.11 and 4.0.0
On 23 April 2015 at 20:33, Eric Dumazet wrote: > On Thu, 2015-04-23 at 11:28 -0700, Eric Dumazet wrote: >> On Thu, 2015-04-23 at 20:18 +0200, Rafał Miłecki wrote: >> >> > >> > Can you help us with this, please? Does bgmac use NAPI incorrectly? >> > Were there some important changes in 3.19 or 4.0? >> > >> >> Right they were important changes in NAPI indeed : >> >> 3b47d30396ba net: gro: add a per device gro flush timer >> d75b1ade567f net: less interrupt masking in NAPI >> bc9ad166e38a net: introduce napi_schedule_irqoff() >> >> This requested some fixes in various drivers : >> >> commit f31ec95fa19e07a8beebcc0297284f23aa57967e >> commit 24e579c8898aa641ede3149234906982290934e5 >> commit 6088beef3f7517717bd21d90b379714dd0837079 >> commit f104fedc0da126abe93dd0f4a9fa13e5133bf9df >> commit 7a05dc64e2e4c611d89007b125b20c0d2a4d31a5 >> commit 001ce546bb537bb5b7821f05633556a0c9787e32 >> commit 3079c652141f9d6377417a7e8fd650c9948df65e >> commit 8acdf999accfd95093db17f33a58429a38782060 >> commit 6a6dc08ff6395f58be3ee568cb970ea956f16819 >> > > It looks like following fix is needed : > > diff --git a/drivers/net/ethernet/broadcom/bgmac.c > b/drivers/net/ethernet/broadcom/bgmac.c > index de77d3a74abc..21e3c38c7c75 100644 > --- a/drivers/net/ethernet/broadcom/bgmac.c > +++ b/drivers/net/ethernet/broadcom/bgmac.c > @@ -1260,7 +1260,7 @@ static int bgmac_poll(struct napi_struct *napi, int > weight) > > /* Poll again if more events arrived in the meantime */ > if (bgmac_read(bgmac, BGMAC_INT_STATUS) & (BGMAC_IS_TX0 | > BGMAC_IS_RX)) > - return handled; > + return weight; > > if (handled < weight) { > napi_complete(napi); > > Yeah, I'm flashing my device with the same patch applied right now :) -- Rafał -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch net] igb: pass the correct maxlen for eth_get_headlen()
On 04/23/2015 11:06 AM, Cong Wang wrote: > On Wed, Apr 22, 2015 at 8:40 PM, Alexander Duyck > wrote: >> On 04/22/2015 04:23 PM, Cong Wang wrote: >>> On Wed, Apr 22, 2015 at 3:34 PM, Alexander Duyck >>> wrote: On 04/22/2015 02:56 PM, Cong Wang wrote: > On Wed, Apr 22, 2015 at 2:42 PM, Alexander Duyck > wrote: >> On 04/22/2015 01:33 PM, Cong Wang wrote: >>> First, make sure you don't miss the TSIP case right above: >>> >>> The frag starting pointer and its size are advanced by: >>> >>> skb_frag_size_sub(frag, IGB_TS_HDR_LEN); >>> ... >>> va += IGB_TS_HDR_LEN; >>> >>> even though we unlikely pull header longer than >>> IGB_RX_HDR_LEN - IGB_TS_HDR_LEN either. >> So I believe this is a possible bug, one heck of a corner case to get >> into though. It requires timestamp in packet, size 240 - 256, and a >> malformed header. >> >> The proper fix would probably be to pull the timestamp out of the packet >> before we add it to the frame. I'll submit a patch to address this. >> > Huh? Doesn't my patch already fix this? skb_frag_size() is always > up to date. Or you mean another different problem? Your patch has other issues. I am still NAKing your patch, but there is an issue with igb that you have pointed out. The proper fix would be to deal with the timestamp before we add the page fragment to the skb. >>> If the first frag is always 2K, then this is not a problem either. >>> IGB_TS_HDR_LEN + IGB_RX_HDR_LEN < 2K. >> The problem is skb->tail will get screwed up. >> > Why? We don't copy timestamp into skb header, instead it is saved > in shared_info, why skb->tail matters here in TSIP case? TSIP only matters if you have a network header with bad data in it that indicates the size is actually larger than it actually is. >> The first frag will be 2K in size only if there are multiple frags. The > > Or it doesn't have frags at all since we just copy it to skb header, right? That is moot since this code only gets called if the skb is nonlinear >> problem in the TSIP case is that we will put the size reported by the >> descriptor into the fragment, and then try to pull the size we > > That size is saved when adding the frag, in TSIP case we just sub it > by IGB_TS_HDR_LEN, which seems correct. > Yes, the size is saved. But with your solution we could pull the whole fragment but not free it which isn't correct as we shouldn't be left with any 0 sized fragments. >> determined via eth_get_headlen. So there is a window where that value >> can be wrong and result in data_len going negative and tail being moved >> beyond the end of the actual data. > This sounds like a different problem if you are saying we should sub > the size in the descriptor too. Therefore I don't see why your patch > could replace mine. Your patches sort-of fixed the problem, but they introduced other issues in the process. The thing I don't think you are getting is that the code was meant to be mutually exclusive w/ the copy-break code. Either the frag is less than IGB_RX_HDR_SIZE in which case we copy-break and don't attached the fragment, or we should be pulling the header and leaving at least 1 byte in the fragment. The problem with your solution is that you potentially pull the entire fragment in, but you don't free it if the size drops to 0. That is why in the other mail I told you the better solution for igb would likely be to just pass IGB_RX_HDR_SIZE - IGB_TS_HDR_SIZE as that way you end up with at least 1 byte left in the fragment after you pull the header. - Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] altera tse: add support for fixed-links.
From: Andreas Oetken Date: Wed, 22 Apr 2015 15:22:22 +0200 > + /* check if a fixed-link is defined in device-tree */ > + if (of_phy_is_fixed_link(priv->device->of_node)) { > + if (of_phy_register_fixed_link(priv->device->of_node) > + < 0) { You should store the return value of of_phy_register_fixed_link() and return that as the error code when it fails. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH][FIX 4.1] bgmac: fix requests for extra polling calls from NAPI
After d75b1ade567f ("net: less interrupt masking in NAPI") polling function has to return whole budget when it wants NAPI to call it again. Signed-off-by: Rafał Miłecki Cc: Felix Fietkau Fixes: eb64e2923a886 ("bgmac: leave interrupts disabled as long as there is work to do") --- drivers/net/ethernet/broadcom/bgmac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c index de77d3a..21e3c38 100644 --- a/drivers/net/ethernet/broadcom/bgmac.c +++ b/drivers/net/ethernet/broadcom/bgmac.c @@ -1260,7 +1260,7 @@ static int bgmac_poll(struct napi_struct *napi, int weight) /* Poll again if more events arrived in the meantime */ if (bgmac_read(bgmac, BGMAC_INT_STATUS) & (BGMAC_IS_TX0 | BGMAC_IS_RX)) - return handled; + return weight; if (handled < weight) { napi_complete(napi); -- 1.8.4.5 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][FIX 4.1] bgmac: fix requests for extra polling calls from NAPI
On 2015-04-23 20:56, Rafał Miłecki wrote: > After d75b1ade567f ("net: less interrupt masking in NAPI") polling > function has to return whole budget when it wants NAPI to call it again. > > Signed-off-by: Rafał Miłecki > Cc: Felix Fietkau > Fixes: eb64e2923a886 ("bgmac: leave interrupts disabled as long as there is > work to do") Acked-by: Felix Fietkau - Felix -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][FIX 4.1] bgmac: fix requests for extra polling calls from NAPI
On Thu, 2015-04-23 at 20:56 +0200, Rafał Miłecki wrote: > After d75b1ade567f ("net: less interrupt masking in NAPI") polling > function has to return whole budget when it wants NAPI to call it again. > > Signed-off-by: Rafał Miłecki > Cc: Felix Fietkau > Fixes: eb64e2923a886 ("bgmac: leave interrupts disabled as long as there is > work to do") > --- > drivers/net/ethernet/broadcom/bgmac.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Signed-off-by: Eric Dumazet Thanks ! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: NAPI: Polling function requesting extra calls - difference between 3.18.11 and 4.0.0
On 23 April 2015 at 20:28, Eric Dumazet wrote: > On Thu, 2015-04-23 at 20:18 +0200, Rafał Miłecki wrote: > >> >> Can you help us with this, please? Does bgmac use NAPI incorrectly? >> Were there some important changes in 3.19 or 4.0? >> > > Right they were important changes in NAPI indeed : > > 3b47d30396ba net: gro: add a per device gro flush timer > d75b1ade567f net: less interrupt masking in NAPI > bc9ad166e38a net: introduce napi_schedule_irqoff() As you obviously noticed, I fixed bgmac now and submitted the fix. Just wanted to say a thanks for helping with that! -- Rafał -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch net] igb: pass the correct maxlen for eth_get_headlen()
On Thu, Apr 23, 2015 at 11:40 AM, Alexander Duyck wrote: > On 04/23/2015 11:06 AM, Cong Wang wrote: >> On Wed, Apr 22, 2015 at 8:40 PM, Alexander Duyck >> wrote: >>> On 04/22/2015 04:23 PM, Cong Wang wrote: On Wed, Apr 22, 2015 at 3:34 PM, Alexander Duyck wrote: > On 04/22/2015 02:56 PM, Cong Wang wrote: >> On Wed, Apr 22, 2015 at 2:42 PM, Alexander Duyck >> wrote: >>> On 04/22/2015 01:33 PM, Cong Wang wrote: First, make sure you don't miss the TSIP case right above: The frag starting pointer and its size are advanced by: skb_frag_size_sub(frag, IGB_TS_HDR_LEN); ... va += IGB_TS_HDR_LEN; even though we unlikely pull header longer than IGB_RX_HDR_LEN - IGB_TS_HDR_LEN either. >>> So I believe this is a possible bug, one heck of a corner case to get >>> into though. It requires timestamp in packet, size 240 - 256, and a >>> malformed header. >>> >>> The proper fix would probably be to pull the timestamp out of the packet >>> before we add it to the frame. I'll submit a patch to address this. >>> >> Huh? Doesn't my patch already fix this? skb_frag_size() is always >> up to date. Or you mean another different problem? > Your patch has other issues. I am still NAKing your patch, but there is > an issue with igb that you have pointed out. The proper fix would be to > deal with the timestamp before we add the page fragment to the skb. > If the first frag is always 2K, then this is not a problem either. IGB_TS_HDR_LEN + IGB_RX_HDR_LEN < 2K. >>> The problem is skb->tail will get screwed up. >>> >> Why? We don't copy timestamp into skb header, instead it is saved >> in shared_info, why skb->tail matters here in TSIP case? > > TSIP only matters if you have a network header with bad data in it that > indicates the size is actually larger than it actually is. OK. > >>> The first frag will be 2K in size only if there are multiple frags. The >> >> Or it doesn't have frags at all since we just copy it to skb header, right? > > That is moot since this code only gets called if the skb is nonlinear > >>> problem in the TSIP case is that we will put the size reported by the >>> descriptor into the fragment, and then try to pull the size we >> >> That size is saved when adding the frag, in TSIP case we just sub it >> by IGB_TS_HDR_LEN, which seems correct. >> > > Yes, the size is saved. But with your solution we could pull the whole > fragment but not free it which isn't correct as we shouldn't be left > with any 0 sized fragments. Good point, then TSIP case should check frag_size == 0 case, the rest handles 0-size case correctly, see below. > >>> determined via eth_get_headlen. So there is a window where that value >>> can be wrong and result in data_len going negative and tail being moved >>> beyond the end of the actual data. >> This sounds like a different problem if you are saying we should sub >> the size in the descriptor too. Therefore I don't see why your patch >> could replace mine. > > Your patches sort-of fixed the problem, but they introduced other issues > in the process. > > The thing I don't think you are getting is that the code was meant to be > mutually exclusive w/ the copy-break code. Either the frag is less than > IGB_RX_HDR_SIZE in which case we copy-break and don't attached the > fragment, or we should be pulling the header and leaving at least 1 byte > in the fragment. The problem with your solution is that you potentially > pull the entire fragment in, but you don't free it if the size drops to Only if it is a malformed packet, and it is still safe as long as we don't run over the buffer size. Also I have a upper limit check for IGB_RX_HDR_SIZE after eth_get_headlen(). > 0. That is why in the other mail I told you the better solution for igb I am sure eth_get_headlen() handles 0 case correctly, it simply returns 0. > would likely be to just pass IGB_RX_HDR_SIZE - IGB_TS_HDR_SIZE as that > way you end up with at least 1 byte left in the fragment after you pull > the header. > The code should already handle pull_len==0 correctly. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net: netcp: remove call to netif_carrier_(on/off) for MAC to Phy interface
Currently when interface type is MAC to Phy, netif_carrier_(on/off) is called which is not needed as Phy lib already updates the carrier status to net stack. This is needed only for other interface types Signed-off-by: Murali Karicheri --- drivers/net/ethernet/ti/netcp_ethss.c |8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c index 2bef655..0a1ef2e 100644 --- a/drivers/net/ethernet/ti/netcp_ethss.c +++ b/drivers/net/ethernet/ti/netcp_ethss.c @@ -1765,7 +1765,9 @@ static void netcp_ethss_link_state_action(struct gbe_priv *gbe_dev, ALE_PORT_STATE, ALE_PORT_STATE_FORWARD); - if (ndev && slave->open) + if (ndev && slave->open && + ((slave->link_interface != SGMII_LINK_MAC_PHY) && + (slave->link_interface != XGMII_LINK_MAC_PHY))) netif_carrier_on(ndev); } else { writel(mac_control, GBE_REG_ADDR(slave, emac_regs, @@ -1773,7 +1775,9 @@ static void netcp_ethss_link_state_action(struct gbe_priv *gbe_dev, cpsw_ale_control_set(gbe_dev->ale, slave->port_num, ALE_PORT_STATE, ALE_PORT_STATE_DISABLE); - if (ndev) + if (ndev && + ((slave->link_interface != SGMII_LINK_MAC_PHY) && + (slave->link_interface != XGMII_LINK_MAC_PHY))) netif_carrier_off(ndev); } -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: NAPI: Polling function requesting extra calls - difference between 3.18.11 and 4.0.0
On Thu, 2015-04-23 at 21:08 +0200, Rafał Miłecki wrote: > As you obviously noticed, I fixed bgmac now and submitted the fix. > > Just wanted to say a thanks for helping with that! > Sure, it was a pleasure ;) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch net] igb: pass the correct maxlen for eth_get_headlen()
(Off-topic...) On Wed, Apr 22, 2015 at 4:23 PM, Cong Wang wrote: > > The code looks correct to me now, except it is suspicious skb->len > is not updated after skb_copy_to_linear_data() while skb->tail is > advanced already. I need to think more before submitting a patch. I feel like we need the following patch, maybe skb->len is updated somewhere else by "skb->tail - skb->head", otherwise we are screwed? diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index a0a9b1f..66e6fb6 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -6843,7 +6843,6 @@ static void igb_pull_tail(struct igb_ring *rx_ring, skb_frag_size_sub(frag, IGB_TS_HDR_LEN); frag->page_offset += IGB_TS_HDR_LEN; skb->data_len -= IGB_TS_HDR_LEN; - skb->len -= IGB_TS_HDR_LEN; /* move va to start of packet data */ va += IGB_TS_HDR_LEN; @@ -6856,12 +6855,12 @@ static void igb_pull_tail(struct igb_ring *rx_ring, /* align pull length to size of long to optimize memcpy performance */ skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long))); + __skb_put(skb, pull_len); /* update all of the pointers */ skb_frag_size_sub(frag, pull_len); frag->page_offset += pull_len; skb->data_len -= pull_len; - skb->tail += pull_len; } /** -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Intel-wired-lan] [Patch net] igb: pass the correct maxlen for eth_get_headlen()
On 04/23/2015 12:30 PM, Cong Wang wrote: (Off-topic...) On Wed, Apr 22, 2015 at 4:23 PM, Cong Wang wrote: The code looks correct to me now, except it is suspicious skb->len is not updated after skb_copy_to_linear_data() while skb->tail is advanced already. I need to think more before submitting a patch. I feel like we need the following patch, maybe skb->len is updated somewhere else by "skb->tail - skb->head", otherwise we are screwed? Maybe in skb_add_rx_frag? You might take a look at it. diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index a0a9b1f..66e6fb6 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -6843,7 +6843,6 @@ static void igb_pull_tail(struct igb_ring *rx_ring, skb_frag_size_sub(frag, IGB_TS_HDR_LEN); frag->page_offset += IGB_TS_HDR_LEN; skb->data_len -= IGB_TS_HDR_LEN; - skb->len -= IGB_TS_HDR_LEN; /* move va to start of packet data */ va += IGB_TS_HDR_LEN; @@ -6856,12 +6855,12 @@ static void igb_pull_tail(struct igb_ring *rx_ring, /* align pull length to size of long to optimize memcpy performance */ skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long))); + __skb_put(skb, pull_len); /* update all of the pointers */ skb_frag_size_sub(frag, pull_len); frag->page_offset += pull_len; skb->data_len -= pull_len; - skb->tail += pull_len; } /** Seriously? Are you even reading the code? The fragment is already a part of the skb, as such it is already included in skb->len. By re-adding the header you are adding bytes that aren't there. All we were doing is moving data from a fragment to the linear portion. No offense but your starting to waste my time with these silly patch ideas. The patches I submitted to intel-wired-lan fix the issue that you found, and there aren't any new issues that it creates so the issue is resolved. And like I said if you need to fix this in stable just subtract IGB_TS_HDR_LEN from the header length scanned in eth_get_headlen and it will resolve the issue you reported and that way we can fix the issue and avoid pulling a fragment down to size 0. - Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On 22.04.2015 00:27, Joe Perches wrote: > On Tue, 2015-04-21 at 23:44 +0200, Mateusz Kulikowski wrote: >> On 21.04.2015 23:22, Joe Perches wrote: >>> On Tue, 2015-04-21 at 22:57 +0200, Mateusz Kulikowski wrote: >> (...) (...) >> True, True; If you prefer $line and ability to --fix - I'll use that in v3 > > I suppose you could do both $line and $stat > and the fix would only work when it's on a > single line. > > Perhaps something like this would work: > > if ($line =~ /whatever/ || > (defined($stat) && $stat =~ /whatever/)) { > if (WARN(...) && > $fix) { > fixed[$fixlinenr] =~ s/whatever/appropriate/; > } > } Isn't it enough to just match $stat and do fix for line (that in some cases will just not match)? One more thing I noticed funny behavior about $stat matches - it reports the same error several times (including as "scope" whole file) Is it feature or "feature" or I missed something? Ex. file: -- cut int foo(void) { baz(); memset(a, b, 0); bar(); } -- cut Output of (@master) -- cut $ scripts/checkpatch.pl -f test42.c --types MEMSET ERROR: memset to 0's uses 0 as the 2nd argument, not the 3rd #1: FILE: test42.c:1: +int foo(void) { baz(); memset(a, b, 0); bar(); } ERROR: memset to 0's uses 0 as the 2nd argument, not the 3rd #2: FILE: test42.c:2: +{ baz(); memset(a, b, 0); bar(); } ERROR: memset to 0's uses 0 as the 2nd argument, not the 3rd #4: FILE: test42.c:4: +memset(a, b, 0); total: 3 errors, 0 warnings, 6 lines checked NOTE: Used message types: MEMSET test42.c has style problems, please review. If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. --cut Regards, Mateusz -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rhashtable: don't attempt to grow when at max_size
On 04/23/15 at 04:38pm, Johannes Berg wrote: > From: Johannes Berg > > The conversion of mac80211's station table to rhashtable had a bug > that I found by accident in code review, that hadn't been found as > rhashtable apparently managed to have a maximum hash chain length > of one (!) in all our testing. This is the desired chain length ;-) > In order to test the bug and verify the fix I set my rhashtable's > max_size very low (4) in order to force getting hash collisions. > > At that point, rhashtable WARNed in rhashtable_insert_rehash() but > didn't actually reject the hash table insertion. This caused it to > lose insertions - my master list of stations would have 9 entries, > but the rhashtable only had 5. This may warrant a deeper look, but > that WARN_ON() just shouldn't happen. The warning got fixed recently (51bb8e331b) and rhashtable_insert_rehash() now only allows a single rehash if at max_size already. It will now return -EBUSY. Insertions may still fail while the table is above 100% utilization so this fix is absolutely needed though. > Fix this by not returning true from rht_grow_above_100() when the > rhashtable's max_size has been reached - in this case the user is > explicitly configuring it to be at most that big, so even if it's > now above 100% it shouldn't attempt to resize. Good catch. I wonder whether we want to trigger a periodic rehash in an interval in this situation or just leave this up to the user to setup a timer himself. > This fixes the "lost insertion" issue and consequently allows my > code to display its error (and verify my fix for it.) > > Signed-off-by: Johannes Berg Acked-by: Thomas Graf -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On 04/22/15 18:22, Alexei Starovoitov wrote: On 4/21/15 10:02 PM, Cong Wang wrote: On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov wrote: TC_ACT_QUEUED was always an alias of TC_ACT_STOLEN. Get rid of redundant checks in all qdiscs. Instead do it once. The current code can be easily extended, while your code not. I don't see the need of this change. well, iproute2 doesn't use TC_ACT_QUEUED action at all and TC_ACT_STOLEN is used by mirred. All in-tree qdiscs alias them. If you're saying that some future actions together with some future qdiscs may take advantage of that, then why they didn't use it over the last 10 years? Having both that do the same thing is only confusing. I think having one value to indicate 'stolen' condition makes TC code easier to understand. Jamal, what's your take on this? Sorry - I am on travel so not very helpful/responsive. Several things: 1) the _XMIT semantics are useful on the egress side because in fact we do have queues and they can be attached to qdiscs etc. The TC_ACT_XXX codes were _intentional_ since ingress works as a classifier shell. The fact that qdiscs dealt with these codes directly allows for specialized handling. Moving them to a generic function seems to defeat that purpose. So I am siding with Cong on this. You probably have some other motivation for doing this which is not clear yet? Are you planning to queue things on ingress? 2) the ACT_QUEUED vs STOLEN was supposed to have semantics of something that was stolen (eg redirection should definetely have been returning STOLEN not QUEUED); something that queues for later re-injection (with any/all metadata) was intended to use QUEUED. I believe netfilter may have followed suit and introduced similar codes (so it would be interesting to see how they use them). In any case, it is clear it is not being used - but i want to point the reason it was there (which may still be useful; problem is TheLinuxWay is cutnpaste where someone repeats the code that exists). cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rhashtable: don't attempt to grow when at max_size
On Thu, 2015-04-23 at 21:46 +0100, Thomas Graf wrote: > On 04/23/15 at 04:38pm, Johannes Berg wrote: > > From: Johannes Berg > > > > The conversion of mac80211's station table to rhashtable had a bug > > that I found by accident in code review, that hadn't been found as > > rhashtable apparently managed to have a maximum hash chain length > > of one (!) in all our testing. > > This is the desired chain length ;-) Sure. But I had a bug in my handling of collisions, so I explicitly wanted to test them. After all, they are in some ways expected in a hash table :) > > At that point, rhashtable WARNed in rhashtable_insert_rehash() but > > didn't actually reject the hash table insertion. This caused it to > > lose insertions - my master list of stations would have 9 entries, > > but the rhashtable only had 5. This may warrant a deeper look, but > > that WARN_ON() just shouldn't happen. > > The warning got fixed recently (51bb8e331b) and > rhashtable_insert_rehash() now only allows a single rehash if at > max_size already. It will now return -EBUSY. > > Insertions may still fail while the table is above 100% utilization > so this fix is absolutely needed though. Yeah just failing would be a bit strange. > > Fix this by not returning true from rht_grow_above_100() when the > > rhashtable's max_size has been reached - in this case the user is > > explicitly configuring it to be at most that big, so even if it's > > now above 100% it shouldn't attempt to resize. > > Good catch. I wonder whether we want to trigger a periodic rehash > in an interval in this situation or just leave this up to the user > to setup a timer himself. You could just document it that it's probably useful if max_size is set? I'm just going to be setting max_size for debug purposes, so don't really care all that much. johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
Jamal Hadi Salim wrote: > 2) the ACT_QUEUED vs STOLEN was supposed to have semantics of something > that was stolen (eg redirection should definetely have been returning > STOLEN not QUEUED); something that queues for later re-injection > (with any/all metadata) was intended to use QUEUED. I believe netfilter > may have followed suit and introduced similar codes (so it would be > interesting to see how they use them). Hooks (Targets) don't queue themselves, i.e. NF_QUEUE tells the netfilter core that the skb is to be handed off to nf_queue machinery, while NF_STOLEN is the more obvious "don't touch this skb ever again". -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Intel-wired-lan] [Patch net] igb: pass the correct maxlen for eth_get_headlen()
On Thu, Apr 23, 2015 at 12:45 PM, Alexander Duyck wrote: > On 04/23/2015 12:30 PM, Cong Wang wrote: >> >> (Off-topic...) >> >> On Wed, Apr 22, 2015 at 4:23 PM, Cong Wang wrote: >>> >>> The code looks correct to me now, except it is suspicious skb->len >>> is not updated after skb_copy_to_linear_data() while skb->tail is >>> advanced already. I need to think more before submitting a patch. >> >> I feel like we need the following patch, maybe skb->len is updated >> somewhere >> else by "skb->tail - skb->head", otherwise we are screwed? > > > Maybe in skb_add_rx_frag? You might take a look at it. I saw that. > >> >> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c >> b/drivers/net/ethernet/intel/igb/igb_main.c >> index a0a9b1f..66e6fb6 100644 >> --- a/drivers/net/ethernet/intel/igb/igb_main.c >> +++ b/drivers/net/ethernet/intel/igb/igb_main.c >> @@ -6843,7 +6843,6 @@ static void igb_pull_tail(struct igb_ring *rx_ring, >> skb_frag_size_sub(frag, IGB_TS_HDR_LEN); >> frag->page_offset += IGB_TS_HDR_LEN; >> skb->data_len -= IGB_TS_HDR_LEN; >> - skb->len -= IGB_TS_HDR_LEN; >> >> /* move va to start of packet data */ >> va += IGB_TS_HDR_LEN; >> @@ -6856,12 +6855,12 @@ static void igb_pull_tail(struct igb_ring >> *rx_ring, >> >> /* align pull length to size of long to optimize memcpy >> performance */ >> skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long))); >> + __skb_put(skb, pull_len); >> >> /* update all of the pointers */ >> skb_frag_size_sub(frag, pull_len); >> frag->page_offset += pull_len; >> skb->data_len -= pull_len; >> - skb->tail += pull_len; >> } >> >> /** > > > Seriously? Are you even reading the code? The fragment is already a part > of the skb, as such it is already included in skb->len. By re-adding the > header you are adding bytes that aren't there. All we were doing is moving > data from a fragment to the linear portion. Hmm, I thought frags are counted only in skb->data_len, I misunderstood skb_headlen() and skb_pagelen(). > > No offense but your starting to waste my time with these silly patch ideas. You can always ignore any off-topic discussion, which I already warned you in the beginning... > The patches I submitted to intel-wired-lan fix the issue that you found, and > there aren't any new issues that it creates so the issue is resolved. And > like I said if you need to fix this in stable just subtract IGB_TS_HDR_LEN > from the header length scanned in eth_get_headlen and it will resolve the > issue you reported and that way we can fix the issue and avoid pulling a > fragment down to size 0. > Please keep non-off-topic discussion from off-topic ones. I will reply there. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On 4/23/15 1:45 PM, Jamal Hadi Salim wrote: 1) the _XMIT semantics are useful on the egress side because in fact we do have queues and they can be attached to qdiscs etc. The TC_ACT_XXX codes were _intentional_ since ingress works as a classifier shell. then it is worse mess than I thought :( Why call it _qdisc_ then? and have special and convoluted handling for it in qdisc_create, qdisc_graft and other places? > Are you planning to queue things on ingress? I thought that was the whole purpose of ingress qdisc. why then we have dev->ingress_queue? If queueing was never a goal, may be we should kill ingress qdisc and replace it with a simple shim that only does cls/act. The code overall will get much simpler and faster. Feels like falling into rabbit hole. The fact that qdiscs dealt with these codes directly allows for specialized handling. Moving them to a generic function seems to defeat that purpose. So I am siding with Cong on this. that's not what patch 1 is doing. It is still doing specialized handling... but in light of what you said above, it looks like much bigger cleanup is needed. We'll continue arguing when I refactor this set and resubmit when net-next reopens. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On Thu, Apr 23, 2015 at 3:13 PM, Alexei Starovoitov wrote: > On 4/23/15 1:45 PM, Jamal Hadi Salim wrote: >> >> >> 1) the _XMIT semantics are useful on the egress side because in fact >> we do have queues and they can be attached to qdiscs etc. >> The TC_ACT_XXX codes were _intentional_ since ingress works as a >> classifier shell. > > > then it is worse mess than I thought :( > Why call it _qdisc_ then? and have special and convoluted handling for > it in qdisc_create, qdisc_graft and other places? It needs to glue into qdisc layer, unify the abstractions. > >> Are you planning to queue things on ingress? > > I thought that was the whole purpose of ingress qdisc. There is no queue for ingress qdisc. > why then we have dev->ingress_queue? Glue layer, qdisc ties too much with txq, which is what I want to solve. We have struct netdev_rx_queue too, but look at it, there is nothing but some sysfs stuffs needed by RPS. > > If queueing was never a goal, may be we should kill ingress qdisc > and replace it with a simple shim that only does cls/act. > The code overall will get much simpler and faster. > As I said in response to your patch for skb->data, we need to align ingress with egress, rather than making more differences, this includes qdisc's too. We do have per-cpu queues for ingress, at least in theory we have a queue for ingress to use. Of course, RX is naturally different from TX, which is the root cause why we have so many differences here. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On 04/23/15 18:13, Alexei Starovoitov wrote: On 4/23/15 1:45 PM, Jamal Hadi Salim wrote: then it is worse mess than I thought :( Why call it _qdisc_ then? and have special and convoluted handling for it in qdisc_create, qdisc_graft and other places? Convinience for tooling and using existing abstractions is the historical. You can attach qdiscs to netdevs. You can then use all sorts of well understood tools. > Are you planning to queue things on ingress? I thought that was the whole purpose of ingress qdisc. why then we have dev->ingress_queue? So you are planning to add queues? If you are that is a different discussion (and the use case needs some clarity). If queueing was never a goal, may be we should kill ingress qdisc and replace it with a simple shim that only does cls/act. The code overall will get much simpler and faster. Attaching to the device was considered "simpler" based on the tooling thought process. On whether we should queue on ingress - it was not considered useful if the packets were going to the host i.e we want to proceed to completion likely queueing to some socket layer further upstream. For packets being forwarded we already had egress qdiscs which had queues so it didnt seem to make sense to enqueue on ingress for such use cases. There was a use case later where multiple ingress ports had to be shared and ifb was born - which is pseudo temporary enqueueing on ingress. Feels like falling into rabbit hole. The fact that qdiscs dealt with these codes directly allows for specialized handling. Moving them to a generic function seems to defeat that purpose. So I am siding with Cong on this. that's not what patch 1 is doing. It is still doing specialized handling... but in light of what you said above, it looks like much bigger cleanup is needed. We'll continue arguing when I refactor this set and resubmit when net-next reopens. Sorry - i was refereing to the patch where you agregated things after the qdisc invokes a classifier. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Suspicious RCU usage in bridge with Linux v4.0-9362-g1fc149933fd4
On Thu, Apr 23, 2015 at 10:07 AM, Josh Boyer wrote: > [ 29.382235] br0: port 1(tap0) entered forwarding state > > [ 29.382286] === > [ 29.382315] [ INFO: suspicious RCU usage. ] > [ 29.382344] 4.1.0-0.rc0.git11.1.fc23.x86_64 #1 Not tainted > [ 29.382380] --- > [ 29.382409] net/bridge/br_private.h:626 suspicious > rcu_dereference_check() usage! > [ 29.382455] >other info that might help us debug this: > > [ 29.382507] >rcu_scheduler_active = 1, debug_locks = 0 > [ 29.382549] 2 locks held by swapper/0/0: > [ 29.382576] #0: (((&p->forward_delay_timer))){+.-...}, at: > [] call_timer_fn+0x5/0x4f0 > [ 29.382660] #1: (&(&br->lock)->rlock){+.-...}, at: > [] br_forward_delay_timer_expired+0x31/0x140 > [bridge] > [ 29.382754] >stack backtrace: > [ 29.382787] CPU: 0 PID: 0 Comm: swapper/0 Not tainted > 4.1.0-0.rc0.git11.1.fc23.x86_64 #1 > [ 29.382838] Hardware name: LENOVO 422916G/LENOVO, BIOS A1KT53AUS 04/07/2015 > [ 29.382882] 3ebfc20364115825 88003c48 > 81892d4b > [ 29.382943] 81e124e0 88003c78 > 8110bcd7 > [ 29.383004] 8800785c9d00 88065485ac58 880c62002800 > 880c5fc88ac0 > [ 29.383065] Call Trace: > [ 29.383084][] dump_stack+0x4c/0x65 > [ 29.383130] [] lockdep_rcu_suspicious+0xe7/0x120 > [ 29.383178] [] br_fill_ifinfo+0x4a9/0x6a0 [bridge] > [ 29.383225] [] br_ifinfo_notify+0x11b/0x4b0 [bridge] > [ 29.383271] [] ? br_hold_timer_expired+0x70/0x70 > [bridge] > [ 29.383320] [] > br_forward_delay_timer_expired+0x58/0x140 [bridge] > [ 29.383371] [] ? br_hold_timer_expired+0x70/0x70 > [bridge] > [ 29.383416] [] call_timer_fn+0xc3/0x4f0 > [ 29.383454] [] ? call_timer_fn+0x5/0x4f0 > [ 29.383493] [] ? lock_release_holdtime.part.29+0xf/0x200 > [ 29.383541] [] ? br_hold_timer_expired+0x70/0x70 > [bridge] > [ 29.383587] [] run_timer_softirq+0x244/0x490 > [ 29.383629] [] __do_softirq+0xec/0x670 > [ 29.383666] [] irq_exit+0x145/0x150 > [ 29.383703] [] smp_apic_timer_interrupt+0x46/0x60 > [ 29.383744] [] apic_timer_interrupt+0x73/0x80 > [ 29.383782][] ? cpuidle_enter_state+0x5f/0x2f0 > [ 29.383832] [] ? cpuidle_enter_state+0x5b/0x2f0 > [ 29.383873] [] cpuidle_enter+0x17/0x20 > [ 29.383908] [] cpu_startup_entry+0x36f/0x5f0 > [ 29.383949] [] rest_init+0x13d/0x150 > [ 29.383986] [] start_kernel+0x4d2/0x4f3 > [ 29.384023] [] ? early_idt_handlers+0x120/0x120 > [ 29.384064] [] x86_64_start_reservations+0x2a/0x2c > [ 29.384105] [] x86_64_start_kernel+0x14a/0x16d We are dereferencing a RCU pointer with rtnl assert, but in the timer context we only have br->lock. It looks like we need to take RCU read lock on that path rather than asserting rtnl lock. Thanks for the report. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH next 2/3] ipvlan: Process fragmented multicast frames correctly
Multicast processing in IPvlan was faulty as is. Eric Dumazet pointed out that fragmented packets wont be processed correctly unless defrag step is introduced. This patch adds the defrag step before driver attempts to process multicast frame(s). Signed-off-by: Mahesh Bandewar --- drivers/net/ipvlan/ipvlan_core.c | 8 include/net/ip.h | 1 + 2 files changed, 9 insertions(+) diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c index 58891666088c..282d32b16a87 100644 --- a/drivers/net/ipvlan/ipvlan_core.c +++ b/drivers/net/ipvlan/ipvlan_core.c @@ -532,6 +532,10 @@ static int ipvlan_xmit_mode_l2(struct sk_buff *skb, struct net_device *dev) return dev_forward_skb(ipvlan->phy_dev, skb); } else if (is_multicast_ether_addr(eth->h_dest)) { + skb = ip_check_defrag(skb, IP_DEFRAG_IPVLAN); + if (!skb) + return RX_HANDLER_CONSUMED; + ipvlan_multicast_enqueue(ipvlan->port, skb); return NET_XMIT_SUCCESS; } @@ -617,6 +621,10 @@ static rx_handler_result_t ipvlan_handle_mode_l2(struct sk_buff **pskb, int addr_type; if (is_multicast_ether_addr(eth->h_dest)) { + skb = ip_check_defrag(skb, IP_DEFRAG_IPVLAN); + if (!skb) + return RX_HANDLER_CONSUMED; + if (ipvlan_external_frame(skb, port)) { ipvlan_multicast_enqueue(port, skb); return RX_HANDLER_CONSUMED; diff --git a/include/net/ip.h b/include/net/ip.h index d14af7edd197..1fb432b346a8 100644 --- a/include/net/ip.h +++ b/include/net/ip.h @@ -476,6 +476,7 @@ enum ip_defrag_users { IP_DEFRAG_VS_FWD, IP_DEFRAG_AF_PACKET, IP_DEFRAG_MACVLAN, + IP_DEFRAG_IPVLAN, }; int ip_defrag(struct sk_buff *skb, u32 user); -- 2.2.0.rc0.207.ga3a616c -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH next 1/3] ipvlan: Defer multicast / broadcast processing to a work-queue
Processing multicast / broadcast in fast path is performance draining and having more links means more clonning and bringing performance down further. Broadcast; in particular, need to be given to all the virtual links. Earlier tricks of enabling broadcast bit for IPv4 only interfaces are not really working since it fails autoconf. Which means enabling braodcast for all the links if protocol specific hacks do not have to be added into the driver. This patch defers all (incoming as well as outgoing) multicast traffic to a work-queue leaving only the unicast traffic in the fast-path. Now if we need to apply any additional tricks to further reduce the impact of this (multicast / broadcast) type of traffic, it can be implemented while processing this work without affecting the fast-path. Signed-off-by: Mahesh Bandewar --- drivers/net/ipvlan/ipvlan.h | 5 ++ drivers/net/ipvlan/ipvlan_core.c | 134 +-- drivers/net/ipvlan/ipvlan_main.c | 5 ++ 3 files changed, 96 insertions(+), 48 deletions(-) diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h index 54549a6223dd..953a97492fab 100644 --- a/drivers/net/ipvlan/ipvlan.h +++ b/drivers/net/ipvlan/ipvlan.h @@ -39,6 +39,8 @@ #define IPVLAN_MAC_FILTER_SIZE (1 << IPVLAN_MAC_FILTER_BITS) #define IPVLAN_MAC_FILTER_MASK (IPVLAN_MAC_FILTER_SIZE - 1) +#define IPVLAN_QBACKLOG_LIMIT 1000 + typedef enum { IPVL_IPV6 = 0, IPVL_ICMPV6, @@ -93,6 +95,8 @@ struct ipvl_port { struct hlist_head hlhead[IPVLAN_HASH_SIZE]; struct list_headipvlans; struct rcu_head rcu; + struct work_struct wq; + struct sk_buff_head backlog; int count; u16 mode; }; @@ -112,6 +116,7 @@ void ipvlan_set_port_mode(struct ipvl_port *port, u32 nval); void ipvlan_init_secret(void); unsigned int ipvlan_mac_hash(const unsigned char *addr); rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb); +void ipvlan_process_multicast(struct work_struct *work); int ipvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev); void ipvlan_ht_addr_add(struct ipvl_dev *ipvlan, struct ipvl_addr *addr); struct ipvl_addr *ipvlan_find_addr(const struct ipvl_dev *ipvlan, diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c index c30b5c300c05..58891666088c 100644 --- a/drivers/net/ipvlan/ipvlan_core.c +++ b/drivers/net/ipvlan/ipvlan_core.c @@ -189,64 +189,85 @@ unsigned int ipvlan_mac_hash(const unsigned char *addr) return hash & IPVLAN_MAC_FILTER_MASK; } -static void ipvlan_multicast_frame(struct ipvl_port *port, struct sk_buff *skb, - const struct ipvl_dev *in_dev, bool local) +void ipvlan_process_multicast(struct work_struct *work) { - struct ethhdr *eth = eth_hdr(skb); + struct ipvl_port *port = container_of(work, struct ipvl_port, wq); + struct ethhdr *ethh; struct ipvl_dev *ipvlan; - struct sk_buff *nskb; + struct sk_buff *skb, *nskb; + struct sk_buff_head list; unsigned int len; unsigned int mac_hash; int ret; + u8 pkt_type; + bool hlocal, dlocal; - if (skb->protocol == htons(ETH_P_PAUSE)) - return; - - rcu_read_lock(); - list_for_each_entry_rcu(ipvlan, &port->ipvlans, pnode) { - if (local && (ipvlan == in_dev)) - continue; + __skb_queue_head_init(&list); - mac_hash = ipvlan_mac_hash(eth->h_dest); - if (!test_bit(mac_hash, ipvlan->mac_filters)) - continue; + spin_lock_bh(&port->backlog.lock); + skb_queue_splice_tail_init(&port->backlog, &list); + spin_unlock_bh(&port->backlog.lock); - ret = NET_RX_DROP; - len = skb->len + ETH_HLEN; - nskb = skb_clone(skb, GFP_ATOMIC); - if (!nskb) - goto mcast_acct; + while ((skb = __skb_dequeue(&list)) != NULL) { + ethh = eth_hdr(skb); + hlocal = ether_addr_equal(ethh->h_source, port->dev->dev_addr); + mac_hash = ipvlan_mac_hash(ethh->h_dest); - if (ether_addr_equal(eth->h_dest, ipvlan->phy_dev->broadcast)) - nskb->pkt_type = PACKET_BROADCAST; + if (ether_addr_equal(ethh->h_dest, port->dev->broadcast)) + pkt_type = PACKET_BROADCAST; else - nskb->pkt_type = PACKET_MULTICAST; + pkt_type = PACKET_MULTICAST; + + dlocal = false; + rcu_read_lock(); + list_for_each_entry_rcu(ipvlan, &port->ipvlans, pnode) { + if (hlocal && (ipvlan->dev == skb->dev)) { + dlocal = true; + continue; +
[PATCH next 3/3] ipvlan: Always set broadcast bit in multicast filter
Earlier tricks of setting broadcast bit only when IPv4 address is added onto interface are not good enough especially when autoconf comes in play. Setting them on always is performance drag but now that multicast / broadcast is not processed in fast-path; enabling broadcast will let autoconf work correctly without affecting performance characteristics of the device. Signed-off-by: Mahesh Bandewar --- drivers/net/ipvlan/ipvlan_main.c | 20 ++-- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c index a16d3017fdc3..8fe9d5ff331e 100644 --- a/drivers/net/ipvlan/ipvlan_main.c +++ b/drivers/net/ipvlan/ipvlan_main.c @@ -218,17 +218,6 @@ static void ipvlan_change_rx_flags(struct net_device *dev, int change) dev_set_allmulti(phy_dev, dev->flags & IFF_ALLMULTI? 1 : -1); } -static void ipvlan_set_broadcast_mac_filter(struct ipvl_dev *ipvlan, bool set) -{ - struct net_device *dev = ipvlan->dev; - unsigned int hashbit = ipvlan_mac_hash(dev->broadcast); - - if (set && !test_bit(hashbit, ipvlan->mac_filters)) - __set_bit(hashbit, ipvlan->mac_filters); - else if (!set && test_bit(hashbit, ipvlan->mac_filters)) - __clear_bit(hashbit, ipvlan->mac_filters); -} - static void ipvlan_set_multicast_mac_filter(struct net_device *dev) { struct ipvl_dev *ipvlan = netdev_priv(dev); @@ -243,6 +232,12 @@ static void ipvlan_set_multicast_mac_filter(struct net_device *dev) netdev_for_each_mc_addr(ha, dev) __set_bit(ipvlan_mac_hash(ha->addr), mc_filters); + /* Turn-on broadcast bit irrespective of address family, +* since broadcast is deferred to a work-queue, hence no +* impact on unicast fast-path processing. +*/ + __set_bit(ipvlan_mac_hash(dev->broadcast), mc_filters); + bitmap_copy(ipvlan->mac_filters, mc_filters, IPVLAN_MAC_FILTER_SIZE); } @@ -710,7 +705,6 @@ static int ipvlan_add_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr) */ if (netif_running(ipvlan->dev)) ipvlan_ht_addr_add(ipvlan, addr); - ipvlan_set_broadcast_mac_filter(ipvlan, true); return 0; } @@ -727,8 +721,6 @@ static void ipvlan_del_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr) list_del(&addr->anode); ipvlan->ipv4cnt--; WARN_ON(ipvlan->ipv4cnt < 0); - if (!ipvlan->ipv4cnt) - ipvlan_set_broadcast_mac_filter(ipvlan, false); kfree_rcu(addr, rcu); return; -- 2.2.0.rc0.207.ga3a616c -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
On Thu, Apr 23, 2015 at 03:24:59PM +0200, Martin Willi wrote: > > Do you have any pointer for me where this is defined? Why is it needed, > given that GCM implicitly authenticates the IV by using it in Y0? The IV if present must be covered by the ICV. This is required by RFC4303 (section 2). But really it's quite obvious. If you don't authenticate the IV, then I can easily inject random crap into your network by changing the IV. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
On Thu, Apr 23, 2015 at 01:45:34PM +0200, Steffen Klassert wrote: > > Adding a second template for the correct implementation is > probaply the only thing that we can do if we don't want to > break backwards compatibility. But maybe we can add a warning > to the old implementation, such that users notice that they > use a broken version. If we are going to do a warning I think the place to do it would be in xfrm_algo.c. We could add an insecure/warning flag and if then print a warning if said algorithm is used. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Resource usages in Linux drivers
Sergei Shtylyov : > On 4/23/2015 1:08 PM, Jia-Ju Bai wrote: [...] > >I also find many drivers do not use these managed APIs, especially in > >ethernet > >card drivers (like e100, r8169). Is it possible to change them? > >Patches welcome. :-) I respectfully disagree. If someone believes basic resouce management to be too hard or error-prone to handle, he should imvho seriously rise the bar and reconsider the way he wants to contribute to the kernel. I may hope he who reads e100.c to think about DMA api, bql or rx ring holes avoidance to quote a few ones. Managed API ? Mildly... -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On Thu, 2015-04-23 at 21:53 +0200, Mateusz Kulikowski wrote: > On 22.04.2015 00:27, Joe Perches wrote: > > On Tue, 2015-04-21 at 23:44 +0200, Mateusz Kulikowski wrote: > >> On 21.04.2015 23:22, Joe Perches wrote: > >>> On Tue, 2015-04-21 at 22:57 +0200, Mateusz Kulikowski wrote: > >> (...) > (...) > >> True, True; If you prefer $line and ability to --fix - I'll use that in v3 > > > > I suppose you could do both $line and $stat > > and the fix would only work when it's on a > > single line. > > > > Perhaps something like this would work: > > > > if ($line =~ /whatever/ || > > (defined($stat) && $stat =~ /whatever/)) { > > if (WARN(...) && > > $fix) { > > fixed[$fixlinenr] =~ s/whatever/appropriate/; > > } > > } > > Isn't it enough to just match $stat and do fix for line (that in > some cases will just not match)? Yeah, that'd work too. > One more thing > I noticed funny behavior about $stat matches - > it reports the same error several times (including as "scope" whole file) > Is it feature or "feature" or I missed something? You have to make sure the first character of $stat is a + if ($stat =~ /\+(?:.*)\bmem -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On Thu, 2015-04-23 at 16:54 -0700, Joe Perches wrote: > On Thu, 2015-04-23 at 21:53 +0200, Mateusz Kulikowski wrote: > > I noticed funny behavior about $stat matches - > > it reports the same error several times (including as "scope" whole file) > > Is it feature or "feature" or I missed something? > > You have to make sure the first character of $stat is a + > > if ($stat =~ /\+(?:.*)\bmem Make that if ($stat =~ /^\+(?:.*)\bmem -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH next 1/3] ipvlan: Defer multicast / broadcast processing to a work-queue
On Thu, 2015-04-23 at 14:29 -0700, Mahesh Bandewar wrote: > +static void ipvlan_multicast_enqueue(struct ipvl_port *port, > + struct sk_buff *skb) > +{ > + if (skb->protocol == htons(ETH_P_PAUSE)) > + return; But what happens to this packet ? It seems leaked ? > + > + spin_lock(&port->backlog.lock); > + if (skb_queue_len(&port->backlog) < IPVLAN_QBACKLOG_LIMIT) { > + __skb_queue_tail(&port->backlog, skb); > + spin_unlock(&port->backlog.lock); > + } else { > + spin_unlock(&port->backlog.lock); > + atomic_long_inc(&skb->dev->rx_dropped); > + kfree_skb(skb); > + } > + schedule_work(&port->wq); No point calling schedule_work(&port->wq); if packet was dropped. We are under pressure, so don't add extra cpu cycles ;) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Resource usages in Linux drivers
On 23/04/15 16:19, Francois Romieu wrote: > Sergei Shtylyov : >> On 4/23/2015 1:08 PM, Jia-Ju Bai wrote: > [...] >>> I also find many drivers do not use these managed APIs, especially in >>> ethernet >>> card drivers (like e100, r8169). Is it possible to change them? >> >>Patches welcome. :-) > > I respectfully disagree. Me too, most of the device managed conversions we have seen were bogus because they were done in a semi-automated way without understanding the peculiarities of the network devices, that is the separation between init/open/close that most other device drivers do not have. A typical example is this: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6892b41d9701283085b655c6086fb57a5d63fa47 > > If someone believes basic resouce management to be too hard or error-prone > to handle, he should imvho seriously rise the bar and reconsider the way > he wants to contribute to the kernel. Well, for one, we could have a device managed register_netdev() which cleans up resources in case of failures and calls free_netdev() automatically, but is that adding much value? > > I may hope he who reads e100.c to think about DMA api, bql or rx ring > holes avoidance to quote a few ones. Managed API ? Mildly... > -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Resource usages in Linux drivers
From: Florian Fainelli Date: Thu, 23 Apr 2015 17:40:12 -0700 > Well, for one, we could have a device managed register_netdev() > which cleans up resources in case of failures and calls > free_netdev() automatically, but is that adding much value? I don't think it's worth it at all. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
rhashtable: Add cap on number of elements in hash table
On Thu, Apr 23, 2015 at 11:59:19AM -0400, David Miller wrote: > From: Johannes Berg > Date: Thu, 23 Apr 2015 16:38:43 +0200 > > > From: Johannes Berg > > > > The conversion of mac80211's station table to rhashtable had a bug > > that I found by accident in code review, that hadn't been found as > > rhashtable apparently managed to have a maximum hash chain length > > of one (!) in all our testing. > > > > In order to test the bug and verify the fix I set my rhashtable's > > max_size very low (4) in order to force getting hash collisions. > > > > At that point, rhashtable WARNed in rhashtable_insert_rehash() but > > didn't actually reject the hash table insertion. This caused it to > > lose insertions - my master list of stations would have 9 entries, > > but the rhashtable only had 5. This may warrant a deeper look, but > > that WARN_ON() just shouldn't happen. > > > > Fix this by not returning true from rht_grow_above_100() when the > > rhashtable's max_size has been reached - in this case the user is > > explicitly configuring it to be at most that big, so even if it's > > now above 100% it shouldn't attempt to resize. > > > > This fixes the "lost insertion" issue and consequently allows my > > code to display its error (and verify my fix for it.) > > > > Signed-off-by: Johannes Berg > > It looks fine to me, but I'll let Herbert and Thomas review this. Thanks for the heads up. It seems that I lost track somewhere along the line. I meant to add an explicit limit on the overall number of entries since that was what users like netlink expected but never got around to doing it. Instead it seems that we're currently relying on the rht_grow_above_100 to protect us. So here is a patch that adds an explicit limit and fixes the problem Johannes reported. ---8<--- We currently have no limit on the number of elements in a hash table. This is very bad especially considering that some rhashtable users had such a limit before the conversion and relied on it for defence against DoS attacks. We already have a maximum hash table size limit but its enforcement is only by luck and results in a nasty WARN_ON. This patch adds a new paramater insecure_max_entries which becomes the cap on the table. If unset it defaults to max_size. If it is also zero it means that there is no cap on the number of elements in the table. However, the table will grow whenever the utilisation hits 100% and if that growth fails, you will get ENOMEM on insertion. As allowing >100% utilisation is potentially dangerous, the name contains the word insecure. Note that the cap is not a hard limit. This is done for performance reasons as enforcing a hard limit will result in use of atomic ops that are heavier than the ones we currently use. The reasoning is that we're only guarding against a gross over- subscription of the table, rather than a small breach of the limit. Reported-by: Johannes Berg Signed-off-by: Herbert Xu diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h index e23d242..95b5a36 100644 --- a/include/linux/rhashtable.h +++ b/include/linux/rhashtable.h @@ -17,6 +17,7 @@ #ifndef _LINUX_RHASHTABLE_H #define _LINUX_RHASHTABLE_H +#include #include #include #include @@ -100,6 +101,7 @@ struct rhashtable; * @key_len: Length of key * @key_offset: Offset of key in struct to be hashed * @head_offset: Offset of rhash_head in struct to be hashed + * @insecure_max_entries: Maximum number of entries (may be exceeded) * @max_size: Maximum size while expanding * @min_size: Minimum size while shrinking * @nulls_base: Base value to generate nulls marker @@ -115,6 +117,7 @@ struct rhashtable_params { size_t key_len; size_t key_offset; size_t head_offset; + unsigned intinsecure_max_entries; unsigned intmax_size; unsigned intmin_size; u32 nulls_base; @@ -282,7 +285,20 @@ static inline bool rht_shrink_below_30(const struct rhashtable *ht, static inline bool rht_grow_above_100(const struct rhashtable *ht, const struct bucket_table *tbl) { - return atomic_read(&ht->nelems) > tbl->size; + return atomic_read(&ht->nelems) > tbl->size && + (!ht->p.max_size || tbl->size < ht->p.max_size); +} + +/** + * rht_grow_above_max - returns true if table is above maximum + * @ht:hash table + * @tbl: current table + */ +static inline bool rht_grow_above_max(const struct rhashtable *ht, + const struct bucket_table *tbl) +{ + return ht->p.insecure_max_entries && + atomic_read(&ht->nelems) >= ht->p.insecure_max_entries; } /* The bucket lock is selected based on the hash and protects mutations @@ -611,6 +627,10 @@ slow_path: goto slow_path; } + err = -E2BIG; + if (u
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On 4/23/15 3:51 PM, Jamal Hadi Salim wrote: So you are planning to add queues? If you are that is a different discussion (and the use case needs some clarity). nope. I wasn't planning to do that. > For packets being forwarded we already had egress qdiscs which had > queues so it didnt seem to make sense to enqueue on ingress for such > use cases. > There was a use case later where multiple ingress ports had to be > shared > and ifb was born - which is pseudo temporary enqueueing on ingress. agree. imo ifb approach is more flexible, since it has full hierarchy of qdiscs. As you're saying above and from the old ifb logs I thought that ifb is _temporary_ and long term plan is to use ingress_queue, but looks like this is not the case. Also not too long ago we decided that we don't want another ingress qdisc. Therefore I think it makes sense to simplify the code and boost performance. I'm not proposing to change tooling, of course. From iproute2 point of view we'll still have ingress qdisc. Right now we're pointlessly allocating memory for it and for ingress_queue, whereas we only need to call cls/act. I'm proposing to kill them and used tcf_proto in net_device instead. Right now to reach cls in critical path on ingress we do: rxq = skb->dev->ingress_queue sch = rxq->qdisc sch->enqueue sch->filter_list with a bunch of 'if' conditions and useless memory accesses in-between. If we add 'ingress_filter_list' directly to net_device, it will be just: skb->dev->ingress_filter_list which is huge performance boost. Code size will shrink as well. iproute2 and all existing tools will work as-is. Looks as win-win to me. The fact that qdiscs dealt with these codes directly allows for specialized handling. Moving them to a generic function seems to defeat that purpose. So I am siding with Cong on this. that's not what patch 1 is doing. It is still doing specialized handling... but in light of what you said above, it looks like much bigger cleanup is needed. We'll continue arguing when I refactor this set and resubmit when net-next reopens. Sorry - i was refereing to the patch where you agregated things after the qdisc invokes a classifier. hmm. There I also didn't convert all qdiscs into single helper. Only those that have exactly the same logic after tc_classify. All other qdiscs have custom handling. No worries, it's hard to review things while traveling. Been there :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net] inet: fix possible panic in reqsk_queue_unlink()
From: Eric Dumazet [ 3897.923145] BUG: unable to handle kernel NULL pointer dereference at 0080 [ 3897.931025] IP: [] reqsk_timer_handler+0x1a6/0x243 There is a race when reqsk_timer_handler() and tcp_check_req() call inet_csk_reqsk_queue_unlink() on the same req at the same time. Before commit fa76ce7328b2 ("inet: get rid of central tcp/dccp listener timer"), listener spinlock was held and race could not happen. To solve this bug, we change reqsk_queue_unlink() to not assume req must be found, and we return a status, to conditionally release a refcount on the request sock. This also means tcp_check_req() in non fastopen case might or not consume req refcount, so tcp_v6_hnd_req() & tcp_v4_hnd_req() have to properly handle this. (Same remark for dccp_check_req() and its callers) inet_csk_reqsk_queue_drop() is now too big to be inlined, as it is called 4 times in tcp and 3 times in dccp. Fixes: fa76ce7328b2 ("inet: get rid of central tcp/dccp listener timer") Signed-off-by: Eric Dumazet Reported-by: Yuchung Cheng --- include/net/inet_connection_sock.h | 20 --- include/net/request_sock.h | 18 -- net/dccp/ipv4.c|3 +- net/dccp/ipv6.c|3 +- net/dccp/minisocks.c |3 -- net/ipv4/inet_connection_sock.c| 34 +++ net/ipv4/tcp_ipv4.c|3 +- net/ipv4/tcp_minisocks.c |7 +++-- net/ipv6/tcp_ipv6.c|3 +- 9 files changed, 48 insertions(+), 46 deletions(-) diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h index 7b5887cd1172..48a815823587 100644 --- a/include/net/inet_connection_sock.h +++ b/include/net/inet_connection_sock.h @@ -279,12 +279,6 @@ static inline void inet_csk_reqsk_queue_add(struct sock *sk, void inet_csk_reqsk_queue_hash_add(struct sock *sk, struct request_sock *req, unsigned long timeout); -static inline void inet_csk_reqsk_queue_removed(struct sock *sk, - struct request_sock *req) -{ - reqsk_queue_removed(&inet_csk(sk)->icsk_accept_queue, req); -} - static inline void inet_csk_reqsk_queue_added(struct sock *sk, const unsigned long timeout) { @@ -306,19 +300,7 @@ static inline int inet_csk_reqsk_queue_is_full(const struct sock *sk) return reqsk_queue_is_full(&inet_csk(sk)->icsk_accept_queue); } -static inline void inet_csk_reqsk_queue_unlink(struct sock *sk, - struct request_sock *req) -{ - reqsk_queue_unlink(&inet_csk(sk)->icsk_accept_queue, req); -} - -static inline void inet_csk_reqsk_queue_drop(struct sock *sk, -struct request_sock *req) -{ - inet_csk_reqsk_queue_unlink(sk, req); - inet_csk_reqsk_queue_removed(sk, req); - reqsk_put(req); -} +void inet_csk_reqsk_queue_drop(struct sock *sk, struct request_sock *req); void inet_csk_destroy_sock(struct sock *sk); void inet_csk_prepare_forced_close(struct sock *sk); diff --git a/include/net/request_sock.h b/include/net/request_sock.h index fe41f3ceb008..9f4265ce8892 100644 --- a/include/net/request_sock.h +++ b/include/net/request_sock.h @@ -212,24 +212,6 @@ static inline int reqsk_queue_empty(struct request_sock_queue *queue) return queue->rskq_accept_head == NULL; } -static inline void reqsk_queue_unlink(struct request_sock_queue *queue, - struct request_sock *req) -{ - struct listen_sock *lopt = queue->listen_opt; - struct request_sock **prev; - - spin_lock(&queue->syn_wait_lock); - - prev = &lopt->syn_table[req->rsk_hash]; - while (*prev != req) - prev = &(*prev)->dl_next; - *prev = req->dl_next; - - spin_unlock(&queue->syn_wait_lock); - if (del_timer(&req->rsk_timer)) - reqsk_put(req); -} - static inline void reqsk_queue_add(struct request_sock_queue *queue, struct request_sock *req, struct sock *parent, diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c index 2b4f21d34df6..ccf4c5629b3c 100644 --- a/net/dccp/ipv4.c +++ b/net/dccp/ipv4.c @@ -453,7 +453,8 @@ static struct sock *dccp_v4_hnd_req(struct sock *sk, struct sk_buff *skb) iph->saddr, iph->daddr); if (req) { nsk = dccp_check_req(sk, skb, req); - reqsk_put(req); + if (!nsk) + reqsk_put(req); return nsk; } nsk = inet_lookup_established(sock_net(sk), &dccp_hashinfo, diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c index 9d0551092c6c..5165571f397a 100644 --- a/net/dccp/ipv6.c +++ b/net/dccp/ipv6.c @@ -301,7 +301,8 @@ static struct sock *dccp_v6_hnd_r
Re: [PATCH v3] ethernet: myri10ge: use arch_phys_wc_add()
On Thu, Apr 23, 2015 at 02:28:40PM -0400, David Miller wrote: > I can't wait forever for the driver maintainers to review this, so > I'm applying it. FWIW, I replied to Luis's patch yesterday. It appears on the netdev list and patchwork. Checked the recipient list, and it includes netdev, but not your address. I guess I will not make that mistake again. https://patchwork.ozlabs.org/patch/463481/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ethernet: myri10ge: use arch_phys_wc_add()
From: Hyong-Youb Kim Date: Fri, 24 Apr 2015 10:57:58 +0900 > X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Apr 2015 01:58:46.2847 (UTC) > X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted > X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR05MB1302 > X-Greylist: Default is to whitelist mail, not delayed by > milter-greylist-4.5.12 (shards.monkeyblade.net [149.20.54.216]); Thu, 23 Apr > 2015 18:58:55 -0700 (PDT) > Content-Type: text/plain; charset="us-ascii" > Content-Disposition: inline > Subject: Re: [PATCH v3] ethernet: myri10ge: use arch_phys_wc_add() > From: Hyong-Youb Kim > To: David Miller > CC: , , , > , , , > , , , > , , , > , , > > Date: Fri, 24 Apr 2015 10:57:58 +0900 > User-Agent: Mutt/1.5.23 (2014-03-12) > > On Thu, Apr 23, 2015 at 02:28:40PM -0400, David Miller wrote: >> I can't wait forever for the driver maintainers to review this, so >> I'm applying it. > > FWIW, I replied to Luis's patch yesterday. It appears on the netdev > list and patchwork. Checked the recipient list, and it includes > netdev, but not your address. I guess I will not make that mistake > again. I saw it. You did not supply an Acked-by: or Tested-by: tag in that response. Also, if you are not going to use the email address listed in the MAINTAINERS entry for this driver, you should update it so I know what email addresses to look for to know if someone is responsible for that driver or not. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH next 1/3] ipvlan: Defer multicast / broadcast processing to a work-queue
On Thu, Apr 23, 2015 at 5:32 PM, Eric Dumazet wrote: > On Thu, 2015-04-23 at 14:29 -0700, Mahesh Bandewar wrote: > >> +static void ipvlan_multicast_enqueue(struct ipvl_port *port, >> + struct sk_buff *skb) >> +{ >> + if (skb->protocol == htons(ETH_P_PAUSE)) >> + return; > > But what happens to this packet ? It seems leaked ? > Hmm, will take care of it in v2. >> + >> + spin_lock(&port->backlog.lock); >> + if (skb_queue_len(&port->backlog) < IPVLAN_QBACKLOG_LIMIT) { >> + __skb_queue_tail(&port->backlog, skb); >> + spin_unlock(&port->backlog.lock); >> + } else { >> + spin_unlock(&port->backlog.lock); >> + atomic_long_inc(&skb->dev->rx_dropped); >> + kfree_skb(skb); >> + } >> + schedule_work(&port->wq); > > No point calling schedule_work(&port->wq); if packet was dropped. > > We are under pressure, so don't add extra cpu cycles ;) > The only possibility of schedule_work() while doing kfree_skb() is when the queue limit is reached. How can the queue be reduced if not scheduled? May be I'm missing something.. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH next 1/3] ipvlan: Defer multicast / broadcast processing to a work-queue
On Thu, 2015-04-23 at 19:54 -0700, Mahesh Bandewar wrote: > The only possibility of schedule_work() while doing kfree_skb() is > when the queue limit is reached. How can the queue be reduced if not > scheduled? May be I'm missing something.. What is the point calling schedule_work() if you queued no packet at this round ? Do you really think it will expedite the queue drain ? Just write instead : spin_lock(&port->backlog.lock); if (skb_queue_len(&port->backlog) < IPVLAN_QBACKLOG_LIMIT) { __skb_queue_tail(&port->backlog, skb); spin_unlock(&port->backlog.lock); schedule_work(&port->wq); } else { spin_unlock(&port->backlog.lock); atomic_long_inc(&skb->dev->rx_dropped); kfree_skb(skb); } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED
On Thu, Apr 23, 2015 at 5:59 PM, Alexei Starovoitov wrote: > On 4/23/15 3:51 PM, Jamal Hadi Salim wrote: >> >> >> So you are planning to add queues? If you are that is a different >> discussion (and the use case needs some clarity). > > > nope. I wasn't planning to do that. > >> For packets being forwarded we already had egress qdiscs which had >> queues so it didnt seem to make sense to enqueue on ingress for such >> use cases. >> There was a use case later where multiple ingress ports had to be >> shared >> and ifb was born - which is pseudo temporary enqueueing on ingress. > > agree. imo ifb approach is more flexible, since it has full hierarchy > of qdiscs. As you're saying above and from the old ifb logs I thought > that ifb is _temporary_ and long term plan is to use ingress_queue, > but looks like this is not the case. Also not too long ago we decided > that we don't want another ingress qdisc. Therefore I think it makes > sense to simplify the code and boost performance. Which performance problem did you see? Did you hit the spinlock bottleneck? That spinlock should be eliminated before you try to do more, and it can be replaced by RCU read lock after John's work (I am still waiting for his patch btw). > I'm not proposing to change tooling, of course. > From iproute2 point of view we'll still have ingress qdisc. > Right now we're pointlessly allocating memory for it and for > ingress_queue, whereas we only need to call cls/act. > I'm proposing to kill them and used tcf_proto in net_device instead. > Right now to reach cls in critical path on ingress we do: > rxq = skb->dev->ingress_queue > sch = rxq->qdisc > sch->enqueue > sch->filter_list > with a bunch of 'if' conditions and useless memory accesses in-between. > If we add 'ingress_filter_list' directly to net_device, > it will be just: > skb->dev->ingress_filter_list > which is huge performance boost. Code size will shrink as well. > iproute2 and all existing tools will work as-is. Looks as win-win to me. Nope, it breaks Qdisc abstraction, filters never attach to netdevice directly. You should stop, since you really don't understand the code. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH next 1/3] ipvlan: Defer multicast / broadcast processing to a work-queue
From: Mahesh Bandewar Date: Thu, 23 Apr 2015 19:54:29 -0700 > On Thu, Apr 23, 2015 at 5:32 PM, Eric Dumazet wrote: >> On Thu, 2015-04-23 at 14:29 -0700, Mahesh Bandewar wrote: >> >>> +static void ipvlan_multicast_enqueue(struct ipvl_port *port, >>> + struct sk_buff *skb) >>> +{ >>> + if (skb->protocol == htons(ETH_P_PAUSE)) >>> + return; >> >> But what happens to this packet ? It seems leaked ? >> > Hmm, will take care of it in v2. Please also provide a proper "[PATCH 0/3] " introduction posting stating what the patch series is doing, and why. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH next 1/3] ipvlan: Defer multicast / broadcast processing to a work-queue
On Thu, Apr 23, 2015 at 9:28 PM, David Miller wrote: > From: Mahesh Bandewar > Date: Thu, 23 Apr 2015 19:54:29 -0700 > >> On Thu, Apr 23, 2015 at 5:32 PM, Eric Dumazet wrote: >>> On Thu, 2015-04-23 at 14:29 -0700, Mahesh Bandewar wrote: >>> +static void ipvlan_multicast_enqueue(struct ipvl_port *port, + struct sk_buff *skb) +{ + if (skb->protocol == htons(ETH_P_PAUSE)) + return; >>> >>> But what happens to this packet ? It seems leaked ? >>> >> Hmm, will take care of it in v2. > > Please also provide a proper "[PATCH 0/3] " introduction posting > stating what the patch series is doing, and why. > Will do. > Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net/tg3: Release IRQs on permanent error
When having permanent EEH error, the PCI device will be removed from the system. For this case, we shouldn't set pcierr_recovery to true wrongly, which blocks the driver to release the allocated interrupts and their handlers. Eventually, we can't disable MSI or MSIx successfully because of the MSI or MSIx interrupts still have associated interrupt actions, which is turned into following stack dump. Oops: Exception in kernel mode, sig: 5 [#1] : [c03b76a8] .free_msi_irqs+0x80/0x1a0 (unreliable) [c039f388] .pci_remove_bus_device+0x98/0x110 [c00790f4] .pcibios_remove_pci_devices+0x9c/0x128 [c0077b98] .handle_eeh_events+0x2d8/0x4b0 [c00782d0] .eeh_event_handler+0x130/0x1c0 [c0022bd4] .kernel_thread+0x54/0x70 Signed-off-by: Gavin Shan --- drivers/net/ethernet/broadcom/tg3.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index 1270b18..069952f 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -18129,7 +18129,9 @@ static pci_ers_result_t tg3_io_error_detected(struct pci_dev *pdev, rtnl_lock(); - tp->pcierr_recovery = true; + /* We needn't recover from permanent error */ + if (state == pci_channel_io_frozen) + tp->pcierr_recovery = true; /* We probably don't have netdev yet */ if (!netdev || !netif_running(netdev)) -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
On Thu, Apr 23, 2015 at 03:24:59PM +0200, Martin Willi wrote: > > Do you have any pointer for me where this is defined? Why is it needed, > given that GCM implicitly authenticates the IV by using it in Y0? Actually you're quite right. Both GCM and CCM implicitly includes the IV in the authentication tag. In fact after reviewing the two relevant RFCs (4106/4309) it seems that we are correct after all since they explicitly exclude the IV from the AAD. Now we just need to figre out whether we're still OK with RFC4543. Sorry for the false alarm. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CCM/GCM implementation defect
On Fri, Apr 24, 2015 at 01:30:00PM +0800, Herbert Xu wrote: > Now we just need to figre out whether we're still OK with RFC4543. Looks like we're OK here too as the diagram in section 3.5 says that the IV should be included in the AAD. So we're all good! Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html