* XDP_DROP and XDP_TX (Was: Re: [net-next PATCH v5 0/6] XDP for virtio_net)
From: Jesper Dangaard Brouer @ 2016-12-13 8:46 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Miller, john.fastabend, daniel, mst, shm, tgraf,
john.r.fastabend, netdev, brouer
In-Reply-To: <20161208193814.GA1954@ast-mbp.thefacebook.com>
On Thu, 8 Dec 2016 11:38:16 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> On Thu, Dec 08, 2016 at 02:17:02PM -0500, David Miller wrote:
> > From: John Fastabend <john.fastabend@gmail.com>
> > Date: Wed, 07 Dec 2016 12:10:47 -0800
> >
[...]
> > > Can't we disable XDP_TX somehow? Many people might only want RX drop,
> > > and extra queues are not always there.
> > >
> >
> > Alexei, Daniel, any thoughts on this?
>
> I don't like it.
I don't know that the use-case virtio XDP_TX is, but I still think we
should implement it for this virtio_net driver, as it allow easier
testing of XDP programs without physical HW.
BUT I do believe XDP_DROP and XDP_TX should be two different capabilities.
I can easily imagine that an older driver only wants to implement the
XDP_DROP facility. The reason is that XDP_TX would require changing too
much driver code, which is a concern for an old, stable and time-proven
driver.
I can also imagine wanting to implement XDP_DROP on my OpenWRT home
router, which I don't think can get an extra HW TX queue for XDP_TX.
I even have a practical use-case for my OpenWRT home router. I
experience a Windows machine being connected to my home network, it
caused some (WiFi) traffic storm that overloaded the small OpenWRT box,
so much that it couldn't even route packets for other machines
connected (on Ethernet). This could also be an IoT device or an
infected machine participating in a DDoS attack. I fairly quickly
identified the bad machine and disconnected it from the network, but
for e.g. conference or hotel networks is can be harder to identify and
disconnect offenders. An XDP eBPF (ddos) filter could allow the sysadm
to "virtually" disconnect a certain MAC address, and restore operation.
> > I know we were trying to claim some base level of feature support for
> > all XDP drivers. I am sympathetic to this argument though for DDOS we
> > do not need XDP_TX support. And virtio can become queue constrained
> > in some cases.
>
> Without XDP_TX it's too crippled. adjust_head() won't be possible,
> packet mangling would have to be disabled and so on.
Even without XDP_TX support, adjust_head() must be supported. The
XDP_PASS action still requires ability to modify the packet.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: Designing a safe RX-zero-copy Memory Model for Networking
From: Mike Rapoport @ 2016-12-13 8:43 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev@vger.kernel.org, linux-mm, John Fastabend,
Willem de Bruijn, Björn Töpel, Karlsson, Magnus,
Alexander Duyck, Mel Gorman, Tom Herbert, Brenden Blanco,
Tariq Toukan, Saeed Mahameed, Jesse Brandeburg, Kalman Meth
In-Reply-To: <20161212161026.0dfd2e13@redhat.com>
On Mon, Dec 12, 2016 at 04:10:26PM +0100, Jesper Dangaard Brouer wrote:
> On Mon, 12 Dec 2016 16:14:33 +0200
> Mike Rapoport <rppt@linux.vnet.ibm.com> wrote:
> >
> > They are copied :-)
> > Presuming we are dealing only with vhost backend, the received skb
> > eventually gets converted to IOVs, which in turn are copied to the guest
> > memory. The IOVs point to the guest memory that is allocated by virtio-net
> > running in the guest.
>
> Thanks for explaining that. It seems like a lot of overhead. I have to
> wrap my head around this... so, the hardware NIC is receiving the
> packet/page, in the RX ring, and after converting it to IOVs, it is
> conceptually transmitted into the guest, and then the guest-side have a
> RX-function to handle this packet. Correctly understood?
Almost :)
For the hardware NIC driver, the receive just follows the "normal" path.
It creates an skb for the packet and passes it to the net core RX. Then the
skb is delivered to tap/macvtap. The later converts the skb to IOVs and
IOVs are pushed to the guest address space.
On the guest side, virtio-net sees these IOVs as a part of its RX ring, it
creates an skb for the packet and passes the skb to the net core of the
guest.
> > I'm not very familiar with XDP eBPF, and it's difficult for me to estimate
> > what needs to be done in BPF program to do proper conversion of skb to the
> > virtio descriptors.
>
> XDP is a step _before_ the SKB is allocated. The XDP eBPF program can
> modify the packet-page data, but I don't think it is needed for your
> use-case. View XDP (primarily) as an early (demux) filter.
>
> XDP is missing a feature your need, which is TX packet into another
> net_device (I actually imagine a port mapping table, that point to a
> net_device). This require a new "TX-raw" NDO that takes a page (+
> offset and length).
>
> I imagine, the virtio driver (virtio_net or a new driver?) getting
> extended with this new "TX-raw" NDO, that takes "raw" packet-pages.
> Whether zero-copy is possible is determined by checking if page
> originates from a page_pool that have enabled zero-copy (and likely
> matching against a "protection domain" id number).
That could be quite a few drivers that will need to implement "TX-raw" then
:)
In general case, the virtual NIC may be connected to the physical network
via long chain of virtual devices such as bridge, veth and ovs.
Actually, because of that we wanted to concentrate on macvtap...
> > We were not considered using XDP yet, so we've decided to limit the initial
> > implementation to macvtap because we can ensure correspondence between a
> > NIC queue and virtual NIC, which is not the case with more generic tap
> > device. It could be that use of XDP will allow for a generic solution for
> > virtio case as well.
>
> You don't need an XDP filter, if you can make the HW do the early demux
> binding into a queue. The check for if memory is zero-copy enabled
> would be the same.
>
> > >
> > > > Have you considered using "push" model for setting the NIC's RX memory?
> > >
> > > I don't understand what you mean by a "push" model?
> >
> > Currently, memory allocation in NIC drivers boils down to alloc_page with
> > some wrapping code. I see two possible ways to make NIC use of some
> > preallocated pages: either NIC driver will call an API (probably different
> > from alloc_page) to obtain that memory, or there will be NDO API that
> > allows to set the NIC's RX buffers. I named the later case "push".
>
> As you might have guessed, I'm not into the "push" model, because this
> means I cannot share the queue with the normal network stack. Which I
> believe is possible as outlined (in email and [2]) and can be done with
> out HW filter features (like macvlan).
I think I should sleep on it a bit more :)
Probably we can add page_pool "backend" implementation to vhost...
^ permalink raw reply
* Re: [PATCH V2 00/22] Broadcom RoCE Driver (bnxt_re)
From: Selvin Xavier @ 2016-12-13 8:36 UTC (permalink / raw)
To: Or Gerlitz
Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Netdev List
In-Reply-To: <CAJ3xEMh98kC1KXGf7uHKD-H91f_NiZXaz-3yTtwQ2s-D7rYqMQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Dec 13, 2016 at 1:29 PM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> I made some quick on-the-surface static checkers etc rub on the new
> driver (Doug, I used
> the bits in your github bnxt_re branch), there are bunch (tons...) of
> smatch [1] and sparse [2]
> complaints along with few checkpatch [3] things too. So... addressing
> them before this goes upstream?
Thank you for pointing it out. I will include this cleanup also as a
part of my v3 series.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: netlink: GPF in sock_sndtimeo
From: Richard Guy Briggs @ 2016-12-13 8:28 UTC (permalink / raw)
To: Cong Wang
Cc: Herbert Xu, Johannes Berg, netdev, Florian Westphal, LKML,
Eric Dumazet, linux-audit, syzkaller, David Miller, Dmitry Vyukov
In-Reply-To: <20161213075117.GH22660@madcap2.tricolour.ca>
On 2016-12-13 02:51, Richard Guy Briggs wrote:
> On 2016-12-09 23:40, Cong Wang wrote:
> > On Fri, Dec 9, 2016 at 8:13 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > On Fri, Dec 9, 2016 at 3:01 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > >> On 2016-12-08 22:57, Cong Wang wrote:
> > >>> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > >>> > I also tried to extend Cong Wang's idea to attempt to proactively respond to a
> > >>> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error
> > >>> > stack dump using mutex_lock(&audit_cmd_mutex) in the notifier callback.
> > >>> > Eliminating the lock since the sock is dead anways eliminates the error.
> > >>> >
> > >>> > Is it safe? I'll resubmit if this looks remotely sane. Meanwhile I'll try to
> > >>> > get the test case to compile.
> > >>>
> > >>> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 'audit_pid'
> > >>> are updated as a whole and race between audit_receive_msg() and
> > >>> NETLINK_URELEASE.
> > >>
> > >> This is what I expected and why I originally added the mutex lock in the
> > >> callback... The dumps I got were bare with no wrapper identifying the
> > >> process context or specific error, so I'm at a bit of a loss how to
> > >> solve this (without thinking more about it) other than instinctively
> > >> removing the mutex.
> > >
> > > Netlink notifier can safely be converted to blocking one, I will send
> > > a patch.
> > >
> > > But I seriously doubt you really need NETLINK_URELEASE here,
> > > it adds nothing but overhead, b/c the netlink notifier is called on
> > > every netlink socket in the system, but for net exit path, that is
> > > relatively a slow path.
> > >
> > > Also, kauditd_send_skb() needs audit_cmd_mutex too.
> >
> > Please let me know what you think about the attached patch?
> >
> > Thanks!
>
> > commit a12b43ee814625933ff155c20dc863c59cfcf240
> > Author: Cong Wang <xiyou.wangcong@gmail.com>
> > Date: Fri Dec 9 17:56:42 2016 -0800
> >
> > audit: close a race condition on audit_sock
> >
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f1ca116..ab947d8 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -423,6 +423,8 @@ static void kauditd_send_skb(struct sk_buff *skb)
> > snprintf(s, sizeof(s), "audit_pid=%d reset", audit_pid);
> > audit_log_lost(s);
> > audit_pid = 0;
> > + audit_nlk_portid = 0;
> > + sock_put(audit_sock);
> > audit_sock = NULL;
> > } else {
> > pr_warn("re-scheduling(#%d) write to audit_pid=%d\n",
> > @@ -899,6 +901,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
> > audit_pid = new_pid;
> > audit_nlk_portid = NETLINK_CB(skb).portid;
> > + sock_hold(skb->sk);
> > + if (audit_sock)
> > + sock_put(audit_sock);
> > audit_sock = skb->sk;
> > }
> > if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
> > @@ -1167,10 +1172,6 @@ static void __net_exit audit_net_exit(struct net *net)
> > {
> > struct audit_net *aunet = net_generic(net, audit_net_id);
> > struct sock *sock = aunet->nlsk;
> > - if (sock == audit_sock) {
> > - audit_pid = 0;
> > - audit_sock = NULL;
> > - }
>
> So how does this not leak memory leaving the sock refcount incremented
> by the registered audit daemon when that daemon shuts down normally?
Sorry, that should have been: How does it not leak if auditd exits
abnormally without sending a shutdown message, but no message is sent on
the queue to trigger an error before the net namespace exits?
> - RGB
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: stmmac DT property snps,axi_all
From: Niklas Cassel @ 2016-12-13 8:26 UTC (permalink / raw)
To: Giuseppe CAVALLARO, Alexandre Torgue; +Cc: netdev
In-Reply-To: <52fdb263-0b36-9984-e46f-78f3f83f1188@st.com>
On 12/13/2016 07:47 AM, Giuseppe CAVALLARO wrote:
> Hello Niklas, Alex,
>
> my fault and a step behind... Current code is OK
> when manage the AAL that, although it is passed from
> the axi structure, it is always used to program,
> for all the chip versions, the writable bit inside the
> DMA_BUS_MODE register.
> So I guess no extra patch is needed.
Hello Giuseppe
I'm not sure what you mean.
Yes, when setting "snps,aal",
the code is correct for all versions of the IP.
However, "snps,axi_all" seems to just be dead code.
Try git grep in the whole tree.
So I still think that my patch is valid.
>
> Regards
> Peppe
>
> On 12/12/2016 3:18 PM, Giuseppe CAVALLARO wrote:
>> Please Niklas
>>
>> when you send the patch, add my
>>
>> Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>>
>>
>> On 12/9/2016 10:53 AM, Niklas Cassel wrote:
>>> On 12/09/2016 10:20 AM, Niklas Cassel wrote:
>>>> On 12/08/2016 02:36 PM, Alexandre Torgue wrote:
>>>>> Hi Niklas,
>>>>>
>>>>> On 12/05/2016 05:18 PM, Niklas Cassel wrote:
>>>>>> Hello Giuseppe
>>>>>>
>>>>>>
>>>>>> I'm trying to figure out what snps,axi_all is supposed to represent.
>>>>>>
>>>>>> It appears that the value is saved, but never used in the code.
>>>>>>
>>>>>> Looking at the register specification, I'm guessing that it represents
>>>>>> Address-Aligned Beats, but there is already the property snps,aal
>>>>>> for that.
>>>>> IMO, it is not useful. Indeed AXI_AAL is a read only bit (in AXI bus
>>>>> mode register) and reflects the aal bit in DMA bus register.
>>>>> As you know we use "snps,aal" to set aal bit in DMA bus register.
>>>>> So "snps,axi_all" entry seems useless. Let's see with Peppe.
>>>> Ok, I see. GMAC and GMAC4 is different here.
>>>>
>>>> For GMAC4 AAL only exists in DMA_SYS_BUS_MODE.
>>>> It's not reflected anywhere else.
>>>>
>>>> The code is correct in the driver.
>>>>
>>>> If snps,axi_all is just created for a read-only register,
>>>> and it is currently never used in the code,
>>>> while we have snps,aal, which is correct and works,
>>>> I guess it should be ok to remove snps,axi_all.
>>>>
>>>> I can cook up a patch.
>>>>
>>>
>>> Here we go :)
>>>
>>> I will send it as a real patch once net-next reopens.
>>>
>>>
>>>> From defc01cb7c22611b89d9cf1fcae72544092bd62c Mon Sep 17 00:00:00 2001
>>> From: Niklas Cassel <niklas.cassel@axis.com>
>>> Date: Fri, 9 Dec 2016 10:27:00 +0100
>>> Subject: [PATCH net-next] net: stmmac: remove unused duplicate property
>>> snps,axi_all
>>>
>>> For core revision 3.x Address-Aligned Beats is available in two
>>> registers.
>>> The DT property snps,aal was created for AAL in the DMA bus register,
>>> which is a read/write bit.
>>> The DT property snps,axi_all was created for AXI_AAL in the AXI bus mode
>>> register, which is a read only bit that reflects the value of AAL in the
>>> DMA bus register.
>>>
>>> Since the value of snps,axi_all is never used in the driver,
>>> and since the property was created for a bit that is read only,
>>> it should be safe to remove the property.
>>>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>>> ---
>>> Documentation/devicetree/bindings/net/stmmac.txt | 1 -
>>> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 1 -
>>> include/linux/stmmac.h | 1 -
>>> 3 files changed, 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt
>>> b/Documentation/devicetree/bindings/net/stmmac.txt
>>> index 128da752fec9..c3d2fd480a1b 100644
>>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>>> @@ -65,7 +65,6 @@ Optional properties:
>>> - snps,wr_osr_lmt: max write outstanding req. limit
>>> - snps,rd_osr_lmt: max read outstanding req. limit
>>> - snps,kbbe: do not cross 1KiB boundary.
>>> - - snps,axi_all: align address
>>> - snps,blen: this is a vector of supported burst length.
>>> - snps,fb: fixed-burst
>>> - snps,mb: mixed-burst
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> index 082cd48db6a7..60ba8993c650 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -121,7 +121,6 @@ static struct stmmac_axi *stmmac_axi_setup(struct
>>> platform_device *pdev)
>>> axi->axi_lpi_en = of_property_read_bool(np, "snps,lpi_en");
>>> axi->axi_xit_frm = of_property_read_bool(np, "snps,xit_frm");
>>> axi->axi_kbbe = of_property_read_bool(np, "snps,axi_kbbe");
>>> - axi->axi_axi_all = of_property_read_bool(np, "snps,axi_all");
>>> axi->axi_fb = of_property_read_bool(np, "snps,axi_fb");
>>> axi->axi_mb = of_property_read_bool(np, "snps,axi_mb");
>>> axi->axi_rb = of_property_read_bool(np, "snps,axi_rb");
>>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
>>> index 266dab9ad782..889e0e9a3f1c 100644
>>> --- a/include/linux/stmmac.h
>>> +++ b/include/linux/stmmac.h
>>> @@ -103,7 +103,6 @@ struct stmmac_axi {
>>> u32 axi_wr_osr_lmt;
>>> u32 axi_rd_osr_lmt;
>>> bool axi_kbbe;
>>> - bool axi_axi_all;
>>> u32 axi_blen[AXI_BLEN];
>>> bool axi_fb;
>>> bool axi_mb;
>>>
>>
>>
>
^ permalink raw reply
* Re: [PATCH V2 00/22] Broadcom RoCE Driver (bnxt_re)
From: Or Gerlitz @ 2016-12-13 7:59 UTC (permalink / raw)
To: Doug Ledford, Selvin Xavier
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Netdev List
In-Reply-To: <23e26353-4317-2836-9f94-d1fc3274a770-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, Dec 13, 2016 at 1:52 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 12/9/2016 1:47 AM, Selvin Xavier wrote:
>> This series introduces the RoCE driver for the Broadcom
>> NetXtreme-E 10/25/40/50 gigabit RoCE HCAs.
>> This driver is dependent on the bnxt_en NIC driver and is
>> based on the bnxt_re branch in Doug's repository. bnxt_en changes
>> required for this patch series is already available in this branch.
>>
>> I am preparing a git repository with these changes as per Jason's
>> comment and will share the details later today.
>>
>> v1-> v2:
>> * The license text in each file updated to reflect Dual license.
>> * Makefile and Kconfig changes are pushed to the last patch
>> * Moved bnxt_re_uverbs_abi.h to include/uapi/rdma folder
>> * Remove duplicate structure definitions from bnxt_re_hsi.h as
>> it is available in the corresponding bnxt_en header file (bnxt_hsi.h)
>> * Removed some unused code reported during code review.
>> * Fixed few sparse warnings
>>
>> Doug,
>> Please review and consider applying this to linux-rdma repository.
>
> There are outstanding review comments to be addressed still yet, and the
> v2 patchset doesn't compile for me in 0day testing. I'm going to bounce
> this one to 4.11.
I made some quick on-the-surface static checkers etc rub on the new
driver (Doug, I used
the bits in your github bnxt_re branch), there are bunch (tons...) of
smatch [1] and sparse [2]
complaints along with few checkpatch [3] things too. So... addressing
them before this goes upstream?
BTW net-next's drivers/net/ethernet/broadcom/bnxt (where you put the
pre patches for the
RDMA driver) is pretty much clean of that
Or.
[1] smatch
CHECK drivers/infiniband/hw/bnxt_re/bnxt_re_main.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_re_debugfs.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_qplib_rcfw.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:185
bnxt_qplib_del_sgid() warn: impossible condition '(*sgid_tbl->hw_id +
index == -1) => (0-65535 == (-1))'
drivers/infiniband/hw/bnxt_re/bnxt_qplib_rcfw.c:107
bnxt_qplib_rcfw_send_message() warn: test_bit() takes a bit number
drivers/infiniband/hw/bnxt_re/bnxt_qplib_rcfw.c:116
bnxt_qplib_rcfw_send_message() warn: test_bit() takes a bit number
drivers/infiniband/hw/bnxt_re/bnxt_qplib_rcfw.c:148
bnxt_qplib_rcfw_send_message() error: double lock 'irqsave:flags'
drivers/infiniband/hw/bnxt_re/bnxt_qplib_rcfw.c:204
bnxt_qplib_rcfw_send_message() error: double unlock 'irqsave:flags'
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:121
bnxt_re_unregister_netdev() warn: variable dereferenced before check
'rdev' (see line 118)
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:140
bnxt_re_register_netdev() warn: variable dereferenced before check
'rdev' (see line 137)
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:155 bnxt_re_free_msix()
warn: variable dereferenced before check 'rdev' (see line 152)
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:173
bnxt_re_request_msix() warn: variable dereferenced before check 'rdev'
(see line 171)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:172
bnxt_qplib_alloc_init_hwq() warn: unsigned '*elements' is never less
than zero.
drivers/infiniband/hw/bnxt_re/bnxt_qplib_rcfw.c:387
bnxt_qplib_deinit_rcfw() warn: test_bit() takes a bit number
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:488
bnxt_qplib_alloc_sgid_tbl() warn: double check that we're allocating
correct size: 2 vs 4
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:688
bnxt_qplib_alloc_dpi_tbl() warn: consider using resource_size() here
drivers/infiniband/hw/bnxt_re/bnxt_qplib_rcfw.c:496
bnxt_qplib_init_rcfw() warn: test_bit() takes a bit number
./include/linux/mm.h:1385 stack_guard_page_end() warn: bitwise AND
condition is false here
./include/linux/mm.h:1379 vma_growsup() warn: bitwise AND condition is
false here
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:522 show_rev() info:
ignoring unreachable code.
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:835 bnxt_re_dev_stop()
warn: was && intended here instead of ||?
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:1048 bnxt_re_ib_reg()
warn: curly braces intended?
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:1050 bnxt_re_ib_reg()
warn: inconsistent indenting
drivers/infiniband/hw/bnxt_re/bnxt_re_main.c:1190 bnxt_re_task() warn:
curly braces intended?
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1639 __flush_sq() warn:
variable dereferenced before check 'budget' (see line 1621)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1852
bnxt_qplib_cq_process_res_ud() warn: shift has higher precedence than
mask
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1861
bnxt_qplib_cq_process_res_ud() warn: inconsistent indenting
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:2015
bnxt_qplib_cq_process_terminal() warn: variable dereferenced before
check 'budget' (see line 1997)
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:1607
bnxt_re_build_qp1_send_v2 warn: unused return: size =
ib_ud_header_pack()
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:1681
bnxt_re_build_qp1_shadow_qp_recv() warn: unsigned 'sge.size' is never
less than zero.
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2066
bnxt_re_post_recv_shadow_qp warn: unused return: payload_sz =
bnxt_re_build_sgl()
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2242
bnxt_re_create_cq() warn: variable dereferenced before check
'cq->umem' (see line 2192)
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2456
bnxt_re_is_loopback_packet() warn: curly braces intended?
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2512
bnxt_re_process_raw_qp_pkt_rx() warn: impossible condition '(pkt_type
< 0) => (0-255 < 0)'
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2578
bnxt_re_process_raw_qp_pkt_rx warn: unused return: rc =
bnxt_re_post_send_shadow_qp()
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2838 bnxt_re_dereg_mr
warn: unused return: rc = bnxt_qplib_free_fast_reg_page_list()
[2] sparse
CHECK drivers/infiniband/hw/bnxt_re/bnxt_re_main.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_re_debugfs.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_qplib_rcfw.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c
CHECK drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:111:33: warning: cast to
restricted __le32
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:114:30: warning:
restricted __le32 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:114:30: warning: cast to
restricted __le32
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:277:28: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:277:28: expected
restricted __le32 <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:277:28: got restricted
__be32 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:278:28: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:278:28: expected
restricted __le32 <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:278:28: got restricted
__be32 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:279:28: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:279:28: expected
restricted __le32 <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:279:28: got restricted
__be32 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:280:28: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:280:28: expected
restricted __le32 <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:280:28: got restricted
__be32 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:282:34: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:282:34: expected
restricted __le16 [addressable] [assigned] [usertype] vlan
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:282:34: got restricted
__le32 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:289:32: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:289:32: expected
restricted __le16 <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:289:32: got restricted
__be16 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:290:32: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:290:32: expected
restricted __le16 <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:290:32: got restricted
__be16 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:291:32: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:291:32: expected
restricted __le16 <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:291:32: got restricted
__be16 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:810:18: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:810:18: expected
restricted __le16 [addressable] [assigned] [usertype] cos0
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:810:18: got unsigned
short [unsigned] [short] [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:811:18: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:811:18: expected
restricted __le16 [addressable] [assigned] [usertype] cos1
drivers/infiniband/hw/bnxt_re/bnxt_qplib_sp.c:811:18: got unsigned
short [unsigned] [short] [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:133:22: warning:
restricted __le32 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:136:24: warning:
restricted __le16 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:136:24: warning: cast to
restricted __le16
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:777:25: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:777:25: expected
restricted __le32 [addressable] [assigned] [usertype] modify_mask
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:777:25: got restricted
__le64 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:822:30: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:822:30: expected
unsigned char [unsigned] [addressable] [assigned] [usertype] path_mtu
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:822:30: got restricted
__le16 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:953:26: warning:
restricted __le16 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:953:26: warning: cast to
restricted __le16
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:956:26: warning:
restricted __le16 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:970:26: warning: cast to
restricted __le32
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:970:26: warning: cast
from restricted __le16
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1211:34: warning:
invalid assignment: +=
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1211:34: left side has type int
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1211:34: right side has
type restricted __le32
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1333:24: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1333:24: expected
unsigned int [unsigned] [usertype] temp32
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1333:24: got restricted
__le32 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1343:46: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1343:46: expected
unsigned long long [unsigned] [long] [long long] [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1343:46: got restricted
__le64 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1363:24: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1363:24: expected
unsigned int [unsigned] [addressable] [usertype] temp32
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1363:24: got restricted
__le32 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1533:27: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1533:27: expected
restricted __le32 [addressable] [assigned] [usertype] cq_fco_cnq_id
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1533:27: got restricted
__le16 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1796:21: warning:
restricted __le32 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1796:21: warning: cast
to restricted __le64
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1849:21: warning:
restricted __le32 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1849:21: warning: cast
to restricted __le64
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1852:39: warning:
restricted __le32 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1904:21: warning:
restricted __le32 degrades to integer
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1904:21: warning: cast
to restricted __le64
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1917:34: warning: cast
to restricted __le16
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1917:34: warning: cast
from restricted __le32
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1015:22: warning:
context imbalance in 'bnxt_qplib_lock_cqs' - wrong count at exit
drivers/infiniband/hw/bnxt_re/bnxt_qplib_fp.c:1030:28: warning:
context imbalance in 'bnxt_qplib_unlock_cqs' - unexpected unlock
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:410:72: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:410:72: expected
unsigned long long [unsigned] [long] [long long] [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:410:72: got restricted
__le64 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:418:56: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:418:56: expected
unsigned long long [unsigned] [long] [long long] [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:418:56: got restricted
__le64 [usertype] <noident>
drivers/infiniband/hw/bnxt_re/bnxt_qplib_res.c:729:6: warning: symbol
'bnxt_qplib_cleanup_pkey_tbl' was not declared. Should it be static?
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:1725:47: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:1725:47: expected
unsigned int [unsigned] [usertype] imm_data_or_inv_key
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:1725:47: got
restricted __be32 [usertype] imm_data
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:1755:47: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:1755:47: expected
unsigned int [unsigned] [usertype] imm_data_or_inv_key
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:1755:47: got
restricted __be32 [usertype] imm_data
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2627:25: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2627:25: expected
restricted __be32 [usertype] imm_data
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2627:25: got unsigned
int [unsigned] [usertype] immdata_or_invrkey
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2696:41: warning:
incorrect type in assignment (different base types)
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2696:41: expected
restricted __be32 [usertype] imm_data
drivers/infiniband/hw/bnxt_re/bnxt_re_ib_verbs.c:2696:41: got unsigned
int [unsigned] [usertype] immdata_or_invrkey
[3] checkpatch
CHECK: Macro argument reuse 'req' - possible side-effects?
CHECK: Macro argument reuse 'prod' - possible side-effects?
CHECK: Macro argument reuse 'dma_addr' - possible side-effects?
CHECK: Macro argument reuse 'rdev' - possible side-effects?
CHECK: Please don't use multiple blank lines
CHECK: Alignment should match open parenthesis
CHECK: Alignment should match open parenthesis
CHECK: Alignment should match open parenthesis
CHECK: Alignment should match open parenthesis
CHECK: Alignment should match open parenthesis
CHECK: Alignment should match open parenthesis
CHECK: Alignment should match open parenthesis
CHECK: Alignment should match open parenthesis
CHECK: Please don't use multiple blank lines
CHECK: DEFINE_MUTEX definition without comment
CHECK: Please use a blank line after function/struct/union/enum declarations
CHECK: Please use a blank line after function/struct/union/enum declarations
CHECK: Please use a blank line after function/struct/union/enum declarations
CHECK: Please use a blank line after function/struct/union/enum declarations
CHECK: Alignment should match open parenthesis
CHECK: Please use a blank line after function/struct/union/enum declarations
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: netlink: GPF in sock_sndtimeo
From: Richard Guy Briggs @ 2016-12-13 7:51 UTC (permalink / raw)
To: Cong Wang
Cc: Herbert Xu, Johannes Berg, netdev, Florian Westphal, LKML,
Eric Dumazet, linux-audit, syzkaller, David Miller, Dmitry Vyukov
In-Reply-To: <CAM_iQpVcHGywXn90EpiSz-LsUDgKVqs-7BY-L7UBCu2VxkC31Q@mail.gmail.com>
On 2016-12-09 23:40, Cong Wang wrote:
> On Fri, Dec 9, 2016 at 8:13 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Fri, Dec 9, 2016 at 3:01 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> On 2016-12-08 22:57, Cong Wang wrote:
> >>> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >>> > I also tried to extend Cong Wang's idea to attempt to proactively respond to a
> >>> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error
> >>> > stack dump using mutex_lock(&audit_cmd_mutex) in the notifier callback.
> >>> > Eliminating the lock since the sock is dead anways eliminates the error.
> >>> >
> >>> > Is it safe? I'll resubmit if this looks remotely sane. Meanwhile I'll try to
> >>> > get the test case to compile.
> >>>
> >>> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 'audit_pid'
> >>> are updated as a whole and race between audit_receive_msg() and
> >>> NETLINK_URELEASE.
> >>
> >> This is what I expected and why I originally added the mutex lock in the
> >> callback... The dumps I got were bare with no wrapper identifying the
> >> process context or specific error, so I'm at a bit of a loss how to
> >> solve this (without thinking more about it) other than instinctively
> >> removing the mutex.
> >
> > Netlink notifier can safely be converted to blocking one, I will send
> > a patch.
> >
> > But I seriously doubt you really need NETLINK_URELEASE here,
> > it adds nothing but overhead, b/c the netlink notifier is called on
> > every netlink socket in the system, but for net exit path, that is
> > relatively a slow path.
> >
> > Also, kauditd_send_skb() needs audit_cmd_mutex too.
>
> Please let me know what you think about the attached patch?
>
> Thanks!
> commit a12b43ee814625933ff155c20dc863c59cfcf240
> Author: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Fri Dec 9 17:56:42 2016 -0800
>
> audit: close a race condition on audit_sock
>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f1ca116..ab947d8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -423,6 +423,8 @@ static void kauditd_send_skb(struct sk_buff *skb)
> snprintf(s, sizeof(s), "audit_pid=%d reset", audit_pid);
> audit_log_lost(s);
> audit_pid = 0;
> + audit_nlk_portid = 0;
> + sock_put(audit_sock);
> audit_sock = NULL;
> } else {
> pr_warn("re-scheduling(#%d) write to audit_pid=%d\n",
> @@ -899,6 +901,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
> audit_pid = new_pid;
> audit_nlk_portid = NETLINK_CB(skb).portid;
> + sock_hold(skb->sk);
> + if (audit_sock)
> + sock_put(audit_sock);
> audit_sock = skb->sk;
> }
> if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
> @@ -1167,10 +1172,6 @@ static void __net_exit audit_net_exit(struct net *net)
> {
> struct audit_net *aunet = net_generic(net, audit_net_id);
> struct sock *sock = aunet->nlsk;
> - if (sock == audit_sock) {
> - audit_pid = 0;
> - audit_sock = NULL;
> - }
So how does this not leak memory leaving the sock refcount incremented
by the registered audit daemon when that daemon shuts down normally?
- RGB
^ permalink raw reply
* Re: Synopsys Ethernet QoS
From: Giuseppe CAVALLARO @ 2016-12-13 7:22 UTC (permalink / raw)
To: Niklas Cassel, Joao Pinto, Florian Fainelli, Andy Shevchenko
Cc: David Miller, larper, rabinv, netdev, CARLOS.PALMINHA, Jie.Deng1,
Stephen Warren, pavel
In-Reply-To: <1d445ec1-deb8-6e36-39c4-6813c446095f@axis.com>
On 12/12/2016 5:25 PM, Niklas Cassel wrote:
>
>
> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>> Hi,
>>
>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>
>>>>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
>>>>> did
>>>>> actually pioneer the upstreaming effort, but it is good to see people
>>>>> from Synopsys willing to fix that in the future.
>>>> Wait, you would like to tell that we have more than 2 drivers for the
>>>> same (okay, same vendor) IP?!
>>>> It's better to unify them earlier, than have n+ copies.
>>> Unfortunately that is the case, see this email:
>>>
>>> https://www.mail-archive.com/netdev@vger.kernel.org/msg142796.html
>>>
>>> dwc_eth_qos and stmmac have some overlap. There seems to be work
>>> underway to unify these two to begin with.
>>>
>>>> P.S. Though, I don't see how sxgbe got in the list. First glance on
>>>> the code doesn't show similarities.
>>> Well samsung/sxgbe looks potentially similar to amd/xgbe, but that's
>>> just my cursory look at the code, it may very well be something entirely
>>> different. The descriptor formats just look suspiciously similar.
>>>
>> Thank you for your inputs! Renaming seems to be a hotspot. I agree that maybe
>> instead of renaming (breaking retro-compatibility as David and Florian
>> mentioned), the best is to move stmmac to synopsys/ after merging *qos* and
>> removing it. As Florian mentioned, git is capable of detecting folder restructured.
>>
>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>> driver would it be possible for you to make an initial analysis of what has to
>> be merged into Stmmac? This way the development would speed-up.
>
> I can answer that question.
>
> I've sent out 12 patches to the stmmac driver
> (all patches are included in the current net-next tree),
ok I have seen these patches applied, I had just a minor concern about
the failure when DMA configuration is missing.
In these years, I have noticed that, for this kind of HW, default DMA
configuration is usually good to have a driver working. AHB, AXI
parameters can be provided to have a best tuning or to fix know issues
on some platforms. So IMO, we should relax the check with a warning.
Please, consider that, the stmmac also supports very old MAC10/100
versions where the DMA configuration was often never passed.
> with these patches the stmmac driver works properly on Axis hardware
> (we use Synopsys GMAC 4.10a synthesized with multiple TX queues).
perfect and thanks a lot for this effort.
> stmmac's DT binding has also been extended with properties that
> existed in DWC EQoS's DT binding, such as no-pbl-x8, txpbl, rxpbl.
>
> Since we have no problem updating the DTB together with the kernel,
> we will simply move to using the start using the stmmac driver,
> with stmmac's DT binding.
>
> However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
> I don't how easy it would be for them to switch to stmmac's DT binding.
> (Adding Stephen Warren to CC.)
ok
>
> The reset sequence that Lars Persson was worried about is not an issue
> with the stmmac driver.
>
thx for this check.
>
> There are some performance problems with the stmmac driver though:
>
> When running iperf3 with 3 streams:
> iperf3 -c 192.168.0.90 -P 3 -t 30
> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
>
> I get really bad fairness between the streams.
Can you confirm you are using the 4.xxa version?
This doesn't match with Alex's experiments on ARM platforms.
> This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
> Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.
this doesn't match with what we had seen but I am happy and open to
review and accept new strategy.
> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
> and we don't see the same problem.
please, if you have new patch add me on CC and we will review all
together.
> Also netperf TCP_RR and UDP_RR gives really bad results compared to the
> dwceqos driver (without IRQ coalescing).
> 2000 transactions/sec vs 9000 transactions/sec.
> Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
> gives the same performance. I guess it's a trade off, low CPU usage
> vs low latency, so I don't know how important TCP_RR/UDP_RR really is.
>
> The best thing would be to get a good working TX IRQ coalesce
> implementation with HR timers in stmmac.
as said, welcome patches.
Basically, the default tuning of coalescence parameters comes from
ST platform experiences. I mean, we tuned to driver to have good
performance and saving CPU on SH4 (UP) and ARM (SMP) systems.
In these years, these default was accepted but, if today we need
to change something welcome effort. On my side, I can try to
perform some bench to see if I have regressions on not.
> Perhaps it should also be investigated if the RX interrupt watchdog
> timeout should have a lower default value.
Do not expect to get many improvements to play with the HW watchdog
due to the poor granularity of the Receive Interrupt Watchdog Timer
Count.
Regards
Peppe
>
>
>
>>
>> Thanks to all.
>>
>> Joao
>
>
^ permalink raw reply
* Re: stmmac DT property snps,axi_all
From: Giuseppe CAVALLARO @ 2016-12-13 6:47 UTC (permalink / raw)
To: Niklas Cassel, Alexandre Torgue; +Cc: netdev
In-Reply-To: <0546490f-8f8c-a22e-db7b-eac521c7ff27@st.com>
Hello Niklas, Alex,
my fault and a step behind... Current code is OK
when manage the AAL that, although it is passed from
the axi structure, it is always used to program,
for all the chip versions, the writable bit inside the
DMA_BUS_MODE register.
So I guess no extra patch is needed.
Regards
Peppe
On 12/12/2016 3:18 PM, Giuseppe CAVALLARO wrote:
> Please Niklas
>
> when you send the patch, add my
>
> Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>
>
> On 12/9/2016 10:53 AM, Niklas Cassel wrote:
>> On 12/09/2016 10:20 AM, Niklas Cassel wrote:
>>> On 12/08/2016 02:36 PM, Alexandre Torgue wrote:
>>>> Hi Niklas,
>>>>
>>>> On 12/05/2016 05:18 PM, Niklas Cassel wrote:
>>>>> Hello Giuseppe
>>>>>
>>>>>
>>>>> I'm trying to figure out what snps,axi_all is supposed to represent.
>>>>>
>>>>> It appears that the value is saved, but never used in the code.
>>>>>
>>>>> Looking at the register specification, I'm guessing that it represents
>>>>> Address-Aligned Beats, but there is already the property snps,aal
>>>>> for that.
>>>> IMO, it is not useful. Indeed AXI_AAL is a read only bit (in AXI bus
>>>> mode register) and reflects the aal bit in DMA bus register.
>>>> As you know we use "snps,aal" to set aal bit in DMA bus register.
>>>> So "snps,axi_all" entry seems useless. Let's see with Peppe.
>>> Ok, I see. GMAC and GMAC4 is different here.
>>>
>>> For GMAC4 AAL only exists in DMA_SYS_BUS_MODE.
>>> It's not reflected anywhere else.
>>>
>>> The code is correct in the driver.
>>>
>>> If snps,axi_all is just created for a read-only register,
>>> and it is currently never used in the code,
>>> while we have snps,aal, which is correct and works,
>>> I guess it should be ok to remove snps,axi_all.
>>>
>>> I can cook up a patch.
>>>
>>
>> Here we go :)
>>
>> I will send it as a real patch once net-next reopens.
>>
>>
>>> From defc01cb7c22611b89d9cf1fcae72544092bd62c Mon Sep 17 00:00:00 2001
>> From: Niklas Cassel <niklas.cassel@axis.com>
>> Date: Fri, 9 Dec 2016 10:27:00 +0100
>> Subject: [PATCH net-next] net: stmmac: remove unused duplicate property
>> snps,axi_all
>>
>> For core revision 3.x Address-Aligned Beats is available in two
>> registers.
>> The DT property snps,aal was created for AAL in the DMA bus register,
>> which is a read/write bit.
>> The DT property snps,axi_all was created for AXI_AAL in the AXI bus mode
>> register, which is a read only bit that reflects the value of AAL in the
>> DMA bus register.
>>
>> Since the value of snps,axi_all is never used in the driver,
>> and since the property was created for a bit that is read only,
>> it should be safe to remove the property.
>>
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>> ---
>> Documentation/devicetree/bindings/net/stmmac.txt | 1 -
>> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 1 -
>> include/linux/stmmac.h | 1 -
>> 3 files changed, 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt
>> b/Documentation/devicetree/bindings/net/stmmac.txt
>> index 128da752fec9..c3d2fd480a1b 100644
>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>> @@ -65,7 +65,6 @@ Optional properties:
>> - snps,wr_osr_lmt: max write outstanding req. limit
>> - snps,rd_osr_lmt: max read outstanding req. limit
>> - snps,kbbe: do not cross 1KiB boundary.
>> - - snps,axi_all: align address
>> - snps,blen: this is a vector of supported burst length.
>> - snps,fb: fixed-burst
>> - snps,mb: mixed-burst
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> index 082cd48db6a7..60ba8993c650 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -121,7 +121,6 @@ static struct stmmac_axi *stmmac_axi_setup(struct
>> platform_device *pdev)
>> axi->axi_lpi_en = of_property_read_bool(np, "snps,lpi_en");
>> axi->axi_xit_frm = of_property_read_bool(np, "snps,xit_frm");
>> axi->axi_kbbe = of_property_read_bool(np, "snps,axi_kbbe");
>> - axi->axi_axi_all = of_property_read_bool(np, "snps,axi_all");
>> axi->axi_fb = of_property_read_bool(np, "snps,axi_fb");
>> axi->axi_mb = of_property_read_bool(np, "snps,axi_mb");
>> axi->axi_rb = of_property_read_bool(np, "snps,axi_rb");
>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
>> index 266dab9ad782..889e0e9a3f1c 100644
>> --- a/include/linux/stmmac.h
>> +++ b/include/linux/stmmac.h
>> @@ -103,7 +103,6 @@ struct stmmac_axi {
>> u32 axi_wr_osr_lmt;
>> u32 axi_rd_osr_lmt;
>> bool axi_kbbe;
>> - bool axi_axi_all;
>> u32 axi_blen[AXI_BLEN];
>> bool axi_fb;
>> bool axi_mb;
>>
>
>
^ permalink raw reply
* Re: [PATCH V2 00/22] Broadcom RoCE Driver (bnxt_re)
From: Michael Chan @ 2016-12-13 6:41 UTC (permalink / raw)
To: Selvin Xavier; +Cc: jtoppins, Doug Ledford, linux-rdma, Netdev
In-Reply-To: <CA+sbYW2fF01N5ZQfaH6Uj7Q63SHwdo1gEwR=eoB6vB1ktQaiag@mail.gmail.com>
On Mon, Dec 12, 2016 at 8:52 PM, Selvin Xavier
<selvin.xavier@broadcom.com> wrote:
>> CHECK drivers/infiniband/hw/bnxtre/bnxt_qplib_rcfw.c
>> CHECK drivers/infiniband/hw/bnxtre/bnxt_qplib_sp.c
>> CHECK drivers/infiniband/hw/bnxtre/bnxt_qplib_fp.c
>> drivers/infiniband/hw/bnxtre/bnxt_qplib_fp.c:1015:22: warning: context
>> imbalance in 'bnxt_qplib_lock_cqs' - wrong count at exit
>> drivers/infiniband/hw/bnxtre/bnxt_qplib_fp.c:1030:28: warning: context
>> imbalance in 'bnxt_qplib_unlock_cqs' - unexpected unlock
> The above two are false positives, since locking and unlocking are
> handled in two separate functions. This is a wrapper to lock/unlock
> both SQ and RQ CQ locks. Functionally it is ok since
> bnxt_qplib_unlock_cqs is called just after the critical section and
> both locks are freed in order. I think we can ignore this warning.
>
>
You can use __releases() and __acquires() macros to denote these cases
for sparse.
^ permalink raw reply
* Re: [PATCH V2 18/22] bnxt_re: Support for DCB
From: Selvin Xavier @ 2016-12-13 6:25 UTC (permalink / raw)
To: Or Gerlitz
Cc: Doug Ledford, linux-rdma@vger.kernel.org, Linux Netdev List,
Eddie Wai, Devesh Sharma, Somnath Kotur, Sriharsha Basavapatna
In-Reply-To: <CAJ3xEMjcKRZpBMEBwQMZO5OqbVzH2=Q3FOAyYUVAz+ouw1jTNQ@mail.gmail.com>
On Sat, Dec 10, 2016 at 7:20 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Fri, Dec 9, 2016 at 8:48 AM, Selvin Xavier
> <selvin.xavier@broadcom.com> wrote:
>> This patch queries the configured RoCE APP Priority on the host
>> using the dcbnl API and programs the RoCE FW with the corresponding
>> Traffic Class(es) for the priority.
>
>> +#define BNXT_RE_ROCE_V1_ETH_TYPE 0x8915
>> +#define BNXT_RE_ROCE_V2_PORT_NO 4791
>
> I believe these two are defined already, try # git grep on each under include
Thanks Or for your comments.
V2 port number is defined in ib_verbs.h. i will include this in the
next patch set.
v1 eth_type is not defined. All vendor drivers have their own definition.
Thanks,
Selvin
^ permalink raw reply
* [PATCH net] virtio-net: correctly enable multiqueue
From: Jason Wang @ 2016-12-13 6:23 UTC (permalink / raw)
To: netdev, virtualization; +Cc: tytso, Neil Horman, Michael S . Tsirkin
Commit 4490001029012539937ff02778fe6180613fa949 ("virtio-net: enable
multiqueue by default") blindly set the affinity instead of queues
during probe which can cause a mismatch of #queues between guest and
host. This patch fixes it by setting queues.
Reported-by: Theodore Ts'o <tytso@mit.edu>
Tested-by: Theodore Ts'o <tytso@mit.edu>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Fixes: 49000102901 ("virtio-net: enable multiqueue by default")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b425fa1..fe9f772 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1930,7 +1930,9 @@ static int virtnet_probe(struct virtio_device *vdev)
goto free_unregister_netdev;
}
- virtnet_set_affinity(vi);
+ rtnl_lock();
+ virtnet_set_queues(vi, vi->curr_queue_pairs);
+ rtnl_unlock();
/* Assume link up if device can't report link status,
otherwise get link status from config. */
--
2.7.4
^ permalink raw reply related
* Re: [PATCH V2 13/22] bnxt_re: Support QP verbs
From: Selvin Xavier @ 2016-12-13 6:08 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, Eddie Wai, Devesh Sharma,
Somnath Kotur, Sriharsha Basavapatna
In-Reply-To: <20161212182737.GC8204-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
On Mon, Dec 12, 2016 at 11:57 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> It can help to review if you break this function into smaller pieces and
> get rid of switch->switch->if construction.
Thanks Leon. I will address this and your previous comments in v3 patch set.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V2 00/22] Broadcom RoCE Driver (bnxt_re)
From: Selvin Xavier @ 2016-12-13 6:04 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161212170701.GA28387-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
On Mon, Dec 12, 2016 at 10:37 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Sat, Dec 10, 2016 at 11:06:58AM +0530, Selvin Xavier wrote:
>> On Fri, Dec 9, 2016 at 12:17 PM, Selvin Xavier
>> <selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> > I am preparing a git repository with these changes as per Jason's
>> > comment and will share the details later today.
>>
>> Please use bnxt_re branch in this git repository.
>>
>> https://github.com/Broadcom/linux-rdma-nxt.git
>
> Why are you using __packed in bnxt_re_uverbs_abi.h ? that doesn't seem
> necessary. It is a good idea to make sure all those structures are a
> multiple of 64 bits (add explicit reserved fields), and make sure you
> test 32 bit verbs as well.
Will take care in v3.
>
> Why are you using debugfs just to export counters? Isn't the core code
> counter framework good enough?
I agree that some of the counters exported by this patch set, tx and
rx bytes/pkts etc, can be exported
through the core counters. i will try adding this support in v3, if
not, will post as a separate patch.
debugfs was introduced more for the future, in case any HW specific
data needs to be displayed.
As of now, it tracks only the count of resources( CQ/MR/QPs) active at
any given point. So its ok to
skip this patch from this series.
>
> Please try and avoid writing functions as defines (eg rdev_to_dev,
> to_bnxt_re, SQE_PG, RCFW_CMDQ_COOKIE, PTR_PG etc)
>
Sure, will take care in v3.
> There is something wrong with the tabs and spaces (see
> https://github.com/Broadcom/linux-rdma-nxt/blob/03e23b087f7e86ea28656273994e065827210ce5/drivers/infiniband/hw/bnxtre/bnxt_re_hsi.h)
>
> FWIW, I really dislike the column alignment style, it is so hard to
> maintain..
This file contains the Macro defines for the FW/HW structures and are
auto-generated. Some of these auto-generated defines are very long
which makes the lines greater than
80 characters. I will fix whatever possible and include in v3 set.
>
> Jason
Thanks,
Selvin
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* DO YOU NEED A LOAN??
From: bancoleite @ 2016-12-13 5:27 UTC (permalink / raw)
To: Recipients
Are you in need of a loan? Apply for more details.
^ permalink raw reply
* [PATCH iproute2 1/2] tc: flower: Fix typo in the flower man page
From: Roi Dayan @ 2016-12-13 5:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Amir Vadai, Hadar Hen Zion, Roi Dayan
In-Reply-To: <1481607232-1342-1-git-send-email-roid@mellanox.com>
Replace vlan_eth_type with vlan_ethtype.
Fixes: 745d91726006 ("tc: flower: Introduce vlan support")
Signed-off-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Hadar Hen Zion <hadarh@mellanox.com>
---
man/man8/tc-flower.8 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
index 90fdfba..1cea54d 100644
--- a/man/man8/tc-flower.8
+++ b/man/man8/tc-flower.8
@@ -27,7 +27,7 @@ flower \- flow based traffic control filter
.IR VID " | "
.B vlan_prio
.IR PRIORITY " | "
-.BR vlan_eth_type " { " ipv4 " | " ipv6 " | "
+.BR vlan_ethtype " { " ipv4 " | " ipv6 " | "
.IR ETH_TYPE " } | "
.BR ip_proto " { " tcp " | " udp " | " sctp " | " icmp " | " icmpv6 " | "
.IR IP_PROTO " } | { "
@@ -87,7 +87,7 @@ Match on vlan tag priority.
.I PRIORITY
is an unsigned 3bit value in decimal format.
.TP
-.BI vlan_eth_type " VLAN_ETH_TYPE"
+.BI vlan_ethtype " VLAN_ETH_TYPE"
Match on layer three protocol.
.I ETH_TYPE
may be either
--
2.7.4
^ permalink raw reply related
* [PATCH iproute2 0/2] Man page fixes
From: Roi Dayan @ 2016-12-13 5:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Amir Vadai, Hadar Hen Zion, Roi Dayan
Hi,
The 2 patches are man page related only.
First fixes a typo and second adding missing man page to the Makefile.
Thanks
Roi Dayan (2):
tc: flower: Fix typo in the flower man page
tc: tunnel_key: Add tc-tunnel_key man page to Makefile
man/man8/Makefile | 1 +
man/man8/tc-flower.8 | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH iproute2 2/2] tc: tunnel_key: Add tc-tunnel_key man page to Makefile
From: Roi Dayan @ 2016-12-13 5:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Amir Vadai, Hadar Hen Zion, Roi Dayan
In-Reply-To: <1481607232-1342-1-git-send-email-roid@mellanox.com>
To be installed with the other man pages.
Fixes: d57639a475a9 ("tc/act_tunnel: Introduce ip tunnel action")
Signed-off-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Amir Vadai <amir@vadai.me>
---
man/man8/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/man/man8/Makefile b/man/man8/Makefile
index de6f249..d4cb01a 100644
--- a/man/man8/Makefile
+++ b/man/man8/Makefile
@@ -17,6 +17,7 @@ MAN8PAGES = $(TARGETS) ip.8 arpd.8 lnstat.8 routel.8 rtacct.8 rtmon.8 rtpr.8 ss.
tc-tcindex.8 tc-u32.8 tc-matchall.8 \
tc-connmark.8 tc-csum.8 tc-mirred.8 tc-nat.8 tc-pedit.8 tc-police.8 \
tc-simple.8 tc-skbedit.8 tc-vlan.8 tc-xt.8 tc-ife.8 tc-skbmod.8 \
+ tc-tunnel_key.8 \
devlink.8 devlink-dev.8 devlink-monitor.8 devlink-port.8 devlink-sb.8
all: $(TARGETS)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 00/27] Remove VLAN CFI bit abuse
From: Michał Mirosław @ 2016-12-13 5:18 UTC (permalink / raw)
To: netdev
In-Reply-To: <cover.1481586602.git.mirq-linux@rere.qmqm.pl>
On Tue, Dec 13, 2016 at 01:12:32AM +0100, Michał Mirosław wrote:
> Dear NetDevs
>
> This series removes an abuse of VLAN CFI bit in Linux networking stack.
> Currently Linux always clears the bit on outgoing traffic and presents
> it cleared to userspace (even via AF_PACKET/tcpdump when hw-accelerated).
[...]
I just noticed net-next got closed few days ago. I'll resend after it
opens again. Nevertheless, review is appreciated.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH v2] audit: use proper refcount locking on audit_sock
From: Richard Guy Briggs @ 2016-12-13 5:10 UTC (permalink / raw)
To: Paul Moore
Cc: netdev, linux-kernel, linux-audit, edumazet, xiyou.wangcong,
dvyukov
In-Reply-To: <CAHC9VhSjU+-KDSUcjHZAPEwCDbyLYGvkmXYcUqSfSsa1+=DsRw@mail.gmail.com>
On 2016-12-12 15:18, Paul Moore wrote:
> On Mon, Dec 12, 2016 at 5:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Resetting audit_sock appears to be racy.
> >
> > audit_sock was being copied and dereferenced without using a refcount on
> > the source sock.
> >
> > Bump the refcount on the underlying sock when we store a refrence in
> > audit_sock and release it when we reset audit_sock. audit_sock
> > modification needs the audit_cmd_mutex.
> >
> > See: https://lkml.org/lkml/2016/11/26/232
> >
> > Thanks to Eric Dumazet <edumazet@google.com> and Cong Wang
> > <xiyou.wangcong@gmail.com> on ideas how to fix it.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > There has been a lot of change in the audit code that is about to go
> > upstream to address audit queue issues. This patch is based on the
> > source tree: git://git.infradead.org/users/pcmoore/audit#next
> > ---
> > kernel/audit.c | 34 ++++++++++++++++++++++++++++------
> > 1 files changed, 28 insertions(+), 6 deletions(-)
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f20eee0..439f7f3 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -452,7 +452,9 @@ static void auditd_reset(void)
> > struct sk_buff *skb;
> >
> > /* break the connection */
> > + sock_put(audit_sock);
> > audit_pid = 0;
> > + audit_nlk_portid = 0;
> > audit_sock = NULL;
> >
> > /* flush all of the retry queue to the hold queue */
> > @@ -478,6 +480,12 @@ static int kauditd_send_unicast_skb(struct sk_buff *skb)
> > if (rc >= 0) {
> > consume_skb(skb);
> > rc = 0;
> > + } else {
> > + if (rc & (-ENOMEM|-EPERM|-ECONNREFUSED)) {
>
> I dislike the way you wrote this because instead of simply looking at
> this to see if it correct I need to sort out all the bits and find out
> if there are other error codes that could run afoul of this check ...
> make it simple, e.g. (rc == -ENOMEM || rc == -EPERM || ...).
> Actually, since EPERM is 1, -EPERM (-1 in two's compliment is
> 0xffffffff) is going to cause this to be true for pretty much any
> value of rc, yes?
Yes, you are correct. We need there a logical or on the results of each
comparison to the return code rather than bit-wise or-ing the result
codes together first to save a step.
> > + mutex_lock(&audit_cmd_mutex);
> > + auditd_reset();
> > + mutex_unlock(&audit_cmd_mutex);
> > + }
>
> The code in audit#next handles netlink_unicast() errors in
> kauditd_thread() and you are adding error handling code here in
> kauditd_send_unicast_skb() ... that's messy. I don't care too much
> where the auditd_reset() call is made, but let's only do it in one
> function; FWIW, I originally put the error handling code in
> kauditd_thread() because there was other error handling code that
> needed to done in that scope so it resulted in cleaner code.
Hmmm, I seem to remember it not returning the return code and I thought
I had changed it to do so, but I see now that it was already there.
Agreed, I needlessly duplicated that error handling.
> Related, I see you are now considering ENOMEM to be a fatal condition,
> that differs from the AUDITD_BAD macro in kauditd_thread(); this
> difference needs to be reconciled.
Also correct about -EPERM now that I check back to the intent of commit
32a1dbaece7e ("audit: try harder to send to auditd upon netlink
failure")
> Finally, you should update the comment header block for auditd_reset()
> that it needs to be called with the audit_cmd_mutex held.
Yup.
> > @@ -1004,17 +1018,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > return -EACCES;
> > }
> > if (audit_pid && new_pid &&
> > - audit_replace(requesting_pid) != -ECONNREFUSED) {
> > + (audit_replace(requesting_pid) & (-ECONNREFUSED|-EPERM|-ENOMEM))) {
>
> Do we simply want to treat any error here as fatal, and not just
> ECONN/EPERM/ENOMEM? If not, let's come up with a single macro to
> handle the fatal netlink_unicast() return codes so we have some chance
> to keep things consistent in the future.
I'll work through this before I post another patch...
> paul moore
- RGB
^ permalink raw reply
* Re: [PATCH V2 00/22] Broadcom RoCE Driver (bnxt_re)
From: Selvin Xavier @ 2016-12-13 4:52 UTC (permalink / raw)
To: jtoppins-H+wXaHxf7aLQT0dZR+AlfA
Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <9cf03e2b-a16d-19ec-a8ce-14f24272bf6a-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Mon, Dec 12, 2016 at 10:24 PM, Jonathan Toppins <jtoppins-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> CHECK drivers/infiniband/hw/bnxtre/bnxt_re_debugfs.c
> CHECK drivers/infiniband/hw/bnxtre/bnxt_qplib_res.c
> drivers/infiniband/hw/bnxtre/bnxt_qplib_res.c:729:6: warning: symbol
> 'bnxt_qplib_cleanup_pkey_tbl' was not declared. Should it be static?
I will remove this warning in v3 patch set.
> CHECK drivers/infiniband/hw/bnxtre/bnxt_qplib_rcfw.c
> CHECK drivers/infiniband/hw/bnxtre/bnxt_qplib_sp.c
> CHECK drivers/infiniband/hw/bnxtre/bnxt_qplib_fp.c
> drivers/infiniband/hw/bnxtre/bnxt_qplib_fp.c:1015:22: warning: context
> imbalance in 'bnxt_qplib_lock_cqs' - wrong count at exit
> drivers/infiniband/hw/bnxtre/bnxt_qplib_fp.c:1030:28: warning: context
> imbalance in 'bnxt_qplib_unlock_cqs' - unexpected unlock
The above two are false positives, since locking and unlocking are
handled in two separate functions. This is a wrapper to lock/unlock
both SQ and RQ CQ locks. Functionally it is ok since
bnxt_qplib_unlock_cqs is called just after the critical section and
both locks are freed in order. I think we can ignore this warning.
> MODPOST 2 modules
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] audit: use proper refcount locking on audit_sock
From: Richard Guy Briggs @ 2016-12-13 4:49 UTC (permalink / raw)
To: Paul Moore
Cc: netdev, linux-kernel, linux-audit, edumazet, xiyou.wangcong,
dvyukov
In-Reply-To: <CAHC9VhTt5Pbw+LzVwRV1E==drwrH0ihUJvJkWwDgOA35OUFV2g@mail.gmail.com>
On 2016-12-12 12:10, Paul Moore wrote:
> On Mon, Dec 12, 2016 at 5:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Resetting audit_sock appears to be racy.
> >
> > audit_sock was being copied and dereferenced without using a refcount on
> > the source sock.
> >
> > Bump the refcount on the underlying sock when we store a refrence in
> > audit_sock and release it when we reset audit_sock. audit_sock
> > modification needs the audit_cmd_mutex.
> >
> > See: https://lkml.org/lkml/2016/11/26/232
> >
> > Thanks to Eric Dumazet <edumazet@google.com> and Cong Wang
> > <xiyou.wangcong@gmail.com> on ideas how to fix it.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > There has been a lot of change in the audit code that is about to go
> > upstream to address audit queue issues. This patch is based on the
> > source tree: git://git.infradead.org/users/pcmoore/audit#next
> > ---
> > kernel/audit.c | 34 ++++++++++++++++++++++++++++------
> > 1 files changed, 28 insertions(+), 6 deletions(-)
>
> This is coming in pretty late for the v4.10 merge window, much later
> than I would usually take things, but this is arguably important, and
> (at first glance) relatively low risk - what testing have you done on
> this?
At this point, compile and boot, and I'm able to compile and run the
supplied test code without any issues.
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f20eee0..439f7f3 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -452,7 +452,9 @@ static void auditd_reset(void)
> > struct sk_buff *skb;
> >
> > /* break the connection */
> > + sock_put(audit_sock);
> > audit_pid = 0;
> > + audit_nlk_portid = 0;
> > audit_sock = NULL;
> >
> > /* flush all of the retry queue to the hold queue */
> > @@ -478,6 +480,12 @@ static int kauditd_send_unicast_skb(struct sk_buff *skb)
> > if (rc >= 0) {
> > consume_skb(skb);
> > rc = 0;
> > + } else {
> > + if (rc & (-ENOMEM|-EPERM|-ECONNREFUSED)) {
> > + mutex_lock(&audit_cmd_mutex);
> > + auditd_reset();
> > + mutex_unlock(&audit_cmd_mutex);
> > + }
> > }
> >
> > return rc;
> > @@ -579,7 +587,9 @@ static int kauditd_thread(void *dummy)
> >
> > auditd = 0;
> > if (AUDITD_BAD(rc, reschedule)) {
> > + mutex_lock(&audit_cmd_mutex);
> > auditd_reset();
> > + mutex_unlock(&audit_cmd_mutex);
> > reschedule = 0;
> > }
> > } else
> > @@ -594,7 +604,9 @@ static int kauditd_thread(void *dummy)
> > auditd = 0;
> > if (AUDITD_BAD(rc, reschedule)) {
> > kauditd_hold_skb(skb);
> > + mutex_lock(&audit_cmd_mutex);
> > auditd_reset();
> > + mutex_unlock(&audit_cmd_mutex);
> > reschedule = 0;
> > } else
> > /* temporary problem (we hope), queue
> > @@ -623,7 +635,9 @@ quick_loop:
> > if (rc) {
> > auditd = 0;
> > if (AUDITD_BAD(rc, reschedule)) {
> > + mutex_lock(&audit_cmd_mutex);
> > auditd_reset();
> > + mutex_unlock(&audit_cmd_mutex);
> > reschedule = 0;
> > }
> >
> > @@ -1004,17 +1018,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > return -EACCES;
> > }
> > if (audit_pid && new_pid &&
> > - audit_replace(requesting_pid) != -ECONNREFUSED) {
> > + (audit_replace(requesting_pid) & (-ECONNREFUSED|-EPERM|-ENOMEM))) {
> > audit_log_config_change("audit_pid", new_pid, audit_pid, 0);
> > return -EEXIST;
> > }
> > if (audit_enabled != AUDIT_OFF)
> > audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
> > - audit_pid = new_pid;
> > - audit_nlk_portid = NETLINK_CB(skb).portid;
> > - audit_sock = skb->sk;
> > - if (!new_pid)
> > + if (new_pid) {
> > + if (audit_sock)
> > + sock_put(audit_sock);
> > + audit_pid = new_pid;
> > + audit_nlk_portid = NETLINK_CB(skb).portid;
> > + sock_hold(skb->sk);
> > + audit_sock = skb->sk;
> > + } else {
> > auditd_reset();
> > + }
> > wake_up_interruptible(&kauditd_wait);
> > }
> > if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
> > @@ -1283,8 +1302,11 @@ static void __net_exit audit_net_exit(struct net *net)
> > {
> > struct audit_net *aunet = net_generic(net, audit_net_id);
> > struct sock *sock = aunet->nlsk;
> > - if (sock == audit_sock)
> > + if (sock == audit_sock) {
> > + mutex_lock(&audit_cmd_mutex);
> > auditd_reset();
> > + mutex_unlock(&audit_cmd_mutex);
> > + }
> >
> > RCU_INIT_POINTER(aunet->nlsk, NULL);
> > synchronize_net();
> > --
> > 1.7.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH V2 for-next 00/11] Code improvements & fixes for HNS RoCE driver
From: Doug Ledford @ 2016-12-13 4:34 UTC (permalink / raw)
To: Salil Mehta
Cc: xavier.huwei-hv44wF8Li93QT0dZR+AlfA,
oulijun-hv44wF8Li93QT0dZR+AlfA, xushaobo2-hv44wF8Li93QT0dZR+AlfA,
mehta.salil.lnk-Re5JQEeQqe8AvxtiuMwx3w, lijun_nudt-9Onoh4P/yGk,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linuxarm-hv44wF8Li93QT0dZR+AlfA
In-Reply-To: <20161115181053.399568-1-salil.mehta-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1959 bytes --]
On 11/15/2016 1:10 PM, Salil Mehta wrote:
> This patchset introduces some code improvements and fixes
> for the identified problems in the HNS RoCE driver.
>
> Lijun Ou (4):
> IB/hns: Add the interface for querying QP1
> IB/hns: add self loopback for CM
> IB/hns: Modify the condition of notifying hardware loopback
> IB/hns: Fix the bug for qp state in hns_roce_v1_m_qp()
>
> Salil Mehta (1):
> IB/hns: Fix for Checkpatch.pl comment style errors
>
> Shaobo Xu (1):
> IB/hns: Implement the add_gid/del_gid and optimize the GIDs
> management
>
> Wei Hu (Xavier) (5):
> IB/hns: Add code for refreshing CQ CI using TPTR
> IB/hns: Optimize the logic of allocating memory using APIs
> IB/hns: Modify the macro for the timeout when cmd process
> IB/hns: Modify query info named port_num when querying RC QP
> IB/hns: Change qpn allocation to round-robin mode.
>
> drivers/infiniband/hw/hns/hns_roce_alloc.c | 11 +-
> drivers/infiniband/hw/hns/hns_roce_cmd.c | 8 +-
> drivers/infiniband/hw/hns/hns_roce_cmd.h | 7 +-
> drivers/infiniband/hw/hns/hns_roce_common.h | 2 -
> drivers/infiniband/hw/hns/hns_roce_cq.c | 17 +-
> drivers/infiniband/hw/hns/hns_roce_device.h | 45 ++--
> drivers/infiniband/hw/hns/hns_roce_eq.c | 6 +-
> drivers/infiniband/hw/hns/hns_roce_hem.c | 6 +-
> drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 267 +++++++++++++++++------
> drivers/infiniband/hw/hns/hns_roce_hw_v1.h | 17 +-
> drivers/infiniband/hw/hns/hns_roce_main.c | 311 +++++++--------------------
> drivers/infiniband/hw/hns/hns_roce_mr.c | 21 +-
> drivers/infiniband/hw/hns/hns_roce_pd.c | 5 +-
> drivers/infiniband/hw/hns/hns_roce_qp.c | 2 +-
> 14 files changed, 363 insertions(+), 362 deletions(-)
>
Series applied, thanks.
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG Key ID: 0E572FDD
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]
^ permalink raw reply
* Re: "virtio-net: enable multiqueue by default" in linux-next breaks networking on GCE
From: Theodore Ts'o @ 2016-12-13 4:19 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, netdev, nhorman, davem
In-Reply-To: <60cd312f-86f9-47e9-0c72-f4c2109e2f87@redhat.com>
On Tue, Dec 13, 2016 at 11:43:00AM +0800, Jason Wang wrote:
> Thanks for reporting this issue. Looks like I blindly set the affinity
> instead of queues during probe. Could you please try the following patch to
> see if it works?
This fixed things, thanks!!
- Ted
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b425fa1..fe9f772 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1930,7 +1930,9 @@ static int virtnet_probe(struct virtio_device *vdev)
> goto free_unregister_netdev;
> }
>
> - virtnet_set_affinity(vi);
> + rtnl_lock();
> + virtnet_set_queues(vi, vi->curr_queue_pairs);
> + rtnl_unlock();
>
> /* Assume link up if device can't report link status,
> otherwise get link status from config. */
>
>
^ permalink raw reply
* Re: [PATCH V2 00/22] Broadcom RoCE Driver (bnxt_re)
From: Selvin Xavier @ 2016-12-13 3:54 UTC (permalink / raw)
To: Doug Ledford; +Cc: linux-rdma, netdev
In-Reply-To: <23e26353-4317-2836-9f94-d1fc3274a770@redhat.com>
On Tue, Dec 13, 2016 at 5:22 AM, Doug Ledford <dledford@redhat.com> wrote:
>
> There are outstanding review comments to be addressed still yet, and the
> v2 patchset doesn't compile for me in 0day testing. I'm going to bounce
> this one to 4.11.
I will address all review comments and fix the 0day compilation error
and post a v3 soon.
Thanks,
Selvin Xavier
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox