* Re: [PATCH net-next 00/18] net: mvpp2: add RSS support
From: David Miller @ 2018-07-13 0:31 UTC (permalink / raw)
To: maxime.chevallier
Cc: netdev, linux-kernel, antoine.tenart, thomas.petazzoni,
gregory.clement, miquel.raynal, nadavh, stefanc, ymarkman, mw
In-Reply-To: <20180712115427.27375-1-maxime.chevallier@bootlin.com>
From: Maxime Chevallier <maxime.chevallier@bootlin.com>
Date: Thu, 12 Jul 2018 13:54:09 +0200
> This series adds support for RSS on PPv2. There already was some code to
> handle the RSS tables, but the driver was missing all the classification
> steps required to actually use these tables.
>
> RSS is used through the classifier, using at least 2 lookups :
> - One using the C2 engine, a TCAM engine that match the packet based on
> some header extracted fields, assigns the default rx queue for that
> packet and tag it for RSS
> - One using the C3Hx engine, which computes the hash that's used to perform
> the lookup in the RSS table.
>
> Since RSS spreads the load across CPUs, we need to make sure that packets
> from the same flow are always assigned the same rx queue, to prevent
> re-ordering.
>
> This series therefore adds a classification step based on the Header Parser,
> that separate ingress traffic into 52 flows, based on some L2, L3 and L4
> parameters.
>
> Patches 1 and 2 fix some header issues, from the driver splitting
>
> Patches 3 to 7 make sure the correct receive queue setup is used for RSS
>
> Patches 8 to 14 deal with the way we handle the RSS tables
>
> Patch 15 implement basic classifier configuration, by using it to assign the
> default receive queue
>
> Patch 16 implement the ingress traffic splitting into multiple flows
>
> Patch 17 adds RSS support, by using the needed classification steps
>
> Patch 18 adds the required ethtool ops to configure the flow hash parameters
>
> This was tested on MacchiatoBin, giving some nice performance improvements
> using ip forwarding (going from 5Gbps to 9.6Gbps total throughput).
>
> RSS is disabled by default.
Series applied, thank you.
^ permalink raw reply
* Re: [BUG] bonded interfaces drop bpdu (stp) frames
From: Mahesh Bandewar (महेश बंडेवार) @ 2018-07-13 0:15 UTC (permalink / raw)
To: Michal Soltys; +Cc: Jay Vosburgh, Chonggang Li, linux-netdev
In-Reply-To: <836819fc-1a70-1b30-d90c-8c00953b8b6c@ziu.info>
On Thu, Jul 12, 2018 at 4:14 PM, Michal Soltys <soltys@ziu.info> wrote:
> On 2018-07-13 00:03, Jay Vosburgh wrote:
>> Mahesh Bandewar (महेश बंडेवार) wrote:
>>
>>>On Thu, Jul 12, 2018 at 11:03 AM, Jay Vosburgh
>>><jay.vosburgh@canonical.com> wrote:
>>>> Michal Soltys <soltys@ziu.info> wrote:
>>>>
>>>>>On 07/12/2018 04:51 PM, Jay Vosburgh wrote:
>>>>>> Mahesh Bandewar (महेश बंडेवार) wrote:
>>>>>>
>>>>>>> On Wed, Jul 11, 2018 at 3:23 PM, Michal Soltys <soltys@ziu.info> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> As weird as that sounds, this is what I observed today after bumping
>>>>>>>> kernel version. I have a setup where 2 bonds are attached to linux
>>>>>>>> bridge and physically are connected to two switches doing MSTP (and
>>>>>>>> linux bridge is just passing them).
>>>>>>>>
>>>>>>>> Initially I suspected some changes related to bridge code - but quick
>>>>>>>> peek at the code showed nothing suspicious - and the part of it that
>>>>>>>> explicitly passes stp frames if stp is not enabled has seen little
>>>>>>>> changes (e.g. per-port group_fwd_mask added recently). Furthermore - if
>>>>>>>> regular non-bonded interfaces are attached everything works fine.
>>>>>>>>
>>>>>>>> Just to be sure I detached the bond (802.3ad mode) and checked it with
>>>>>>>> simple tcpdump (ether proto \\stp) - and indeed no hello packets were
>>>>>>>> there (with them being present just fine on active enslaved interface,
>>>>>>>> or on the bond device in earlier kernels).
>>>>>>>>
>>>>>>>> If time permits I'll bisect tommorow to pinpoint the commit, but from
>>>>>>>> quick todays test - 4.9.x is working fine, while 4.16.16 (tested on
>>>>>>>> debian) and 4.17.3 (tested on archlinux) are failing.
>>>>>>>>
>>>>>>>> Unless this is already a known issue (or you have any suggestions what
>>>>>>>> could be responsible).
>>>>>>>>
>>>>>>> I believe these are link-local-multicast messages and sometime back a
>>>>>>> change went into to not pass those frames to the bonding master. This
>>>>>>> could be the side effect of that.
>>>>>>
>>>>>> Mahesh, I suspect you're thinking of:
>>>>>>
>>>>>> commit b89f04c61efe3b7756434d693b9203cc0cce002e
>>>>>> Author: Chonggang Li <chonggangli@google.com>
>>>>>> Date: Sun Apr 16 12:02:18 2017 -0700
>>>>>>
>>>>>> bonding: deliver link-local packets with skb->dev set to link that packets arrived on
>>>>>>
>>>>>> Michal, are you able to revert this patch and test?
>>>>>>
>>>>>> -J
>>>>>>
>>>>>> ---
>>>>>> -Jay Vosburgh, jay.vosburgh@canonical.com
>>>>>>
>>>>>
>>>>>
>>>>>Just tested - yes, reverting that patch solves the issues.
>>>>
>>>> Chonggang,
>>>>
>>>> Reading the changelog in your commit referenced above, I'm not
>>>> entirely sure what actual problem it is fixing. Could you elaborate?
>>>>
>>>> As the patch appears to cause a regression, it needs to be
>>>> either fixed or reverted.
>>>>
>>>> Mahesh, you signed-off on it as well, perhaps you also have some
>>>> context?
>>>>
>>>
>>>I think the original idea behind it was to pass the LLDPDUs to the
>>>stack on the interface that they came on since this is considered to
>>>be link-local traffic and passing to bond-master would loose it's
>>>"linklocal-ness". This is true for LLDP and if you change the skb->dev
>>>of the packet, then you don't know which slave link it came on in
>>>(from LLDP consumer's perspective).
>>>
>>>I don't know much about STP but trunking two links and aggregating
>>>this link info through bond-master seems wrong. Just like LLDP, you
>>>are losing info specific to a link and the decision derived from that
>>>info could be wrong.
>>>
>>>Having said that, we determine "linklocal-ness" by looking at L2 and
>>>bondmaster shares this with lts slaves. So it does seem fair to pass
>>>those frames to the bonding-master but at the same time link-local
>>>traffic is supposed to be limited to the physical link (LLDP/STP/LACP
>>>etc). Your thoughts?
>>
>> I agree the whole thing sounds kind of weird, but I'm curious as
>> to what Michal's actual use case is; he presumably has some practical
>> use for this, since he noticed that the behavior changed.
>>
>
> The whole "link-local" term is a bit I don't know - at this point it
> feels like too many things were thrown into single bag and it got
> somewhat confusing (bpdu, lldp, pause frames, lacp, pae, qinq mulitcast
> that afaik has its own address) - I added some examples in another reply
> I did at the same time as you were typing this one =)
>
>> Michal, you mentioned MSTP and using 802.3ad (LACP) mode; how
>> does that combination work rationally given that the bond might send and
>> receive traffic across multiple slaves? Or does the switch side bundle
>> the ports together into a single logical interface for MSTP purposes?
>> On the TX side, I think the bond will likely balance all STP frames to
>> just one slave.
>>
>
> The basic concept - two "main" switches with "important" machines
> connected to those. One switch dies and everything keeps working. With
> no unused ports and so on.
>
> In more details:
>
> Originally I was trying MSTP daemon (on "important" machines) which
> seems quite well and completely coded, but cannot really work correctly
> - as afaik you can't put port (in linux bridge conext) in different
> forwarding/blocking/etc. state per-region - itow per group of vlans (or
> mstpd didn't know how to do that, or it wasn't implemented - I didn't
> look too deep back then, though my interest resurfaced in recent days).
>
> So that option was out of the question. But any switch, real or not,
> /must/ pass bpdu frames if it doesn't interpret them.
I think it's contrary! These (STP) frames are link-local-multicast
frames i.e. their dest-mac is multicast-mac (unlike the one that you
would see on an interface). So a *real* switch would never pass those
frames (because there wont be an entry in the CAM table) and would
have to consume those frames. Just imagine a userspace LACP-daemon
receiving LACPDUs on bonding master, it wouldn't make right decisions
about slaves and hence the health of the aggregator would be
questionable. Also imagine LACP frames being forwards by *real*
switch, things wont work as intended.
> So instead of
> having active mstp participant, we have passive linux bridge that passes
> the frames and the two real switches around that care of mstp, treating
> the linux as a shared segment. The costs/priorities/etc. on the real
> swtiches are set so one bond handles two regions, and the other bond
> handles other two regions. If any of the real switches dies or is taken
> down for e.g. firmware update - the bond going to the other switch
> handles all four regions (failover is of course not as fast as with
> active rstp/mstp participation, but works quite well none the less -
> around 10s after some tuning).
>
> We could have used RSTP for that purpose as well - but that being all or
> nothing in context of per-port blocking/forwarding, would leave half of
> the ports unused - and we wanted to avoid that (that's why MSTP was
> created after all).
>
> Instead of using 2 bonds (2 interfaces each) we could just use 4
> interfaces directly, one per region. But two of those regions see very
> little traffic, so we put more and less active regions in pairs.
>
>> As for a resolution, presuming that Michal has some reasonable
>> use case, I'm thinking along the lines of reverting the new (leave frame
>> attached to slave) behavior for the general case and adding a special
>> case for LLDP and friends to get the new behavior. I'd like to avoid
>> adding any new options to bonding.
>>
>
> My use case aside, this will cause issues for anyone attaching bond
> (instead of direct interface or veth) to a bridge and doing something
> more complex with it - whether related to stp or to selectively passing
> e.g. lldp using group_fwd_mask sysfs. Or having LLDP daemon (e.g.
> systemd-resolvd to not look far away) told to do LLDP on bond device
> (even most basic active-backup case) and remaining blind. Or anything
> else that expects to see/pass those multicasts on/via bonded device
> (which is just a convenient way to create virtual interface out of real
> interfaces after all - ITOW shouldn't probably make any calls in this
> regard).
A real switch cannot forward link-local-multicast and if it does, then
I would consider it as broken. Having said that we are taking about
the bonding-master and as I mentioned earlier, it seems fair to pass
those frames to bonding master because we share L2 with slaves but we
should pass the frames to the stack as they are (current behavior!) to
maintain the "linklocal-ness" so both master and the current-slave get
a copy (it's multicast after all).
Thanks,
--mahesh..
^ permalink raw reply
* Proposal
From: Miss Victoria Mehmet @ 2018-07-12 23:32 UTC (permalink / raw)
To: Recipients
Hello
I have a business proposal of mutual benefits i would like to discuss with
you i asked before and i still await your positive response thanks
^ permalink raw reply
* Re: [PATCH] liquidio: Use %pad printk format for dma_addr_t values
From: Felix Manlunas @ 2018-07-13 0:06 UTC (permalink / raw)
To: Helge Deller
Cc: Derek Chickles, Satanand Burla, Felix Manlunas, Raghu Vatsavayi,
netdev, linux-parisc
In-Reply-To: <20180712203629.GA32537@ls3530.fritz.box>
On Thu, Jul 12, 2018 at 10:36:29PM +0200, Helge Deller wrote:
> Use the existing %pad printk format to print dma_addr_t values.
> This avoids the following warnings when compiling on the parisc platform:
>
> warning: format '%llx' expects argument of type 'long long unsigned int', but argument 2 has type 'dma_addr_t {aka unsigned int}' [-Wformat=]
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/drivers/net/ethernet/cavium/liquidio/request_manager.c b/drivers/net/ethernet/cavium/liquidio/request_manager.c
> index 1f2e75da28f8..d5d9e47daa4b 100644
> --- a/drivers/net/ethernet/cavium/liquidio/request_manager.c
> +++ b/drivers/net/ethernet/cavium/liquidio/request_manager.c
> @@ -110,8 +110,8 @@ int octeon_init_instr_queue(struct octeon_device *oct,
>
> memset(iq->request_list, 0, sizeof(*iq->request_list) * num_descs);
>
> - dev_dbg(&oct->pci_dev->dev, "IQ[%d]: base: %p basedma: %llx count: %d\n",
> - iq_no, iq->base_addr, iq->base_addr_dma, iq->max_count);
> + dev_dbg(&oct->pci_dev->dev, "IQ[%d]: base: %p basedma: %pad count: %d\n",
> + iq_no, iq->base_addr, &iq->base_addr_dma, iq->max_count);
>
> iq->txpciq.u64 = txpciq.u64;
> iq->fill_threshold = (u32)conf->db_min;
Acked-by: Felix Manlunas <felix.manlunas@cavium.com>
^ permalink raw reply
* Re: [PATCH net-next] net: gro: properly remove skb from list
From: David Miller @ 2018-07-13 0:02 UTC (permalink / raw)
To: bhole_prashant_q7; +Cc: brouer, tyhicks, netdev
In-Reply-To: <20180712072459.8800-1-bhole_prashant_q7@lab.ntt.co.jp>
From: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Date: Thu, 12 Jul 2018 16:24:59 +0900
> Following crash occurs in validate_xmit_skb_list() when same skb is
> iterated multiple times in the loop and consume_skb() is called.
>
> The root cause is calling list_del_init(&skb->list) and not clearing
> skb->next in d4546c2509b1. list_del_init(&skb->list) sets skb->next
> to point to skb itself. skb->next needs to be cleared because other
> parts of network stack uses another kind of SKB lists.
> validate_xmit_skb_list() uses such list.
>
> A similar type of bugfix was reported by Jesper Dangaard Brouer.
> https://patchwork.ozlabs.org/patch/942541/
>
> This patch clears skb->next and changes list_del_init() to list_del()
> so that list->prev will maintain the list poison.
...
> Fixes: d4546c2509b1 ("net: Convert GRO SKB handling to list_head.")
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> Reported-by: Tyler Hicks <tyhicks@canonical.com>
Applied, thank you.
Hopefully we can convert more layers to list_head SKB usage, and
thus no longer need hacks like this.
Thanks.
^ permalink raw reply
* Re: [PATCH net] selftests: in udpgso_bench do not test udp zerocopy
From: David Miller @ 2018-07-12 23:57 UTC (permalink / raw)
To: willemdebruijn.kernel; +Cc: netdev, willemb
In-Reply-To: <20180711160046.198091-3-willemdebruijn.kernel@gmail.com>
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed, 11 Jul 2018 12:00:46 -0400
> From: Willem de Bruijn <willemb@google.com>
>
> The udpgso benchmark compares various configurations of UDP and TCP.
> Including one that is not upstream, udp zerocopy. This is a leftover
> from the earlier RFC patchset.
>
> The test is part of kselftests and run in continuous spinners. Remove
> the failing case to make the test start passing.
>
> Fixes: 3a687bef148d ("selftests: udp gso benchmark")
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] packet: reset network header if packet shorter than ll reserved space
From: David Miller @ 2018-07-12 23:57 UTC (permalink / raw)
To: willemdebruijn.kernel; +Cc: netdev, willemb
In-Reply-To: <20180711160046.198091-2-willemdebruijn.kernel@gmail.com>
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed, 11 Jul 2018 12:00:45 -0400
> From: Willem de Bruijn <willemb@google.com>
>
> If variable length link layer headers result in a packet shorter
> than dev->hard_header_len, reset the network header offset. Else
> skb->mac_len may exceed skb->len after skb_mac_reset_len.
>
> packet_sendmsg_spkt already has similar logic.
>
> Fixes: b84bbaf7a6c8 ("packet: in packet_snd start writing at link layer allocation")
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net] nsh: set mac len based on inner packet
From: David Miller @ 2018-07-12 23:57 UTC (permalink / raw)
To: willemdebruijn.kernel; +Cc: netdev, willemb
In-Reply-To: <20180711160046.198091-1-willemdebruijn.kernel@gmail.com>
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed, 11 Jul 2018 12:00:44 -0400
> From: Willem de Bruijn <willemb@google.com>
>
> When pulling the NSH header in nsh_gso_segment, set the mac length
> based on the encapsulated packet type.
>
> skb_reset_mac_len computes an offset to the network header, which
> here still points to the outer packet:
>
> > skb_reset_network_header(skb);
> > [...]
> > __skb_pull(skb, nsh_len);
> > skb_reset_mac_header(skb); // now mac hdr starts nsh_len == 8B after net hdr
> > skb_reset_mac_len(skb); // mac len = net hdr - mac hdr == (u16) -8 == 65528
> > [..]
> > skb_mac_gso_segment(skb, ..)
>
> Link: http://lkml.kernel.org/r/CAF=yD-KeAcTSOn4AxirAxL8m7QAS8GBBe1w09eziYwvPbbUeYA@mail.gmail.com
> Reported-by: syzbot+7b9ed9872dab8c32305d@syzkaller.appspotmail.com
> Fixes: c411ed854584 ("nsh: add GSO support")
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Applied and queued up for -stable.
^ permalink raw reply
* [net-next, 2/3] tcp: convert icsk_user_timeout from jiffies to msecs
From: Jon Maxwell @ 2018-07-13 0:02 UTC (permalink / raw)
To: davem
Cc: edumazet, ncardwell, David.Laight, kuznet, yoshfuji, netdev,
linux-kernel, jmaxwell
Create a seperate helper routine called tcp_retransmit_stamp() as per Neal
Cardwells suggestion. To be used by the final commit in this series and
retransmits_timed_out().
Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
---
net/ipv4/tcp_timer.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index fa34984d0b12..8ab8c9645294 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -22,6 +22,20 @@
#include <linux/gfp.h>
#include <net/tcp.h>
+u32 tcp_retransmit_stamp(const struct sock *sk)
+{
+ u32 start_ts = tcp_sk(sk)->retrans_stamp;
+
+ if (unlikely(!start_ts)) {
+ struct sk_buff *head = tcp_rtx_queue_head(sk);
+
+ if (!head)
+ return 0;
+ start_ts = tcp_skb_timestamp(head);
+ }
+ return start_ts;
+}
+
/**
* tcp_write_err() - close socket and save error info
* @sk: The socket the error has appeared on.
@@ -166,14 +180,9 @@ static bool retransmits_timed_out(struct sock *sk,
if (!inet_csk(sk)->icsk_retransmits)
return false;
- start_ts = tcp_sk(sk)->retrans_stamp;
- if (unlikely(!start_ts)) {
- struct sk_buff *head = tcp_rtx_queue_head(sk);
-
- if (!head)
- return false;
- start_ts = tcp_skb_timestamp(head);
- }
+ start_ts = tcp_retransmit_stamp(sk);
+ if (!start_ts)
+ return false;
if (likely(timeout == 0)) {
linear_backoff_thresh = ilog2(TCP_RTO_MAX/rto_base);
--
2.13.6
^ permalink raw reply related
* Re: [PATCH net-next 00/10] s390/qeth: updates 2018-07-11
From: David Miller @ 2018-07-12 23:43 UTC (permalink / raw)
To: jwi; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, ubraun
In-Reply-To: <20180711154247.5523-1-jwi@linux.ibm.com>
From: Julian Wiedmann <jwi@linux.ibm.com>
Date: Wed, 11 Jul 2018 17:42:37 +0200
> please apply this first batch of qeth patches for net-next. It brings the
> usual cleanups, and some performance improvements to the transmit paths.
Series applied, thank you.
^ permalink raw reply
* Re: [net-next PATCH] net: ipv4: fix listify ip_rcv_finish in case of forwarding
From: David Miller @ 2018-07-12 23:41 UTC (permalink / raw)
To: brouer; +Cc: ecree, netdev
In-Reply-To: <153132125549.13161.16380200872856218805.stgit@firesoul>
From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Wed, 11 Jul 2018 17:01:20 +0200
> In commit 5fa12739a53d ("net: ipv4: listify ip_rcv_finish") calling
> dst_input(skb) was split-out. The ip_sublist_rcv_finish() just calls
> dst_input(skb) in a loop.
>
> The problem is that ip_sublist_rcv_finish() forgot to remove the SKB
> from the list before invoking dst_input(). Further more we need to
> clear skb->next as other parts of the network stack use another kind
> of SKB lists for xmit_more (see dev_hard_start_xmit).
>
> A crash occurs if e.g. dst_input() invoke ip_forward(), which calls
> dst_output()/ip_output() that eventually calls __dev_queue_xmit() +
> sch_direct_xmit(), and a crash occurs in validate_xmit_skb_list().
>
> This patch only fixes the crash, but there is a huge potential for
> a performance boost if we can pass an SKB-list through to ip_forward.
>
> Fixes: 5fa12739a53d ("net: ipv4: listify ip_rcv_finish")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> Only driver sfc actually uses this, but I don't have this NIC, so I
> tested this on mlx5, with my own changes to make it use netif_receive_skb_list(),
> but I'm not ready to upstream the mlx5 driver change yet.
Applied, thanks Jesper.
This whole:
list_del();
skb->next = NULL;
business is exactly the kind of dragons I was worried about when starting
to use list_head with SKBs.
There is a similar fix wrt. the GRO stuff that I'm about to apply as well.
It definitely is better if we don't have to forcefully hand off NULL
->next next pointers like this in the long term.
^ permalink raw reply
* Re: [BUG] bonded interfaces drop bpdu (stp) frames
From: Michal Soltys @ 2018-07-12 23:14 UTC (permalink / raw)
To: Jay Vosburgh,
Mahesh Bandewar (महेश बंडेवार)
Cc: Chonggang Li, linux-netdev
In-Reply-To: <30874.1531433032@nyx>
On 2018-07-13 00:03, Jay Vosburgh wrote:
> Mahesh Bandewar (महेश बंडेवार) wrote:
>
>>On Thu, Jul 12, 2018 at 11:03 AM, Jay Vosburgh
>><jay.vosburgh@canonical.com> wrote:
>>> Michal Soltys <soltys@ziu.info> wrote:
>>>
>>>>On 07/12/2018 04:51 PM, Jay Vosburgh wrote:
>>>>> Mahesh Bandewar (महेश बंडेवार) wrote:
>>>>>
>>>>>> On Wed, Jul 11, 2018 at 3:23 PM, Michal Soltys <soltys@ziu.info> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> As weird as that sounds, this is what I observed today after bumping
>>>>>>> kernel version. I have a setup where 2 bonds are attached to linux
>>>>>>> bridge and physically are connected to two switches doing MSTP (and
>>>>>>> linux bridge is just passing them).
>>>>>>>
>>>>>>> Initially I suspected some changes related to bridge code - but quick
>>>>>>> peek at the code showed nothing suspicious - and the part of it that
>>>>>>> explicitly passes stp frames if stp is not enabled has seen little
>>>>>>> changes (e.g. per-port group_fwd_mask added recently). Furthermore - if
>>>>>>> regular non-bonded interfaces are attached everything works fine.
>>>>>>>
>>>>>>> Just to be sure I detached the bond (802.3ad mode) and checked it with
>>>>>>> simple tcpdump (ether proto \\stp) - and indeed no hello packets were
>>>>>>> there (with them being present just fine on active enslaved interface,
>>>>>>> or on the bond device in earlier kernels).
>>>>>>>
>>>>>>> If time permits I'll bisect tommorow to pinpoint the commit, but from
>>>>>>> quick todays test - 4.9.x is working fine, while 4.16.16 (tested on
>>>>>>> debian) and 4.17.3 (tested on archlinux) are failing.
>>>>>>>
>>>>>>> Unless this is already a known issue (or you have any suggestions what
>>>>>>> could be responsible).
>>>>>>>
>>>>>> I believe these are link-local-multicast messages and sometime back a
>>>>>> change went into to not pass those frames to the bonding master. This
>>>>>> could be the side effect of that.
>>>>>
>>>>> Mahesh, I suspect you're thinking of:
>>>>>
>>>>> commit b89f04c61efe3b7756434d693b9203cc0cce002e
>>>>> Author: Chonggang Li <chonggangli@google.com>
>>>>> Date: Sun Apr 16 12:02:18 2017 -0700
>>>>>
>>>>> bonding: deliver link-local packets with skb->dev set to link that packets arrived on
>>>>>
>>>>> Michal, are you able to revert this patch and test?
>>>>>
>>>>> -J
>>>>>
>>>>> ---
>>>>> -Jay Vosburgh, jay.vosburgh@canonical.com
>>>>>
>>>>
>>>>
>>>>Just tested - yes, reverting that patch solves the issues.
>>>
>>> Chonggang,
>>>
>>> Reading the changelog in your commit referenced above, I'm not
>>> entirely sure what actual problem it is fixing. Could you elaborate?
>>>
>>> As the patch appears to cause a regression, it needs to be
>>> either fixed or reverted.
>>>
>>> Mahesh, you signed-off on it as well, perhaps you also have some
>>> context?
>>>
>>
>>I think the original idea behind it was to pass the LLDPDUs to the
>>stack on the interface that they came on since this is considered to
>>be link-local traffic and passing to bond-master would loose it's
>>"linklocal-ness". This is true for LLDP and if you change the skb->dev
>>of the packet, then you don't know which slave link it came on in
>>(from LLDP consumer's perspective).
>>
>>I don't know much about STP but trunking two links and aggregating
>>this link info through bond-master seems wrong. Just like LLDP, you
>>are losing info specific to a link and the decision derived from that
>>info could be wrong.
>>
>>Having said that, we determine "linklocal-ness" by looking at L2 and
>>bondmaster shares this with lts slaves. So it does seem fair to pass
>>those frames to the bonding-master but at the same time link-local
>>traffic is supposed to be limited to the physical link (LLDP/STP/LACP
>>etc). Your thoughts?
>
> I agree the whole thing sounds kind of weird, but I'm curious as
> to what Michal's actual use case is; he presumably has some practical
> use for this, since he noticed that the behavior changed.
>
The whole "link-local" term is a bit I don't know - at this point it
feels like too many things were thrown into single bag and it got
somewhat confusing (bpdu, lldp, pause frames, lacp, pae, qinq mulitcast
that afaik has its own address) - I added some examples in another reply
I did at the same time as you were typing this one =)
> Michal, you mentioned MSTP and using 802.3ad (LACP) mode; how
> does that combination work rationally given that the bond might send and
> receive traffic across multiple slaves? Or does the switch side bundle
> the ports together into a single logical interface for MSTP purposes?
> On the TX side, I think the bond will likely balance all STP frames to
> just one slave.
>
The basic concept - two "main" switches with "important" machines
connected to those. One switch dies and everything keeps working. With
no unused ports and so on.
In more details:
Originally I was trying MSTP daemon (on "important" machines) which
seems quite well and completely coded, but cannot really work correctly
- as afaik you can't put port (in linux bridge conext) in different
forwarding/blocking/etc. state per-region - itow per group of vlans (or
mstpd didn't know how to do that, or it wasn't implemented - I didn't
look too deep back then, though my interest resurfaced in recent days).
So that option was out of the question. But any switch, real or not,
/must/ pass bpdu frames if it doesn't interpret them. So instead of
having active mstp participant, we have passive linux bridge that passes
the frames and the two real switches around that care of mstp, treating
the linux as a shared segment. The costs/priorities/etc. on the real
swtiches are set so one bond handles two regions, and the other bond
handles other two regions. If any of the real switches dies or is taken
down for e.g. firmware update - the bond going to the other switch
handles all four regions (failover is of course not as fast as with
active rstp/mstp participation, but works quite well none the less -
around 10s after some tuning).
We could have used RSTP for that purpose as well - but that being all or
nothing in context of per-port blocking/forwarding, would leave half of
the ports unused - and we wanted to avoid that (that's why MSTP was
created after all).
Instead of using 2 bonds (2 interfaces each) we could just use 4
interfaces directly, one per region. But two of those regions see very
little traffic, so we put more and less active regions in pairs.
> As for a resolution, presuming that Michal has some reasonable
> use case, I'm thinking along the lines of reverting the new (leave frame
> attached to slave) behavior for the general case and adding a special
> case for LLDP and friends to get the new behavior. I'd like to avoid
> adding any new options to bonding.
>
My use case aside, this will cause issues for anyone attaching bond
(instead of direct interface or veth) to a bridge and doing something
more complex with it - whether related to stp or to selectively passing
e.g. lldp using group_fwd_mask sysfs. Or having LLDP daemon (e.g.
systemd-resolvd to not look far away) told to do LLDP on bond device
(even most basic active-backup case) and remaining blind. Or anything
else that expects to see/pass those multicasts on/via bonded device
(which is just a convenient way to create virtual interface out of real
interfaces after all - ITOW shouldn't probably make any calls in this
regard).
^ permalink raw reply
* Proposal
From: Miss Victoria Mehmet @ 2018-07-12 22:46 UTC (permalink / raw)
To: Recipients
Hello
I have a business proposal of mutual benefits i would like to discuss with
you.
^ permalink raw reply
* Re: [PATCH v2 net-next 7/9] lan743x: Add EEE support
From: Andrew Lunn @ 2018-07-12 22:49 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-8-git-send-email-Bryan.Whitehead@microchip.com>
> +static int lan743x_ethtool_set_eee(struct net_device *netdev,
> + struct ethtool_eee *eee)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> + struct phy_device *phydev = NULL;
> + u32 buf = 0;
> + int ret = 0;
> +
> + if (!netdev)
> + return -EINVAL;
> + adapter = netdev_priv(netdev);
> + if (!adapter)
> + return -EINVAL;
> + phydev = netdev->phydev;
> + if (!phydev)
> + return -EIO;
> + if (!phydev->drv) {
> + netif_err(adapter, drv, adapter->netdev,
> + "Missing PHY Driver\n");
> + return -EIO;
> + }
> +
> + if (eee->eee_enabled) {
> + ret = phy_init_eee(phydev, 0);
> + if (ret) {
> + netif_err(adapter, drv, adapter->netdev,
> + "EEE initialization failed\n");
> + return ret;
> + }
> +
> + buf = lan743x_csr_read(adapter, MAC_CR);
> + buf |= MAC_CR_EEE_EN_;
> + lan743x_csr_write(adapter, MAC_CR, buf);
> +
> + phy_ethtool_set_eee(phydev, eee);
> +
> + buf = (u32)eee->tx_lpi_timer;
> + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
> + netif_info(adapter, drv, adapter->netdev, "Enabled EEE\n");
> + } else {
> + buf = lan743x_csr_read(adapter, MAC_CR);
> + buf &= ~MAC_CR_EEE_EN_;
> + lan743x_csr_write(adapter, MAC_CR, buf);
> + netif_info(adapter, drv, adapter->netdev, "Disabled EEE\n");
> + }
> +
Hi Bryan
You should call phy_ethtool_set_eee() in both cases, so that it gets
disabled in the PHY as well. It needs to stop advertising it.
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 6/9] lan743x: Add power management support
From: Andrew Lunn @ 2018-07-12 22:42 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-7-git-send-email-Bryan.Whitehead@microchip.com>
> +#ifdef CONFIG_PM
> +static void lan743x_ethtool_get_wol(struct net_device *netdev,
> + struct ethtool_wolinfo *wol)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> +
> + wol->supported = WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
> + WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
> +
> + wol->wolopts = adapter->wolopts;
> +}
> +#endif /* CONFIG_PM */
> +
> +#ifdef CONFIG_PM
> +static int lan743x_ethtool_set_wol(struct net_device *netdev,
> + struct ethtool_wolinfo *wol)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> +
> + if (wol->wolopts & WAKE_MAGICSECURE)
> + return -EOPNOTSUPP;
> +
> + adapter->wolopts = 0;
> + if (wol->wolopts & WAKE_UCAST)
> + adapter->wolopts |= WAKE_UCAST;
> + if (wol->wolopts & WAKE_MCAST)
> + adapter->wolopts |= WAKE_MCAST;
> + if (wol->wolopts & WAKE_BCAST)
> + adapter->wolopts |= WAKE_BCAST;
> + if (wol->wolopts & WAKE_MAGIC)
> + adapter->wolopts |= WAKE_MAGIC;
> + if (wol->wolopts & WAKE_PHY)
> + adapter->wolopts |= WAKE_PHY;
> + if (wol->wolopts & WAKE_ARP)
> + adapter->wolopts |= WAKE_ARP;
> +
> + device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
> +
> + phy_ethtool_set_wol(netdev->phydev, wol);
Hi Bryan
This seems asymmetric. set_wol you call into the phylib to enable wol
in the PHY. But get_wol does not call into phylib. So the phy has no
chance to set what it supports.
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 5/9] lan743x: Add support for ethtool eeprom access
From: Andrew Lunn @ 2018-07-12 22:35 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-6-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:05:02PM -0400, Bryan Whitehead wrote:
> Implement ethtool eeprom access
> Also provides access to OTP (One Time Programming)
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 4/9] lan743x: Add support for ethtool message level
From: Andrew Lunn @ 2018-07-12 22:31 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-5-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:05:01PM -0400, Bryan Whitehead wrote:
> Implement ethtool message level
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 3/9] lan743x: Add support for ethtool statistics
From: Andrew Lunn @ 2018-07-12 22:31 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-4-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:05:00PM -0400, Bryan Whitehead wrote:
> Implement ethtool statistics
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 2/9] lan743x: Add support for ethtool link settings
From: Andrew Lunn @ 2018-07-12 22:29 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-3-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:04:59PM -0400, Bryan Whitehead wrote:
> Use default link setting functions
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next 1/9] lan743x: Add support for ethtool get_drvinfo
From: Andrew Lunn @ 2018-07-12 22:28 UTC (permalink / raw)
To: Bryan Whitehead; +Cc: davem, netdev, UNGLinuxDriver, richardcochran
In-Reply-To: <1531422306-6547-2-git-send-email-Bryan.Whitehead@microchip.com>
On Thu, Jul 12, 2018 at 03:04:58PM -0400, Bryan Whitehead wrote:
> Implement ethtool get_drvinfo
>
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net] net: Don't copy pfmemalloc flag in __copy_skb_header()
From: David Miller @ 2018-07-12 22:15 UTC (permalink / raw)
To: sbrivio; +Cc: mgorman, eric.dumazet, fw, ptalbert, netdev
In-Reply-To: <93db92329a9964c336965c166e5c858cf46cd0a5.1531305883.git.sbrivio@redhat.com>
From: Stefano Brivio <sbrivio@redhat.com>
Date: Wed, 11 Jul 2018 14:39:42 +0200
> The pfmemalloc flag indicates that the skb was allocated from
> the PFMEMALLOC reserves, and the flag is currently copied on skb
> copy and clone.
>
> However, an skb copied from an skb flagged with pfmemalloc
> wasn't necessarily allocated from PFMEMALLOC reserves, and on
> the other hand an skb allocated that way might be copied from an
> skb that wasn't.
>
> So we should not copy the flag on skb copy, and rather decide
> whether to allow an skb to be associated with sockets unrelated
> to page reclaim depending only on how it was allocated.
>
> Move the pfmemalloc flag before headers_start[0] using an
> existing 1-bit hole, so that __copy_skb_header() doesn't copy
> it.
>
> When cloning, we'll now take care of this flag explicitly,
> contravening to the warning comment of __skb_clone().
>
> While at it, restore the newline usage introduced by commit
> b19372273164 ("net: reorganize sk_buff for faster
> __copy_skb_header()") to visually separate bytes used in
> bitfields after headers_start[0], that was gone after commit
> a9e419dc7be6 ("netfilter: merge ctinfo into nfct pointer storage
> area"), and describe the pfmemalloc flag in the kernel-doc
> structure comment.
>
> This doesn't change the size of sk_buff or cacheline boundaries,
> but consolidates the 15 bits hole before tc_index into a 2 bytes
> hole before csum, that could now be filled more easily.
>
> Reported-by: Patrick Talbert <ptalbert@redhat.com>
> Fixes: c93bdd0e03e8 ("netvm: allow skb allocation to use PFMEMALLOC reserves")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Applied and queued up for -stable, thank you.
^ permalink raw reply
* Re: [BUG] bonded interfaces drop bpdu (stp) frames
From: Michal Soltys @ 2018-07-12 22:14 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार),
Jay Vosburgh
Cc: Chonggang Li, linux-netdev
In-Reply-To: <CAF2d9jjXmiVZwDu-q+M5bmdrgyMr0U_tiUzxoCUiWwZT4NS5Cw@mail.gmail.com>
On 2018-07-12 23:26, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Thu, Jul 12, 2018 at 11:03 AM, Jay Vosburgh
> <jay.vosburgh@canonical.com> wrote:
>> Michal Soltys <soltys@ziu.info> wrote:
>>
>>>On 07/12/2018 04:51 PM, Jay Vosburgh wrote:
>>>> Mahesh Bandewar (महेश बंडेवार) wrote:
>>>>
>>>>> On Wed, Jul 11, 2018 at 3:23 PM, Michal Soltys <soltys@ziu.info> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> As weird as that sounds, this is what I observed today after bumping
>>>>>> kernel version. I have a setup where 2 bonds are attached to linux
>>>>>> bridge and physically are connected to two switches doing MSTP (and
>>>>>> linux bridge is just passing them).
>>>>>>
>>>>>> Initially I suspected some changes related to bridge code - but quick
>>>>>> peek at the code showed nothing suspicious - and the part of it that
>>>>>> explicitly passes stp frames if stp is not enabled has seen little
>>>>>> changes (e.g. per-port group_fwd_mask added recently). Furthermore - if
>>>>>> regular non-bonded interfaces are attached everything works fine.
>>>>>>
>>>>>> Just to be sure I detached the bond (802.3ad mode) and checked it with
>>>>>> simple tcpdump (ether proto \\stp) - and indeed no hello packets were
>>>>>> there (with them being present just fine on active enslaved interface,
>>>>>> or on the bond device in earlier kernels).
>>>>>>
>>>>>> If time permits I'll bisect tommorow to pinpoint the commit, but from
>>>>>> quick todays test - 4.9.x is working fine, while 4.16.16 (tested on
>>>>>> debian) and 4.17.3 (tested on archlinux) are failing.
>>>>>>
>>>>>> Unless this is already a known issue (or you have any suggestions what
>>>>>> could be responsible).
>>>>>>
>>>>> I believe these are link-local-multicast messages and sometime back a
>>>>> change went into to not pass those frames to the bonding master. This
>>>>> could be the side effect of that.
>>>>
>>>> Mahesh, I suspect you're thinking of:
>>>>
>>>> commit b89f04c61efe3b7756434d693b9203cc0cce002e
>>>> Author: Chonggang Li <chonggangli@google.com>
>>>> Date: Sun Apr 16 12:02:18 2017 -0700
>>>>
>>>> bonding: deliver link-local packets with skb->dev set to link that packets arrived on
>>>>
>>>> Michal, are you able to revert this patch and test?
>>>>
>>>> -J
>>>>
>>>> ---
>>>> -Jay Vosburgh, jay.vosburgh@canonical.com
>>>>
>>>
>>>
>>>Just tested - yes, reverting that patch solves the issues.
>>
>> Chonggang,
>>
>> Reading the changelog in your commit referenced above, I'm not
>> entirely sure what actual problem it is fixing. Could you elaborate?
>>
>> As the patch appears to cause a regression, it needs to be
>> either fixed or reverted.
>>
>> Mahesh, you signed-off on it as well, perhaps you also have some
>> context?
>>
>
> I think the original idea behind it was to pass the LLDPDUs to the
> stack on the interface that they came on since this is considered to
> be link-local traffic and passing to bond-master would loose it's
> "linklocal-ness". This is true for LLDP and if you change the skb->dev
> of the packet, then you don't know which slave link it came on in
> (from LLDP consumer's perspective).
>
> I don't know much about STP but trunking two links and aggregating
> this link info through bond-master seems wrong. Just like LLDP, you
> are losing info specific to a link and the decision derived from that
> info could be wrong.
>
> Having said that, we determine "linklocal-ness" by looking at L2 and
> bondmaster shares this with lts slaves. So it does seem fair to pass
> those frames to the bonding-master but at the same time link-local
> traffic is supposed to be limited to the physical link (LLDP/STP/LACP
> etc). Your thoughts?
>
But, isn't bond de-facto considered the "physical link" ? Not directly
of course, but say an LLDP daemon would likely be more interested in
getting LLDP data from a bond device (or a bridge device, if the bond is
attached to one), than from its enslaved interfaces (and enslaved
interfaces can be changed, not mentioning potentially complex setup
itself, even if usually it's just lacp&go ).
ITOW, blocking link-local multicasts on bond level (among those - bpdu,
pae, lldp) is a bit like if the interface itself hid LACP before bond code.
A few other examples:
- putting bonds in a bridge is pretty normal thing - and whether the
bridge interpretes the spanning tree data itself (via in-kernel classic
stp or userspace daemon for e.g. rstp) or passes the trafic, it must see
the BPDU frames. Otherwise it becomes blind to the whole spanning tree
protocol - and implicitly other switches around - real or virtual ones.
It's literally instant loop disaster. br_input.c specifically takes care
to pass those frames if the bridge has stp turned off
- "group_fwd_mask" (again in bridge context) has been added to bridge
code - and recently as a per-port knob as well - to specifically allow
the control of what kind of "link-local" stuff is passed or not. LLDP
and 802.1X PAE were, afaik, the main reasons for that sysfs variable.
The per-port setting is even more relaxed (iirc, only pause frames are
not passable)
- LLDP daemon example - as above
^ permalink raw reply
* Re: [PATCH bpf] bpf: fix some bad __rcu annotations in bpf/core.c
From: Roman Gushchin @ 2018-07-12 22:26 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, linux-kernel, kernel-team, Alexei Starovoitov, yhs
In-Reply-To: <31c2b14a-8890-0ebd-bb59-01616e9d8d5d@iogearbox.net>
On Tue, Jul 10, 2018 at 10:03:19AM +0200, Daniel Borkmann wrote:
> Hi Roman,
>
> On 07/10/2018 03:59 AM, Roman Gushchin wrote:
> > Sparse shows some "incorrect type" warnings in the bpf core code.
>
> Thanks for taking a stab at these! It would really help if you could
> split the patch into a small series and fix each individual case that
> is problematic here.
>
> Please also add Fixes tags to the patches.
Sure.
The only problem which I have with these sparse warnings,
is that my cgroup local storage patchset touches some of
these lines, and I'm receiving automatic complains.
>
> More below.
>
> > They are caused by bad __rcu annotations:
> > 1) bpf_prog_array_alloc() returns an __rcu pointer, which isn't true.
> > At that moment it's obviously an exclusive "owning" pointer,
> > which is valid for an infinite amount of time, so __rcu is
> > meaningless.
> > 2) The progs local variable in compute_effective_progs should be
> > marked as __bpf too, it's a local variable, not shared with anyone
>
> Typo: __bpf ?
Yep, fixed.
>
> > else at all. The real __rcu variable is array pointer, which should
> > be assigned with rcu_assign_pointer.
> > 3) __rcu progs argument of bpf_prog_array_free() should be casted
> > to a simple pointer before calling kfree_rcu().
> > 4) There is a missing rcu_dereference() annotation in
> > bpf_prog_array_copy_to_user().
> > 5) old_array __rcu pointer in bpf_prog_array_copy() is used as
> > a "normal" non-__rcu pointer.
> >
> > These changes remove the following sparse warnings:
> > kernel/bpf/core.c:1544:31: warning: incorrect type in return expression (different address spaces)
> > kernel/bpf/core.c:1544:31: expected struct bpf_prog_array [noderef] <asn:4>*
> > kernel/bpf/core.c:1544:31: got void *
> > kernel/bpf/core.c:1548:17: warning: incorrect type in return expression (different address spaces)
> > kernel/bpf/core.c:1548:17: expected struct bpf_prog_array [noderef] <asn:4>*
> > kernel/bpf/core.c:1548:17: got struct bpf_prog_array *<noident>
> > kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
> > kernel/bpf/core.c:1556:9: expected struct callback_head *head
> > kernel/bpf/core.c:1556:9: got struct callback_head [noderef] <asn:4>*<noident>
> > kernel/bpf/core.c:1629:34: warning: incorrect type in initializer (different address spaces)
> > kernel/bpf/core.c:1629:34: expected struct bpf_prog **prog
> > kernel/bpf/core.c:1629:34: got struct bpf_prog *[noderef] <asn:4>*<noident>
> > kernel/bpf/core.c:1653:31: warning: incorrect type in assignment (different address spaces)
> > kernel/bpf/core.c:1653:31: expected struct bpf_prog **existing_prog
> > kernel/bpf/core.c:1653:31: got struct bpf_prog *[noderef] <asn:4>*<noident>
> > kernel/bpf/core.c:1681:15: warning: incorrect type in assignment (different address spaces)
> > kernel/bpf/core.c:1681:15: expected struct bpf_prog_array *array
> > kernel/bpf/core.c:1681:15: got struct bpf_prog_array [noderef] <asn:4>*
> > kernel/bpf/core.c:1687:31: warning: incorrect type in assignment (different address spaces)
> > kernel/bpf/core.c:1687:31: expected struct bpf_prog **[assigned] existing_prog
> > kernel/bpf/core.c:1687:31: got struct bpf_prog *[noderef] <asn:4>*<noident>
> >
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > ---
> > include/linux/bpf.h | 2 +-
> > kernel/bpf/cgroup.c | 7 +++----
> > kernel/bpf/core.c | 14 ++++++++------
> > 3 files changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 8827e797ff97..943fb08d8287 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -352,7 +352,7 @@ struct bpf_prog_array {
> > struct bpf_prog *progs[0];
> > };
> >
> > -struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
> > +struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags);
> > void bpf_prog_array_free(struct bpf_prog_array __rcu *progs);
> > int bpf_prog_array_length(struct bpf_prog_array __rcu *progs);
> > int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > index 3d83ee7df381..badabb0b435c 100644
> > --- a/kernel/bpf/cgroup.c
> > +++ b/kernel/bpf/cgroup.c
> > @@ -95,7 +95,7 @@ static int compute_effective_progs(struct cgroup *cgrp,
> > enum bpf_attach_type type,
> > struct bpf_prog_array __rcu **array)
> > {
> > - struct bpf_prog_array __rcu *progs;
> > + struct bpf_prog_array *progs;
> > struct bpf_prog_list *pl;
> > struct cgroup *p = cgrp;
> > int cnt = 0;
> > @@ -120,13 +120,12 @@ static int compute_effective_progs(struct cgroup *cgrp,
> > &p->bpf.progs[type], node) {
> > if (!pl->prog)
> > continue;
> > - rcu_dereference_protected(progs, 1)->
> > - progs[cnt++] = pl->prog;
> > + progs->progs[cnt++] = pl->prog;
> > }
> > p = cgroup_parent(p);
> > } while (p);
> >
> > - *array = progs;
> > + rcu_assign_pointer(*array, progs);
> > return 0;
> > }
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 1e5625d46414..f6e5b207a0d7 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -1538,7 +1538,7 @@ static struct {
> > .null_prog = NULL,
> > };
> >
> > -struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
> > +struct bpf_prog_array *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
> > {
> > if (prog_cnt)
> > return kzalloc(sizeof(struct bpf_prog_array) +
> > @@ -1550,10 +1550,11 @@ struct bpf_prog_array __rcu *bpf_prog_array_alloc(u32 prog_cnt, gfp_t flags)
>
> Looks good to me to here.
>
> > void bpf_prog_array_free(struct bpf_prog_array __rcu *progs)
> > {
> > - if (!progs ||
> > - progs == (struct bpf_prog_array __rcu *)&empty_prog_array.hdr)
> > + struct bpf_prog_array *array = rcu_access_pointer(progs);
>
> Can you elaborate on the rcu_access_pointer() part? This looks odd, at minimum
> this needs a comment explaining why it's needed. Is the __rcu annotation above
> even correct?
No, it's not. But fixing it causes to use rcu_access_pointer()
for almost every bpf_prog_array_delete_safe() call.
Still better probably, will go this path in v2.
>
> > +
> > + if (!array || array == &empty_prog_array.hdr)
> > return;
> > - kfree_rcu(progs, rcu);
> > + kfree_rcu(array, rcu);
> > }
> >
> > int bpf_prog_array_length(struct bpf_prog_array __rcu *progs)
> > @@ -1626,7 +1627,7 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
> > void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
> > struct bpf_prog *old_prog)
> > {
> > - struct bpf_prog **prog = progs->progs;
> > + struct bpf_prog **prog = rcu_dereference(progs)->progs;
>
> Can you elaborate here as well? __rcu annotation buggy instead?
But here it's fine. Please, look at bpf_prog_array_length() and bpf_prog_array_copy_to_user().
Same applies here.
If we want to be more precise, the "progs" field in the bpf_prog_array should
also be marked with __rcu, but this is beyond the scope of this patch(set).
>
> > for (; *prog; prog++)
> > if (*prog == old_prog) {
> > @@ -1635,11 +1636,12 @@ void bpf_prog_array_delete_safe(struct bpf_prog_array __rcu *progs,
> > }
> > }
> >
> > -int bpf_prog_array_copy(struct bpf_prog_array __rcu *old_array,
> > +int bpf_prog_array_copy(struct bpf_prog_array __rcu *__old_array,
> > struct bpf_prog *exclude_prog,
> > struct bpf_prog *include_prog,
> > struct bpf_prog_array **new_array)
> > {
> > + struct bpf_prog_array *old_array = rcu_access_pointer(__old_array);
>
> Same comment here, this doesn't look right. We even fetch old_array->progs
> from it later on in this path.
Same here. old_array->progs field is not marked with __bpf currently.
Thanks!
^ permalink raw reply
* Re: Re: [Qemu-devel] [PATCH v3 0/3] Use of unique identifier for pairing virtio and passthrough devices...
From: Siwei Liu @ 2018-07-12 22:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Cornelia Huck, si-wei liu, Roman Kagan, Venu Busireddy,
Marcel Apfelbaum, virtio-dev, qemu-devel, Samudrala, Sridhar,
Alexander Duyck, Netdev
In-Reply-To: <20180712235500-mutt-send-email-mst@kernel.org>
On Thu, Jul 12, 2018 at 2:00 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jul 12, 2018 at 01:52:53PM -0700, Siwei Liu wrote:
>> The definition is incomplete due to lack of spec. There's no "host"
>> part defined yet in the host-guest interface. If match by MAC is an
>> interface, the same must be done on the host(device) side as well,
>> which has been agreed not the way to go. However, I don't think that's
>> what the author intends to do by interpreting his QEMU patch - it
>> missed the other parts as well, such as the feature negotiation and
>> how it interacts with the paired device.
>>
>> What I said is that match by MAC is just a guest implementation that
>> one can change at any time. We now have the group ID on QEMU, why
>> still sticking to matching by MAC? It shoulnd't be a host-guest
>> interface in the first place anyway.
>
> I think that match by MAC is a simple portable way to match devices.
> E.g. it will work seamlessly with niche things like zPCI. However
That's a good point. I'm not sure if it's a valid assumption that zPCI
should always use the same MAC address as that of virtio. Someone
who's more familiar with the use case may decide and work on that. It
means VFIO device has to take in the MAC address as an identifier to
the "-device vfio-pci,.." QEMU option. I think there's no point to
match device using group ID in QEMU while using MAC in the guest.
Based on that assumption, I'd go with making VIRTIO_NET_F_STANDBY to
match device based on group ID, while someone may come up with another
feature bit later, say VIRTIO_NET_F_STANDBY_BY_MAC when its QEMU
support is available. Would it make sense?
-Siwei
> there are other niche use-cases that aren't addressed by match by MAC
> such as PF pass-through as a primary, and the pci bridge trick addresses
> that at cost of some portability.
>
> So I see no issues supporting both mechanisms, but others on the TC
> might feel differently.
>
> --
> MST
^ permalink raw reply
* Re: [PATCH mlx5-next v1 2/8] net/mlx5: Add support for flow table destination number
From: Jason Gunthorpe @ 2018-07-12 22:05 UTC (permalink / raw)
To: Or Gerlitz
Cc: Yishai Hadas, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Saeed Mahameed, linux-netdev
In-Reply-To: <CAJ3xEMg-cfRwZTwK2Eu2cdH1YuSF5rJy88YWYrbyWywKun5hPg@mail.gmail.com>
On Fri, Jul 13, 2018 at 12:51:10AM +0300, Or Gerlitz wrote:
> On Fri, Jul 13, 2018 at 12:26 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> > On Fri, Jul 13, 2018 at 12:00:41AM +0300, Or Gerlitz wrote:
> >> On Wed, Jul 11, 2018 at 2:10 PM, Leon Romanovsky <leon@kernel.org> wrote:
> >> > From: Yishai Hadas <yishaih@mellanox.com>
> >> >
> >> > Add support to set a destination from a flow table number.
> >> > This functionality will be used in downstream patches from this
> >> > series by the DEVX stuff.
> >>
> >> Reading your cover letter, I still don't understand what is missing
> >> in the current mlx5 fs core API for your needs. After all, you do
> >> create flow tables from the IB driver through fs core calls, right?
> >> so @ the end of the day, you have the FT pointer to provide the
> >> core, why you need the FT number?
> >
> > Via the devx API userspace can create flow tables directly without
> > going to the driver's flow steering core.
>
> so why you change the core?
User space flow tables don't get any traffic until they are linked
into the main steering. The only ID the kernel gets for them when
adding this link is the actual PRM handle, not a pointer - hence the
change.
Jason
^ 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