* mlx4 w/ IOMMU broken, kernel 3.12.6
From: David Lamparter @ 2014-01-08 13:49 UTC (permalink / raw)
To: Amir Vadai; +Cc: netdev
Hi,
mlx4 is currently broken when an IOMMU is enabled, the driver does not
seem to set up regions correctly (and also, the card seems to access things
before its driver is loaded):
(This is the in-kernel driver. The separately distributed 1.5.10
Mellanox driver does not seem to build against kernel 3.12.6.)
[ 1.897271] IOMMU 0 0xfbffc000: using Queued invalidation
[ 1.897569] IOMMU: Setting RMRR:
[ 1.897872] IOMMU: Setting identity map for device 0000:00:1d.0 [0x7dea2000 - 0x7deaefff]
[ 1.898415] IOMMU: Setting identity map for device 0000:00:1a.0 [0x7dea2000 - 0x7deaefff]
[ 1.898930] IOMMU: Prepare 0-16MiB unity mapping for LPC
[ 1.899217] IOMMU: Setting identity map for device 0000:00:1f.0 [0x0 - 0xffffff]
[ 1.899726] PCI-DMA: Intel(R) Virtualization Technology for Directed I/O
[ 1.902086] dmar: DRHD: handling fault status reg 2
[ 1.902362] dmar: DMAR:[DMA Read] Request device [06:00.0] fault addr 72469000
DMAR:[fault reason 01] Present bit in root entry is clear
(...)
[ 3.299985] mlx4_core: Mellanox ConnectX core driver v1.1 (Dec, 2011)
[ 3.300247] mlx4_core: Initializing 0000:06:00.0
[ 4.306091] dmar: DRHD: handling fault status reg 102
[ 4.306359] dmar: DMAR:[DMA Read] Request device [06:00.0] fault addr 7236f000
DMAR:[fault reason 06] PTE Read access is not set
[ 14.309294] mlx4_core 0000:06:00.0: command 0x4 timed out (go bit not cleared)
[ 14.309759] mlx4_core 0000:06:00.0: QUERY_FW command failed, aborting.
[ 15.313896] mlx4_core: probe of 0000:06:00.0 failed with error -110
(PCI/IOMMU Host is an Intel Xeon E5-2630v2)
Since the driver hasn't been touched in net-next, I assume this issue
hasn't been fixed in the time since 3.12 was released; my apologies if
this is incorrect.
Card information:
# lspci -vvnns 06:00.0
06:00.0 Ethernet controller [0200]: Mellanox Technologies MT27500 Family [ConnectX-3] [15b3:1003]
Subsystem: Mellanox Technologies Device [15b3:0077]
Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ >SERR- <PERR- INTx-
Interrupt: pin A routed to IRQ 42
Region 0: Memory at fba00000 (64-bit, non-prefetchable) [size=1M]
Region 2: Memory at 380fff000000 (64-bit, prefetchable) [size=8M]
Expansion ROM at fb900000 [disabled] [size=1M]
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [48] Vital Product Data
pcilib: sysfs_read_vpd: read failed: Connection timed out
Not readable
Capabilities: [9c] MSI-X: Enable- Count=128 Masked-
Vector table: BAR=0 offset=0007c000
PBA: BAR=0 offset=0007d000
Capabilities: [60] Express (v2) Endpoint, MSI 00
DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 unlimited
ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset+
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- FLReset-
MaxPayload 256 bytes, MaxReadReq 512 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
LnkCap: Port #8, Speed 8GT/s, Width x8, ASPM L0s, Exit Latency L0s unlimited, L1 unlimited
ClockPM- Surprise- LLActRep- BwNot-
LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 8GT/s, Width x8, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
DevCap2: Completion Timeout: Range ABCD, TimeoutDis+, LTR-, OBFF Not Supported
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-, OBFF Disabled
LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-
Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1-
EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
Capabilities: [100 v1] Alternative Routing-ID Interpretation (ARI)
ARICap: MFVC- ACS-, Next Function: 0
ARICtl: MFVC- ACS-, Function Group: 0
Capabilities: [148 v1] Device Serial Number 00-02-c9-03-00-ea-72-e0
Capabilities: [154 v2] Advanced Error Reporting
UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UESvrt: DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
AERCap: First Error Pointer: 00, GenCap- CGenEn- ChkCap- ChkEn-
Capabilities: [18c v1] #19
Cheers,
David
^ permalink raw reply
* Re: [PATCH -next] openvswitch: Use kmem_cache_free() instead of kfree()
From: Jesse Gross @ 2014-01-08 14:17 UTC (permalink / raw)
To: Wei Yongjun
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org, netdev, David Miller,
Wei Yongjun
In-Reply-To: <CAPgLHd_jaxf4jsvUzBhE_RFioHgZzvmWhA=d_J5phUOhBGy49g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jan 8, 2014 at 5:13 AM, Wei Yongjun <weiyj.lk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
>
> memory allocated by kmem_cache_alloc() should be freed using
> kmem_cache_free(), not kfree().
>
> Fixes: e298e5057006 ('openvswitch: Per cpu flow stats.')
> Signed-off-by: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
Good catch, thanks.
I'll let David pick this up directly since I just flushed my patch queue.
Acked-by: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply
* Re: [net-next 4/8] i40evf: virtual channel interface
From: Ben Hutchings @ 2014-01-08 14:16 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: davem, Greg Rose, netdev, gospo, sassmann, Mitch Williams
In-Reply-To: <1388537594-24601-5-git-send-email-jeffrey.t.kirsher@intel.com>
On Tue, 2013-12-31 at 16:53 -0800, Jeff Kirsher wrote:
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c
[...]
> +void i40evf_virtchnl_completion(struct i40evf_adapter *adapter,
> + enum i40e_virtchnl_ops v_opcode,
> + i40e_status v_retval,
> + u8 *msg, u16 msglen)
> +{
> + struct net_device *netdev = adapter->netdev;
> +
> + if (v_opcode == I40E_VIRTCHNL_OP_EVENT) {
> + struct i40e_virtchnl_pf_event *vpe =
> + (struct i40e_virtchnl_pf_event *)msg;
> + switch (vpe->event) {
> + case I40E_VIRTCHNL_EVENT_LINK_CHANGE:
> + adapter->link_up =
> + vpe->event_data.link_event.link_status;
> + if (adapter->link_up && !netif_carrier_ok(netdev)) {
> + dev_info(&adapter->pdev->dev, "NIC Link is Up\n");
> + netif_carrier_on(netdev);
> + netif_tx_wake_all_queues(netdev);
> + } else if (!adapter->link_up) {
> + dev_info(&adapter->pdev->dev, "NIC Link is Down\n");
> + netif_carrier_off(netdev);
> + netif_tx_stop_all_queues(netdev);
> + }
[...]
The queue start/stop here should be redundant as linkwatch will do it
automatically.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [net-next 3/8] i40evf: core ethtool functionality
From: Ben Hutchings @ 2014-01-08 14:14 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: davem, Greg Rose, netdev, gospo, sassmann, Mitch Williams
In-Reply-To: <1388537594-24601-4-git-send-email-jeffrey.t.kirsher@intel.com>
On Tue, 2013-12-31 at 16:53 -0800, Jeff Kirsher wrote:
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
[...]
> +#define I40EVF_QUEUE_STATS_LEN \
> + (((struct i40evf_adapter *) \
> + netdev_priv(netdev))->vsi_res->num_queue_pairs * 4)
netdev should be a macro parameter, not just assumed as a local
variable.
[...]
> +static int i40evf_get_settings(struct net_device *netdev,
> + struct ethtool_cmd *ecmd)
> +{
> + /* In the future the VF will be able to query the PF for
> + * some information - for now use a dummy value
> + */
> + ecmd->supported = SUPPORTED_10000baseT_Full;
> + ecmd->autoneg = AUTONEG_DISABLE;
> + ecmd->transceiver = XCVR_DUMMY1;
> + ecmd->port = PORT_NONE;
This is not even consistent, as its claims that 1000BASE-T is supported
but speed/duplex are always set to 0. Why not set supported = 0?
> + return 0;
> +}
[...]
> +static int i40evf_get_sset_count(struct net_device *netdev, int sset)
> +{
> + if (sset == ETH_SS_STATS)
> + return I40EVF_STATS_LEN;
> + else
> + return -ENOTSUPP;
-EINVAL
> +}
[...]
> +static int i40evf_set_coalesce(struct net_device *netdev,
> + struct ethtool_coalesce *ec)
> +{
> + struct i40evf_adapter *adapter = netdev_priv(netdev);
> + struct i40e_hw *hw = &adapter->hw;
> + struct i40e_vsi *vsi = &adapter->vsi;
> + struct i40e_q_vector *q_vector;
> + int i;
> +
> + if (ec->tx_max_coalesced_frames || ec->rx_max_coalesced_frames)
> + vsi->work_limit = ec->tx_max_coalesced_frames;
Why is the actual value of ec->rx_max_coalesced_frames ignored here?
Should this be min() or max() of the two fields?
> + switch (ec->rx_coalesce_usecs) {
> + case 0:
> + vsi->rx_itr_setting = 0;
> + break;
> + case 1:
> + vsi->rx_itr_setting = (I40E_ITR_DYNAMIC
> + | ITR_REG_TO_USEC(I40E_ITR_RX_DEF));
> + break;
This looks a bit magic; why is 1 us interpreted as 'dynamic' (adaptive?)
when there is a separate flag for enabling adaptive moderation?
[...]
> +static struct ethtool_ops i40evf_ethtool_ops = {
[...]
Should be const.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: IPv6: Bug in net-next
From: Hannes Frederic Sowa @ 2014-01-08 14:13 UTC (permalink / raw)
To: François-Xavier Le Bail, netdev, davem
In-Reply-To: <20140108140400.GH9007@order.stressinduktion.org>
On Wed, Jan 08, 2014 at 03:04:00PM +0100, Hannes Frederic Sowa wrote:
> On Wed, Jan 08, 2014 at 05:35:39AM -0800, François-Xavier Le Bail wrote:
> > On Wed, 1/8/14, François-Xavier Le Bail <fx.lebail@yahoo.com> wrote:
> >
> > > > I think there is a bug in actual net-next.
> >
> > > > When I execute this code and press Crtl-C, all the IPv6
> > > > Link-Layer addresses are deleted.
> >
> > > > This happened between
> > > > c1ddf295f5183a5189196a8035546842caa2055a and HEAD.
> >
> > > > [...]
> >
> > > Sorry, it is not linked to this code.
> > > I'm looking for the reason ...
> >
> > It seems that the valid lifetime and preferred lft are set to 0 sec for autoconfigured LLA.
>
> Grrr, I guess we should switch in addrconf_add_linklocal to call ipv6_add_addr
> with INFINITY_LIFE_TIME preferred and valid lft. IFA_PERMANENT may now expire,
> too since commit fad8da3e0 ("ipv6 addrconf: fix preferred lifetime
> state-changing behavior while valid_lft is infinity").
I am testing this patch:
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 31f75ea..f7e86e3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2528,7 +2528,8 @@ static void add_addr(struct inet6_dev *idev, const struct in6_addr *addr,
struct inet6_ifaddr *ifp;
ifp = ipv6_add_addr(idev, addr, NULL, plen,
- scope, IFA_F_PERMANENT, 0, 0);
+ scope, IFA_F_PERMANENT,
+ INFINITY_LIFE_TIME, INFINITY_LIFE_TIME);
if (!IS_ERR(ifp)) {
spin_lock_bh(&ifp->lock);
ifp->flags &= ~IFA_F_TENTATIVE;
@@ -2656,7 +2657,8 @@ static void addrconf_add_linklocal(struct inet6_dev *idev, const struct in6_addr
#endif
- ifp = ipv6_add_addr(idev, addr, NULL, 64, IFA_LINK, addr_flags, 0, 0);
+ ifp = ipv6_add_addr(idev, addr, NULL, 64, IFA_LINK, addr_flags,
+ INFINITY_LIFE_TIME, INFINITY_LIFE_TIME);
if (!IS_ERR(ifp)) {
addrconf_prefix_route(&ifp->addr, ifp->prefix_len, idev->dev, 0, 0);
addrconf_dad_start(ifp);
^ permalink raw reply related
* Re: [PATCH net-next v3 6/9] xen-netback: Handle guests with too many frags
From: Zoltan Kiss @ 2014-01-08 14:13 UTC (permalink / raw)
To: Eric Dumazet
Cc: ian.campbell, wei.liu2, xen-devel, netdev, linux-kernel,
jonathan.davies
In-Reply-To: <1389189272.26646.89.camel@edumazet-glaptop2.roam.corp.google.com>
On 08/01/14 13:54, Eric Dumazet wrote:
> On Wed, 2014-01-08 at 13:49 +0000, Zoltan Kiss wrote:
>> On 08/01/14 02:12, Eric Dumazet wrote:
>>> On Wed, 2014-01-08 at 00:10 +0000, Zoltan Kiss wrote:
>>>
>>>>
>>>> + if (skb_shinfo(skb)->frag_list) {
>>>> + nskb = skb_shinfo(skb)->frag_list;
>>>> + xenvif_fill_frags(vif, nskb, INVALID_PENDING_IDX);
>>>> + skb->len += nskb->len;
>>>> + skb->data_len += nskb->len;
>>>> + skb->truesize += nskb->truesize;
>>>> + skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
>>>> + skb_shinfo(nskb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
>>>> + vif->tx_zerocopy_sent += 2;
>>>> + nskb = skb;
>>>> +
>>>> + skb = skb_copy_expand(skb,
>>>> + 0,
>>>> + 0,
>>>> + GFP_ATOMIC | __GFP_NOWARN);
>>>
>>> skb can be NULL here
>>
>> Thanks, fixed that.
>
> BTW, I am not sure why you copy the skb.
>
> Is it to get rid of frag_list, and why ?
Yes, it is to get rid of the frag_list, just to be on the safe side. I'm
not sure if it is normal to send a big skb with MAX_SKB_FRAGS frags plus
an empty skb on the frag_list with one frag, so I just consolidate them
here. This scenario shouldn't happen very often anyway, even guests
which can send more than MAX_SKB_FRAGS slots tends to do it rarely.
Zoli
^ permalink raw reply
* Re: [PATCH net-next v3 1/9] xen-netback: Introduce TX grant map definitions
From: Zoltan Kiss @ 2014-01-08 14:08 UTC (permalink / raw)
To: David Miller
Cc: ian.campbell, wei.liu2, xen-devel, netdev, linux-kernel,
jonathan.davies
In-Reply-To: <20140107.202951.729942261773265015.davem@davemloft.net>
On 08/01/14 01:29, David Miller wrote:
>> +static inline int tx_dealloc_work_todo(struct xenvif *vif)
>
> Make this return bool.
Done, also in the last patch.
>> + wait_event_interruptible(vif->dealloc_wq,
>> + tx_dealloc_work_todo(vif) ||
>> + kthread_should_stop());
>
> Inconsistent indentation. You should make the arguments line up at
> exactly the first column after the openning parenthesis of the function
> call.
Done, thanks.
Zoli
^ permalink raw reply
* Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get
From: Florian Westphal @ 2014-01-08 14:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: Florian Westphal, Andrey Vagin, netfilter-devel, netfilter,
coreteam, netdev, linux-kernel, vvs, Pablo Neira Ayuso,
Patrick McHardy, Jozsef Kadlecsik, David S. Miller,
Cyrill Gorcunov
In-Reply-To: <1389188536.26646.84.camel@edumazet-glaptop2.roam.corp.google.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > This will also set up a null-binding when no matching SNAT/DNAT/MASQERUADE
> > rule existed.
> >
> > The manipulations of the skb->nfct->ext nat area are performed without
> > a lock. Concurrent access is supposedly impossible as the conntrack
> > should not (yet) be in the hash table.
> >
> > The confirmed bit is set right before we insert the conntrack into
> > the hash table (after we traversed rules, ct is ready to be
> > 'published').
> >
> > i.e. when the confirmed bit is NOT set we should not be 'seeing' the nf_conn
> > struct when we perform the lookup, as it should still be sitting on the
> > 'unconfirmed' list, being invisible to readers.
> >
> > Does that explanation make sense to you?
> >
> > Thanks for looking into this.
>
> Still, this patch adds a loop. And maybe an infinite one if confirmed
> bit is set from an context that was interrupted by this one.
Hmm. There should be at most one retry.
The confirmed bit should always be set here.
If it isn't then this conntrack shouldn't be in the hash table, i.e.
when we re-try we should find the same conntrack again with the bit set.
Asuming the other cpu git interrupted after setting confirmed
bit but before inserting it into the hash table, then our re-try
should not be able find a matching entry.
Maybe I am missing something, but I don't see how we could (upon
retry) find the very same entry again with the bit still not set.
> If you need to test the confirmed bit, then you also need to test it
> before taking the refcount.
I don't think that would make sense, because it should always be
set (inserting conntrack into hash table without confirmed set is
illegal, and it is never unset again).
[ when allocating a new conntrack, ct->status is zeroed, which also
clears the flag. This happens just before we set the new objects
refcount to 1 ]
^ permalink raw reply
* Re: IPv6: Bug in net-next
From: Hannes Frederic Sowa @ 2014-01-08 14:04 UTC (permalink / raw)
To: François-Xavier Le Bail; +Cc: netdev, davem
In-Reply-To: <1389188139.44648.YahooMailBasic@web125506.mail.ne1.yahoo.com>
On Wed, Jan 08, 2014 at 05:35:39AM -0800, François-Xavier Le Bail wrote:
> On Wed, 1/8/14, François-Xavier Le Bail <fx.lebail@yahoo.com> wrote:
>
> > > I think there is a bug in actual net-next.
>
> > > When I execute this code and press Crtl-C, all the IPv6
> > > Link-Layer addresses are deleted.
>
> > > This happened between
> > > c1ddf295f5183a5189196a8035546842caa2055a and HEAD.
>
> > > [...]
>
> > Sorry, it is not linked to this code.
> > I'm looking for the reason ...
>
> It seems that the valid lifetime and preferred lft are set to 0 sec for autoconfigured LLA.
Grrr, I guess we should switch in addrconf_add_linklocal to call ipv6_add_addr
with INFINITY_LIFE_TIME preferred and valid lft. IFA_PERMANENT may now expire,
too since commit fad8da3e0 ("ipv6 addrconf: fix preferred lifetime
state-changing behavior while valid_lft is infinity").
Greetings,
Hannes
^ permalink raw reply
* Re: IPv6: Bug in net-next
From: Damien Wyart @ 2014-01-08 13:54 UTC (permalink / raw)
To: François-Xavier Le Bail; +Cc: netdev, davem
In-Reply-To: <1389188139.44648.YahooMailBasic@web125506.mail.ne1.yahoo.com>
> > > When I execute this code and press Crtl-C, all the IPv6 Link-Layer
> > > addresses are deleted.
> It seems that the valid lifetime and preferred lft are set to 0 sec
> for autoconfigured LLA.
I am also seeing the problem with Linus' up-to-date tree (3.13-rc7+).
On eth0, LLA is not present after boot, as well as ::1 (Host scope) on
the loopback (which causes some daemons to not start properly as they
try to listen on ::1). Manually setting these adresses with ip addr
works, they do not disappear afterwards.
--
Damien
^ permalink raw reply
* [PATCH -next] ip_tunnel: fix sparse non static symbol warning
From: Wei Yongjun @ 2014-01-08 13:59 UTC (permalink / raw)
To: davem, kuznet, jmorris, yoshfuji, kaber, therbert; +Cc: yongjun_wei, netdev
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Fixes the following sparse warning:
net/ipv4/ip_tunnel.c:116:18: warning:
symbol 'tunnel_dst_check' was not declared. Should it be static?
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
net/ipv4/ip_tunnel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 07a5ed3..d3929a6 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -113,7 +113,7 @@ static inline struct dst_entry *tunnel_dst_get(struct ip_tunnel *t)
return dst;
}
-struct dst_entry *tunnel_dst_check(struct ip_tunnel *t, u32 cookie)
+static struct dst_entry *tunnel_dst_check(struct ip_tunnel *t, u32 cookie)
{
struct dst_entry *dst = tunnel_dst_get(t);
^ permalink raw reply related
* [PATCH net-next 2/3] xen-netback: use new skb_checksum_setup function
From: Paul Durrant @ 2014-01-08 13:58 UTC (permalink / raw)
To: netdev, xen-devel; +Cc: Paul Durrant, Ian Campbell, Wei Liu
In-Reply-To: <1389189511-14568-1-git-send-email-paul.durrant@citrix.com>
Use skb_checksum_setup to set up partial checksum offsets rather
then a private implementation.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
drivers/net/xen-netback/netback.c | 260 +------------------------------------
1 file changed, 3 insertions(+), 257 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 4f81ac0..2605405 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -39,7 +39,6 @@
#include <linux/udp.h>
#include <net/tcp.h>
-#include <net/ip6_checksum.h>
#include <xen/xen.h>
#include <xen/events.h>
@@ -1048,257 +1047,9 @@ static int xenvif_set_skb_gso(struct xenvif *vif,
return 0;
}
-static inline int maybe_pull_tail(struct sk_buff *skb, unsigned int len,
- unsigned int max)
-{
- if (skb_headlen(skb) >= len)
- return 0;
-
- /* If we need to pullup then pullup to the max, so we
- * won't need to do it again.
- */
- if (max > skb->len)
- max = skb->len;
-
- if (__pskb_pull_tail(skb, max - skb_headlen(skb)) == NULL)
- return -ENOMEM;
-
- if (skb_headlen(skb) < len)
- return -EPROTO;
-
- return 0;
-}
-
-/* This value should be large enough to cover a tagged ethernet header plus
- * maximally sized IP and TCP or UDP headers.
- */
-#define MAX_IP_HDR_LEN 128
-
-static int checksum_setup_ip(struct xenvif *vif, struct sk_buff *skb,
- int recalculate_partial_csum)
-{
- unsigned int off;
- bool fragment;
- int err;
-
- fragment = false;
-
- err = maybe_pull_tail(skb,
- sizeof(struct iphdr),
- MAX_IP_HDR_LEN);
- if (err < 0)
- goto out;
-
- if (ip_hdr(skb)->frag_off & htons(IP_OFFSET | IP_MF))
- fragment = true;
-
- off = ip_hdrlen(skb);
-
- err = -EPROTO;
-
- if (fragment)
- goto out;
-
- switch (ip_hdr(skb)->protocol) {
- case IPPROTO_TCP:
- err = maybe_pull_tail(skb,
- off + sizeof(struct tcphdr),
- MAX_IP_HDR_LEN);
- if (err < 0)
- goto out;
-
- if (!skb_partial_csum_set(skb, off,
- offsetof(struct tcphdr, check))) {
- err = -EPROTO;
- goto out;
- }
-
- if (recalculate_partial_csum)
- tcp_hdr(skb)->check =
- ~csum_tcpudp_magic(ip_hdr(skb)->saddr,
- ip_hdr(skb)->daddr,
- skb->len - off,
- IPPROTO_TCP, 0);
- break;
- case IPPROTO_UDP:
- err = maybe_pull_tail(skb,
- off + sizeof(struct udphdr),
- MAX_IP_HDR_LEN);
- if (err < 0)
- goto out;
-
- if (!skb_partial_csum_set(skb, off,
- offsetof(struct udphdr, check))) {
- err = -EPROTO;
- goto out;
- }
-
- if (recalculate_partial_csum)
- udp_hdr(skb)->check =
- ~csum_tcpudp_magic(ip_hdr(skb)->saddr,
- ip_hdr(skb)->daddr,
- skb->len - off,
- IPPROTO_UDP, 0);
- break;
- default:
- goto out;
- }
-
- err = 0;
-
-out:
- return err;
-}
-
-/* This value should be large enough to cover a tagged ethernet header plus
- * an IPv6 header, all options, and a maximal TCP or UDP header.
- */
-#define MAX_IPV6_HDR_LEN 256
-
-#define OPT_HDR(type, skb, off) \
- (type *)(skb_network_header(skb) + (off))
-
-static int checksum_setup_ipv6(struct xenvif *vif, struct sk_buff *skb,
- int recalculate_partial_csum)
-{
- int err;
- u8 nexthdr;
- unsigned int off;
- unsigned int len;
- bool fragment;
- bool done;
-
- fragment = false;
- done = false;
-
- off = sizeof(struct ipv6hdr);
-
- err = maybe_pull_tail(skb, off, MAX_IPV6_HDR_LEN);
- if (err < 0)
- goto out;
-
- nexthdr = ipv6_hdr(skb)->nexthdr;
-
- len = sizeof(struct ipv6hdr) + ntohs(ipv6_hdr(skb)->payload_len);
- while (off <= len && !done) {
- switch (nexthdr) {
- case IPPROTO_DSTOPTS:
- case IPPROTO_HOPOPTS:
- case IPPROTO_ROUTING: {
- struct ipv6_opt_hdr *hp;
-
- err = maybe_pull_tail(skb,
- off +
- sizeof(struct ipv6_opt_hdr),
- MAX_IPV6_HDR_LEN);
- if (err < 0)
- goto out;
-
- hp = OPT_HDR(struct ipv6_opt_hdr, skb, off);
- nexthdr = hp->nexthdr;
- off += ipv6_optlen(hp);
- break;
- }
- case IPPROTO_AH: {
- struct ip_auth_hdr *hp;
-
- err = maybe_pull_tail(skb,
- off +
- sizeof(struct ip_auth_hdr),
- MAX_IPV6_HDR_LEN);
- if (err < 0)
- goto out;
-
- hp = OPT_HDR(struct ip_auth_hdr, skb, off);
- nexthdr = hp->nexthdr;
- off += ipv6_authlen(hp);
- break;
- }
- case IPPROTO_FRAGMENT: {
- struct frag_hdr *hp;
-
- err = maybe_pull_tail(skb,
- off +
- sizeof(struct frag_hdr),
- MAX_IPV6_HDR_LEN);
- if (err < 0)
- goto out;
-
- hp = OPT_HDR(struct frag_hdr, skb, off);
-
- if (hp->frag_off & htons(IP6_OFFSET | IP6_MF))
- fragment = true;
-
- nexthdr = hp->nexthdr;
- off += sizeof(struct frag_hdr);
- break;
- }
- default:
- done = true;
- break;
- }
- }
-
- err = -EPROTO;
-
- if (!done || fragment)
- goto out;
-
- switch (nexthdr) {
- case IPPROTO_TCP:
- err = maybe_pull_tail(skb,
- off + sizeof(struct tcphdr),
- MAX_IPV6_HDR_LEN);
- if (err < 0)
- goto out;
-
- if (!skb_partial_csum_set(skb, off,
- offsetof(struct tcphdr, check))) {
- err = -EPROTO;
- goto out;
- }
-
- if (recalculate_partial_csum)
- tcp_hdr(skb)->check =
- ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
- &ipv6_hdr(skb)->daddr,
- skb->len - off,
- IPPROTO_TCP, 0);
- break;
- case IPPROTO_UDP:
- err = maybe_pull_tail(skb,
- off + sizeof(struct udphdr),
- MAX_IPV6_HDR_LEN);
- if (err < 0)
- goto out;
-
- if (!skb_partial_csum_set(skb, off,
- offsetof(struct udphdr, check))) {
- err = -EPROTO;
- goto out;
- }
-
- if (recalculate_partial_csum)
- udp_hdr(skb)->check =
- ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
- &ipv6_hdr(skb)->daddr,
- skb->len - off,
- IPPROTO_UDP, 0);
- break;
- default:
- goto out;
- }
-
- err = 0;
-
-out:
- return err;
-}
-
static int checksum_setup(struct xenvif *vif, struct sk_buff *skb)
{
- int err = -EPROTO;
- int recalculate_partial_csum = 0;
+ bool recalculate_partial_csum = false;
/* A GSO SKB must be CHECKSUM_PARTIAL. However some buggy
* peers can fail to set NETRXF_csum_blank when sending a GSO
@@ -1308,19 +1059,14 @@ static int checksum_setup(struct xenvif *vif, struct sk_buff *skb)
if (skb->ip_summed != CHECKSUM_PARTIAL && skb_is_gso(skb)) {
vif->rx_gso_checksum_fixup++;
skb->ip_summed = CHECKSUM_PARTIAL;
- recalculate_partial_csum = 1;
+ recalculate_partial_csum = true;
}
/* A non-CHECKSUM_PARTIAL SKB does not require setup. */
if (skb->ip_summed != CHECKSUM_PARTIAL)
return 0;
- if (skb->protocol == htons(ETH_P_IP))
- err = checksum_setup_ip(vif, skb, recalculate_partial_csum);
- else if (skb->protocol == htons(ETH_P_IPV6))
- err = checksum_setup_ipv6(vif, skb, recalculate_partial_csum);
-
- return err;
+ return skb_checksum_setup(skb, recalculate_partial_csum);
}
static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next 3/3] xen-netfront: use new skb_checksum_setup function
From: Paul Durrant @ 2014-01-08 13:58 UTC (permalink / raw)
To: netdev, xen-devel
Cc: Paul Durrant, Konrad Rzeszutek Wilk, Boris Ostrovsky,
David Vrabel
In-Reply-To: <1389189511-14568-1-git-send-email-paul.durrant@citrix.com>
Use skb_checksum_setup to set up partial checksum offsets rather
then a private implementation.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
drivers/net/xen-netfront.c | 48 +++-----------------------------------------
1 file changed, 3 insertions(+), 45 deletions(-)
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e59acb1..c41537b 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -859,9 +859,7 @@ static RING_IDX xennet_fill_frags(struct netfront_info *np,
static int checksum_setup(struct net_device *dev, struct sk_buff *skb)
{
- struct iphdr *iph;
- int err = -EPROTO;
- int recalculate_partial_csum = 0;
+ bool recalculate_partial_csum = false;
/*
* A GSO SKB must be CHECKSUM_PARTIAL. However some buggy
@@ -873,54 +871,14 @@ static int checksum_setup(struct net_device *dev, struct sk_buff *skb)
struct netfront_info *np = netdev_priv(dev);
np->rx_gso_checksum_fixup++;
skb->ip_summed = CHECKSUM_PARTIAL;
- recalculate_partial_csum = 1;
+ recalculate_partial_csum = true;
}
/* A non-CHECKSUM_PARTIAL SKB does not require setup. */
if (skb->ip_summed != CHECKSUM_PARTIAL)
return 0;
- if (skb->protocol != htons(ETH_P_IP))
- goto out;
-
- iph = (void *)skb->data;
-
- switch (iph->protocol) {
- case IPPROTO_TCP:
- if (!skb_partial_csum_set(skb, 4 * iph->ihl,
- offsetof(struct tcphdr, check)))
- goto out;
-
- if (recalculate_partial_csum) {
- struct tcphdr *tcph = tcp_hdr(skb);
- tcph->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
- skb->len - iph->ihl*4,
- IPPROTO_TCP, 0);
- }
- break;
- case IPPROTO_UDP:
- if (!skb_partial_csum_set(skb, 4 * iph->ihl,
- offsetof(struct udphdr, check)))
- goto out;
-
- if (recalculate_partial_csum) {
- struct udphdr *udph = udp_hdr(skb);
- udph->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
- skb->len - iph->ihl*4,
- IPPROTO_UDP, 0);
- }
- break;
- default:
- if (net_ratelimit())
- pr_err("Attempting to checksum a non-TCP/UDP packet, dropping a protocol %d packet\n",
- iph->protocol);
- goto out;
- }
-
- err = 0;
-
-out:
- return err;
+ return skb_checksum_setup(skb, recalculate_partial_csum);
}
static int handle_incoming_queue(struct net_device *dev,
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next 1/3] net: add skb_checksum_setup
From: Paul Durrant @ 2014-01-08 13:58 UTC (permalink / raw)
To: netdev, xen-devel
Cc: Paul Durrant, David Miller, Eric Dumazet, Veaceslav Falico,
Alexander Duyck, Nicolas Dichtel
In-Reply-To: <1389189511-14568-1-git-send-email-paul.durrant@citrix.com>
This patch adds a function to set up the partial checksum offset for IP
packets (and optionally re-calculate the pseudo-header checksum) into the
core network code.
The implementation was previously private and duplicated between xen-netback
and xen-netfront, however it is not xen-specific and is potentially useful
to any network driver.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: David Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
include/linux/netdevice.h | 1 +
net/core/dev.c | 271 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 272 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a2a70cc..15b1003 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2940,6 +2940,7 @@ void netdev_upper_dev_unlink(struct net_device *dev,
void *netdev_lower_dev_get_private(struct net_device *dev,
struct net_device *lower_dev);
int skb_checksum_help(struct sk_buff *skb);
+int skb_checksum_setup(struct sk_buff *skb, bool recalculate);
struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
netdev_features_t features, bool tx_path);
struct sk_buff *skb_mac_gso_segment(struct sk_buff *skb,
diff --git a/net/core/dev.c b/net/core/dev.c
index ce01847..cf9fc30 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -101,6 +101,7 @@
#include <net/dst.h>
#include <net/pkt_sched.h>
#include <net/checksum.h>
+#include <net/ip6_checksum.h>
#include <net/xfrm.h>
#include <linux/highmem.h>
#include <linux/init.h>
@@ -2281,6 +2282,276 @@ out:
}
EXPORT_SYMBOL(skb_checksum_help);
+static inline int skb_maybe_pull_tail(struct sk_buff *skb, unsigned int len,
+ unsigned int max)
+{
+ if (skb_headlen(skb) >= len)
+ return 0;
+
+ /* If we need to pullup then pullup to the max, so we
+ * won't need to do it again.
+ */
+ if (max > skb->len)
+ max = skb->len;
+
+ if (__pskb_pull_tail(skb, max - skb_headlen(skb)) == NULL)
+ return -ENOMEM;
+
+ if (skb_headlen(skb) < len)
+ return -EPROTO;
+
+ return 0;
+}
+
+/* This value should be large enough to cover a tagged ethernet header plus
+ * maximally sized IP and TCP or UDP headers.
+ */
+#define MAX_IP_HDR_LEN 128
+
+static int skb_checksum_setup_ip(struct sk_buff *skb, bool recalculate)
+{
+ unsigned int off;
+ bool fragment;
+ int err;
+
+ fragment = false;
+
+ err = skb_maybe_pull_tail(skb,
+ sizeof(struct iphdr),
+ MAX_IP_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ if (ip_hdr(skb)->frag_off & htons(IP_OFFSET | IP_MF))
+ fragment = true;
+
+ off = ip_hdrlen(skb);
+
+ err = -EPROTO;
+
+ if (fragment)
+ goto out;
+
+ switch (ip_hdr(skb)->protocol) {
+ case IPPROTO_TCP:
+ err = skb_maybe_pull_tail(skb,
+ off + sizeof(struct tcphdr),
+ MAX_IP_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ if (!skb_partial_csum_set(skb, off,
+ offsetof(struct tcphdr, check))) {
+ err = -EPROTO;
+ goto out;
+ }
+
+ if (recalculate)
+ tcp_hdr(skb)->check =
+ ~csum_tcpudp_magic(ip_hdr(skb)->saddr,
+ ip_hdr(skb)->daddr,
+ skb->len - off,
+ IPPROTO_TCP, 0);
+ break;
+ case IPPROTO_UDP:
+ err = skb_maybe_pull_tail(skb,
+ off + sizeof(struct udphdr),
+ MAX_IP_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ if (!skb_partial_csum_set(skb, off,
+ offsetof(struct udphdr, check))) {
+ err = -EPROTO;
+ goto out;
+ }
+
+ if (recalculate)
+ udp_hdr(skb)->check =
+ ~csum_tcpudp_magic(ip_hdr(skb)->saddr,
+ ip_hdr(skb)->daddr,
+ skb->len - off,
+ IPPROTO_UDP, 0);
+ break;
+ default:
+ goto out;
+ }
+
+ err = 0;
+
+out:
+ return err;
+}
+
+/* This value should be large enough to cover a tagged ethernet header plus
+ * an IPv6 header, all options, and a maximal TCP or UDP header.
+ */
+#define MAX_IPV6_HDR_LEN 256
+
+#define OPT_HDR(type, skb, off) \
+ (type *)(skb_network_header(skb) + (off))
+
+static int skb_checksum_setup_ipv6(struct sk_buff *skb, bool recalculate)
+{
+ int err;
+ u8 nexthdr;
+ unsigned int off;
+ unsigned int len;
+ bool fragment;
+ bool done;
+
+ fragment = false;
+ done = false;
+
+ off = sizeof(struct ipv6hdr);
+
+ err = skb_maybe_pull_tail(skb, off, MAX_IPV6_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ nexthdr = ipv6_hdr(skb)->nexthdr;
+
+ len = sizeof(struct ipv6hdr) + ntohs(ipv6_hdr(skb)->payload_len);
+ while (off <= len && !done) {
+ switch (nexthdr) {
+ case IPPROTO_DSTOPTS:
+ case IPPROTO_HOPOPTS:
+ case IPPROTO_ROUTING: {
+ struct ipv6_opt_hdr *hp;
+
+ err = skb_maybe_pull_tail(skb,
+ off +
+ sizeof(struct ipv6_opt_hdr),
+ MAX_IPV6_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ hp = OPT_HDR(struct ipv6_opt_hdr, skb, off);
+ nexthdr = hp->nexthdr;
+ off += ipv6_optlen(hp);
+ break;
+ }
+ case IPPROTO_AH: {
+ struct ip_auth_hdr *hp;
+
+ err = skb_maybe_pull_tail(skb,
+ off +
+ sizeof(struct ip_auth_hdr),
+ MAX_IPV6_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ hp = OPT_HDR(struct ip_auth_hdr, skb, off);
+ nexthdr = hp->nexthdr;
+ off += ipv6_authlen(hp);
+ break;
+ }
+ case IPPROTO_FRAGMENT: {
+ struct frag_hdr *hp;
+
+ err = skb_maybe_pull_tail(skb,
+ off +
+ sizeof(struct frag_hdr),
+ MAX_IPV6_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ hp = OPT_HDR(struct frag_hdr, skb, off);
+
+ if (hp->frag_off & htons(IP6_OFFSET | IP6_MF))
+ fragment = true;
+
+ nexthdr = hp->nexthdr;
+ off += sizeof(struct frag_hdr);
+ break;
+ }
+ default:
+ done = true;
+ break;
+ }
+ }
+
+ err = -EPROTO;
+
+ if (!done || fragment)
+ goto out;
+
+ switch (nexthdr) {
+ case IPPROTO_TCP:
+ err = skb_maybe_pull_tail(skb,
+ off + sizeof(struct tcphdr),
+ MAX_IPV6_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ if (!skb_partial_csum_set(skb, off,
+ offsetof(struct tcphdr, check))) {
+ err = -EPROTO;
+ goto out;
+ }
+
+ if (recalculate)
+ tcp_hdr(skb)->check =
+ ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
+ &ipv6_hdr(skb)->daddr,
+ skb->len - off,
+ IPPROTO_TCP, 0);
+ break;
+ case IPPROTO_UDP:
+ err = skb_maybe_pull_tail(skb,
+ off + sizeof(struct udphdr),
+ MAX_IPV6_HDR_LEN);
+ if (err < 0)
+ goto out;
+
+ if (!skb_partial_csum_set(skb, off,
+ offsetof(struct udphdr, check))) {
+ err = -EPROTO;
+ goto out;
+ }
+
+ if (recalculate)
+ udp_hdr(skb)->check =
+ ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
+ &ipv6_hdr(skb)->daddr,
+ skb->len - off,
+ IPPROTO_UDP, 0);
+ break;
+ default:
+ goto out;
+ }
+
+ err = 0;
+
+out:
+ return err;
+}
+
+/* Set up partial checksum offset and optionally recalculate pseudo header
+ * checksum.
+ */
+int skb_checksum_setup(struct sk_buff *skb, bool recalculate)
+{
+ int err;
+
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ err = skb_checksum_setup_ip(skb, recalculate);
+ break;
+
+ case htons(ETH_P_IPV6):
+ err = skb_checksum_setup_ipv6(skb, recalculate);
+ break;
+
+ default:
+ err = -EPROTO;
+ break;
+ }
+
+ return err;
+}
+EXPORT_SYMBOL(skb_checksum_setup);
+
__be16 skb_network_protocol(struct sk_buff *skb)
{
__be16 type = skb->protocol;
--
1.7.10.4
^ permalink raw reply related
* [PATCH net-next 0/3] make skb_checksum_setup generally available
From: Paul Durrant @ 2014-01-08 13:58 UTC (permalink / raw)
To: netdev, xen-devel
Both xen-netfront and xen-netback need to be able to set up the partial
checksum offset of an skb and may also need to recalculate the pseudo-
header checksum in the process. This functionality is currently private
and duplicated between the two drivers.
Patch #1 of this series moves the implementation into the core network code
as there is nothing xen-specific about it and it is potentially useful to
any network driver.
Patch #2 removes the private implementation from netback.
Patch #3 removes the private implementation from netfront.
^ permalink raw reply
* Re: [PATCH net-next v3 6/9] xen-netback: Handle guests with too many frags
From: Eric Dumazet @ 2014-01-08 13:54 UTC (permalink / raw)
To: Zoltan Kiss
Cc: ian.campbell, wei.liu2, xen-devel, netdev, linux-kernel,
jonathan.davies
In-Reply-To: <52CD5785.4050402@citrix.com>
On Wed, 2014-01-08 at 13:49 +0000, Zoltan Kiss wrote:
> On 08/01/14 02:12, Eric Dumazet wrote:
> > On Wed, 2014-01-08 at 00:10 +0000, Zoltan Kiss wrote:
> >
> >>
> >> + if (skb_shinfo(skb)->frag_list) {
> >> + nskb = skb_shinfo(skb)->frag_list;
> >> + xenvif_fill_frags(vif, nskb, INVALID_PENDING_IDX);
> >> + skb->len += nskb->len;
> >> + skb->data_len += nskb->len;
> >> + skb->truesize += nskb->truesize;
> >> + skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
> >> + skb_shinfo(nskb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
> >> + vif->tx_zerocopy_sent += 2;
> >> + nskb = skb;
> >> +
> >> + skb = skb_copy_expand(skb,
> >> + 0,
> >> + 0,
> >> + GFP_ATOMIC | __GFP_NOWARN);
> >
> > skb can be NULL here
>
> Thanks, fixed that.
BTW, I am not sure why you copy the skb.
Is it to get rid of frag_list, and why ?
^ permalink raw reply
* Re: [PATCH net-next v3 6/9] xen-netback: Handle guests with too many frags
From: Zoltan Kiss @ 2014-01-08 13:49 UTC (permalink / raw)
To: Eric Dumazet
Cc: ian.campbell, wei.liu2, xen-devel, netdev, linux-kernel,
jonathan.davies
In-Reply-To: <1389147141.26646.74.camel@edumazet-glaptop2.roam.corp.google.com>
On 08/01/14 02:12, Eric Dumazet wrote:
> On Wed, 2014-01-08 at 00:10 +0000, Zoltan Kiss wrote:
>
>>
>> + if (skb_shinfo(skb)->frag_list) {
>> + nskb = skb_shinfo(skb)->frag_list;
>> + xenvif_fill_frags(vif, nskb, INVALID_PENDING_IDX);
>> + skb->len += nskb->len;
>> + skb->data_len += nskb->len;
>> + skb->truesize += nskb->truesize;
>> + skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
>> + skb_shinfo(nskb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
>> + vif->tx_zerocopy_sent += 2;
>> + nskb = skb;
>> +
>> + skb = skb_copy_expand(skb,
>> + 0,
>> + 0,
>> + GFP_ATOMIC | __GFP_NOWARN);
>
> skb can be NULL here
Thanks, fixed that.
Zoli
^ permalink raw reply
* Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get (v2)
From: Eric Dumazet @ 2014-01-08 13:47 UTC (permalink / raw)
To: Andrey Vagin
Cc: netfilter-devel, netfilter, coreteam, netdev, linux-kernel, vvs,
Florian Westphal, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, Cyrill Gorcunov
In-Reply-To: <1389187051-7794-1-git-send-email-avagin@openvz.org>
On Wed, 2014-01-08 at 17:17 +0400, Andrey Vagin wrote:
> Lets look at destroy_conntrack:
>
> hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
> ...
> nf_conntrack_free(ct)
> kmem_cache_free(net->ct.nf_conntrack_cachep, ct);
>
> net->ct.nf_conntrack_cachep is created with SLAB_DESTROY_BY_RCU.
>
> The hash is protected by rcu, so readers look up conntracks without
> locks.
> A conntrack is removed from the hash, but in this moment a few readers
> still can use the conntrack. Then this conntrack is released and another
> thread creates conntrack with the same address and the equal tuple.
> After this a reader starts to validate the conntrack:
> * It's not dying, because a new conntrack was created
> * nf_ct_tuple_equal() returns true.
>
> But this conntrack is not initialized yet, so it can not be used by two
> threads concurrently. In this case BUG_ON may be triggered from
> nf_nat_setup_info().
>
> Florian Westphal suggested to check the confirm bit too. I think it's
> right.
>
> task 1 task 2 task 3
> nf_conntrack_find_get
> ____nf_conntrack_find
> destroy_conntrack
> hlist_nulls_del_rcu
> nf_conntrack_free
> kmem_cache_free
> __nf_conntrack_alloc
> kmem_cache_alloc
> memset(&ct->tuplehash[IP_CT_DIR_MAX],
> if (nf_ct_is_dying(ct))
> if (!nf_ct_tuple_equal()
>
> I'm not sure, that I have ever seen this race condition in a real life.
> Currently we are investigating a bug, which is reproduced on a few node.
> In our case one conntrack is initialized from a few tasks concurrently,
> we don't have any other explanation for this.
>
> <2>[46267.083061] kernel BUG at net/ipv4/netfilter/nf_nat_core.c:322!
> ...
> <4>[46267.083951] RIP: 0010:[<ffffffffa01e00a4>] [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590 [nf_nat]
> ...
> <4>[46267.085549] Call Trace:
> <4>[46267.085622] [<ffffffffa023421b>] alloc_null_binding+0x5b/0xa0 [iptable_nat]
> <4>[46267.085697] [<ffffffffa02342bc>] nf_nat_rule_find+0x5c/0x80 [iptable_nat]
> <4>[46267.085770] [<ffffffffa0234521>] nf_nat_fn+0x111/0x260 [iptable_nat]
> <4>[46267.085843] [<ffffffffa0234798>] nf_nat_out+0x48/0xd0 [iptable_nat]
> <4>[46267.085919] [<ffffffff814841b9>] nf_iterate+0x69/0xb0
> <4>[46267.085991] [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
> <4>[46267.086063] [<ffffffff81484374>] nf_hook_slow+0x74/0x110
> <4>[46267.086133] [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
> <4>[46267.086207] [<ffffffff814b5890>] ? dst_output+0x0/0x20
> <4>[46267.086277] [<ffffffff81495204>] ip_output+0xa4/0xc0
> <4>[46267.086346] [<ffffffff814b65a4>] raw_sendmsg+0x8b4/0x910
> <4>[46267.086419] [<ffffffff814c10fa>] inet_sendmsg+0x4a/0xb0
> <4>[46267.086491] [<ffffffff814459aa>] ? sock_update_classid+0x3a/0x50
> <4>[46267.086562] [<ffffffff81444d67>] sock_sendmsg+0x117/0x140
> <4>[46267.086638] [<ffffffff8151997b>] ? _spin_unlock_bh+0x1b/0x20
> <4>[46267.086712] [<ffffffff8109d370>] ? autoremove_wake_function+0x0/0x40
> <4>[46267.086785] [<ffffffff81495e80>] ? do_ip_setsockopt+0x90/0xd80
> <4>[46267.086858] [<ffffffff8100be0e>] ? call_function_interrupt+0xe/0x20
> <4>[46267.086936] [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
> <4>[46267.087006] [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
> <4>[46267.087081] [<ffffffff8118f2e8>] ? kmem_cache_alloc+0xd8/0x1e0
> <4>[46267.087151] [<ffffffff81445599>] sys_sendto+0x139/0x190
> <4>[46267.087229] [<ffffffff81448c0d>] ? sock_setsockopt+0x16d/0x6f0
> <4>[46267.087303] [<ffffffff810efa47>] ? audit_syscall_entry+0x1d7/0x200
> <4>[46267.087378] [<ffffffff810ef795>] ? __audit_syscall_exit+0x265/0x290
> <4>[46267.087454] [<ffffffff81474885>] ? compat_sys_setsockopt+0x75/0x210
> <4>[46267.087531] [<ffffffff81474b5f>] compat_sys_socketcall+0x13f/0x210
> <4>[46267.087607] [<ffffffff8104dea3>] ia32_sysret+0x0/0x5
> <4>[46267.087676] Code: 91 20 e2 01 75 29 48 89 de 4c 89 f7 e8 56 fa ff ff 85 c0 0f 84 68 fc ff ff 0f b6 4d c6 41 8b 45 00 e9 4d fb ff ff e8 7c 19 e9 e0 <0f> 0b eb fe f6 05 17 91 20 e2 80 74 ce 80 3d 5f 2e 00 00 00 74
> <1>[46267.088023] RIP [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590
>
> v2: move nf_ct_is_confirmed into the unlikely() annotation
>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Andrey Vagin <avagin@openvz.org>
> ---
> net/netfilter/nf_conntrack_core.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 43549eb..403f634 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -387,8 +387,12 @@ begin:
> !atomic_inc_not_zero(&ct->ct_general.use)))
> h = NULL;
> else {
> + /* A conntrack can be recreated with the equal tuple,
> + * so we need to check that the conntrack is initialized
> + */
> if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
> - nf_ct_zone(ct) != zone)) {
> + nf_ct_zone(ct) != zone ||
> + !nf_ct_is_confirmed(ct))) {
> nf_ct_put(ct);
> goto begin;
> }
I am still not convinced of this being the right fix.
The key we test after taking the refcount should be the same key that we
test before taking the refcount, otherwise we might add a loop here.
Your patch did not change ____nf_conntrack_find(), so I find it
confusing.
^ permalink raw reply
* Re: [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get
From: Eric Dumazet @ 2014-01-08 13:42 UTC (permalink / raw)
To: Florian Westphal
Cc: Andrey Vagin, netfilter-devel, netfilter, coreteam, netdev,
linux-kernel, vvs, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, Cyrill Gorcunov
In-Reply-To: <20140107152520.GF9894@breakpoint.cc>
On Tue, 2014-01-07 at 16:25 +0100, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > > index 43549eb..7a34bb2 100644
> > > --- a/net/netfilter/nf_conntrack_core.c
> > > +++ b/net/netfilter/nf_conntrack_core.c
> > > @@ -387,8 +387,12 @@ begin:
> > > !atomic_inc_not_zero(&ct->ct_general.use)))
> > > h = NULL;
> > > else {
> > > + /* A conntrack can be recreated with the equal tuple,
> > > + * so we need to check that the conntrack is initialized
> > > + */
> > > if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
> > > - nf_ct_zone(ct) != zone)) {
> > > + nf_ct_zone(ct) != zone) ||
> > > + !nf_ct_is_confirmed(ct)) {
> > > nf_ct_put(ct);
> > > goto begin;
> > > }
> >
> > I do not think this is the right way to fix this problem (if said
> > problem is confirmed)
> >
> > Remember the rule about SLAB_DESTROY_BY_RCU :
> >
> > When a struct is freed, then reused, its important to set the its refcnt
> > (from 0 to 1) only when the structure is fully ready for use.
> >
> > If a lookup finds a structure which is not yet setup, the
> > atomic_inc_not_zero() will fail.
>
> Indeed. But, the structure itself might be ready (or rather,
> can be ready since the allocation side will set the refcount to one
> after doing the initial work, such as zapping old ->status flags and
> setting tuple information).
>
> The problem is with nat extension area stored in the ct->ext area.
> This extension area is preallocated but the snat/dnat action
> information is only set up after the ct (or rather, the skb that grabbed
> a reference to the nf_conn entry) traverses nat pre/postrouting.
>
> This will also set up a null-binding when no matching SNAT/DNAT/MASQERUADE
> rule existed.
>
> The manipulations of the skb->nfct->ext nat area are performed without
> a lock. Concurrent access is supposedly impossible as the conntrack
> should not (yet) be in the hash table.
>
> The confirmed bit is set right before we insert the conntrack into
> the hash table (after we traversed rules, ct is ready to be
> 'published').
>
> i.e. when the confirmed bit is NOT set we should not be 'seeing' the nf_conn
> struct when we perform the lookup, as it should still be sitting on the
> 'unconfirmed' list, being invisible to readers.
>
> Does that explanation make sense to you?
>
> Thanks for looking into this.
Still, this patch adds a loop. And maybe an infinite one if confirmed
bit is set from an context that was interrupted by this one.
If you need to test the confirmed bit, then you also need to test it
before taking the refcount.
^ permalink raw reply
* Re: IPv6: Bug in net-next
From: François-Xavier Le Bail @ 2014-01-08 13:35 UTC (permalink / raw)
To: netdev; +Cc: davem
In-Reply-To: <1389187126.20074.YahooMailBasic@web125504.mail.ne1.yahoo.com>
On Wed, 1/8/14, François-Xavier Le Bail <fx.lebail@yahoo.com> wrote:
> > I think there is a bug in actual net-next.
> > When I execute this code and press Crtl-C, all the IPv6
> > Link-Layer addresses are deleted.
> > This happened between
> > c1ddf295f5183a5189196a8035546842caa2055a and HEAD.
> > [...]
> Sorry, it is not linked to this code.
> I'm looking for the reason ...
It seems that the valid lifetime and preferred lft are set to 0 sec for autoconfigured LLA.
BR,
Francois-Xavier
^ permalink raw reply
* Re: IPv6: Bug in net-next
From: François-Xavier Le Bail @ 2014-01-08 13:18 UTC (permalink / raw)
To: netdev; +Cc: davem
In-Reply-To: <1389186135.88856.YahooMailBasic@web125506.mail.ne1.yahoo.com>
On Wed, 1/8/14, François-Xavier Le Bail <fx.lebail@yahoo.com> wrote:
> Hello,
> I think there is a bug in actual net-next.
> When I execute this code and press Crtl-C, all the IPv6
> Link-Layer addresses are deleted.
> This happened between
> c1ddf295f5183a5189196a8035546842caa2055a and HEAD.
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <netinet/in.h>
> #include <net/if.h>
>
> int main (int argc, char **argv) {
> struct ipv6_mreq mreq = { 0 };
> int sockfd1 = socket (AF_INET6, SOCK_DGRAM, 0);
>
> inet_pton (AF_INET6, "2a01:999::1", &mreq.ipv6mr_multiaddr);
> mreq.ipv6mr_interface = if_nametoindex ("dummy0");
> setsockopt (sockfd1, IPPROTO_IPV6, IPV6_JOIN_ANYCAST, &mreq, sizeof (mreq));
>
> pause ();
> }
Sorry, it is not linked to this code.
I'm looking for the reason ...
BR,
Francois-Xavier
^ permalink raw reply
* [PATCH] netfilter: nf_conntrack: fix RCU race in nf_conntrack_find_get (v2)
From: Andrey Vagin @ 2014-01-08 13:17 UTC (permalink / raw)
To: netfilter-devel
Cc: netfilter, coreteam, netdev, linux-kernel, vvs, Andrey Vagin,
Florian Westphal, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, Cyrill Gorcunov
In-Reply-To: <1389090711-15843-1-git-send-email-avagin@openvz.org>
Lets look at destroy_conntrack:
hlist_nulls_del_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode);
...
nf_conntrack_free(ct)
kmem_cache_free(net->ct.nf_conntrack_cachep, ct);
net->ct.nf_conntrack_cachep is created with SLAB_DESTROY_BY_RCU.
The hash is protected by rcu, so readers look up conntracks without
locks.
A conntrack is removed from the hash, but in this moment a few readers
still can use the conntrack. Then this conntrack is released and another
thread creates conntrack with the same address and the equal tuple.
After this a reader starts to validate the conntrack:
* It's not dying, because a new conntrack was created
* nf_ct_tuple_equal() returns true.
But this conntrack is not initialized yet, so it can not be used by two
threads concurrently. In this case BUG_ON may be triggered from
nf_nat_setup_info().
Florian Westphal suggested to check the confirm bit too. I think it's
right.
task 1 task 2 task 3
nf_conntrack_find_get
____nf_conntrack_find
destroy_conntrack
hlist_nulls_del_rcu
nf_conntrack_free
kmem_cache_free
__nf_conntrack_alloc
kmem_cache_alloc
memset(&ct->tuplehash[IP_CT_DIR_MAX],
if (nf_ct_is_dying(ct))
if (!nf_ct_tuple_equal()
I'm not sure, that I have ever seen this race condition in a real life.
Currently we are investigating a bug, which is reproduced on a few node.
In our case one conntrack is initialized from a few tasks concurrently,
we don't have any other explanation for this.
<2>[46267.083061] kernel BUG at net/ipv4/netfilter/nf_nat_core.c:322!
...
<4>[46267.083951] RIP: 0010:[<ffffffffa01e00a4>] [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590 [nf_nat]
...
<4>[46267.085549] Call Trace:
<4>[46267.085622] [<ffffffffa023421b>] alloc_null_binding+0x5b/0xa0 [iptable_nat]
<4>[46267.085697] [<ffffffffa02342bc>] nf_nat_rule_find+0x5c/0x80 [iptable_nat]
<4>[46267.085770] [<ffffffffa0234521>] nf_nat_fn+0x111/0x260 [iptable_nat]
<4>[46267.085843] [<ffffffffa0234798>] nf_nat_out+0x48/0xd0 [iptable_nat]
<4>[46267.085919] [<ffffffff814841b9>] nf_iterate+0x69/0xb0
<4>[46267.085991] [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
<4>[46267.086063] [<ffffffff81484374>] nf_hook_slow+0x74/0x110
<4>[46267.086133] [<ffffffff81494e70>] ? ip_finish_output+0x0/0x2f0
<4>[46267.086207] [<ffffffff814b5890>] ? dst_output+0x0/0x20
<4>[46267.086277] [<ffffffff81495204>] ip_output+0xa4/0xc0
<4>[46267.086346] [<ffffffff814b65a4>] raw_sendmsg+0x8b4/0x910
<4>[46267.086419] [<ffffffff814c10fa>] inet_sendmsg+0x4a/0xb0
<4>[46267.086491] [<ffffffff814459aa>] ? sock_update_classid+0x3a/0x50
<4>[46267.086562] [<ffffffff81444d67>] sock_sendmsg+0x117/0x140
<4>[46267.086638] [<ffffffff8151997b>] ? _spin_unlock_bh+0x1b/0x20
<4>[46267.086712] [<ffffffff8109d370>] ? autoremove_wake_function+0x0/0x40
<4>[46267.086785] [<ffffffff81495e80>] ? do_ip_setsockopt+0x90/0xd80
<4>[46267.086858] [<ffffffff8100be0e>] ? call_function_interrupt+0xe/0x20
<4>[46267.086936] [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
<4>[46267.087006] [<ffffffff8118cb10>] ? ub_slab_ptr+0x20/0x90
<4>[46267.087081] [<ffffffff8118f2e8>] ? kmem_cache_alloc+0xd8/0x1e0
<4>[46267.087151] [<ffffffff81445599>] sys_sendto+0x139/0x190
<4>[46267.087229] [<ffffffff81448c0d>] ? sock_setsockopt+0x16d/0x6f0
<4>[46267.087303] [<ffffffff810efa47>] ? audit_syscall_entry+0x1d7/0x200
<4>[46267.087378] [<ffffffff810ef795>] ? __audit_syscall_exit+0x265/0x290
<4>[46267.087454] [<ffffffff81474885>] ? compat_sys_setsockopt+0x75/0x210
<4>[46267.087531] [<ffffffff81474b5f>] compat_sys_socketcall+0x13f/0x210
<4>[46267.087607] [<ffffffff8104dea3>] ia32_sysret+0x0/0x5
<4>[46267.087676] Code: 91 20 e2 01 75 29 48 89 de 4c 89 f7 e8 56 fa ff ff 85 c0 0f 84 68 fc ff ff 0f b6 4d c6 41 8b 45 00 e9 4d fb ff ff e8 7c 19 e9 e0 <0f> 0b eb fe f6 05 17 91 20 e2 80 74 ce 80 3d 5f 2e 00 00 00 74
<1>[46267.088023] RIP [<ffffffffa01e00a4>] nf_nat_setup_info+0x564/0x590
v2: move nf_ct_is_confirmed into the unlikely() annotation
Cc: Florian Westphal <fw@strlen.de>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
net/netfilter/nf_conntrack_core.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 43549eb..403f634 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -387,8 +387,12 @@ begin:
!atomic_inc_not_zero(&ct->ct_general.use)))
h = NULL;
else {
+ /* A conntrack can be recreated with the equal tuple,
+ * so we need to check that the conntrack is initialized
+ */
if (unlikely(!nf_ct_tuple_equal(tuple, &h->tuple) ||
- nf_ct_zone(ct) != zone)) {
+ nf_ct_zone(ct) != zone ||
+ !nf_ct_is_confirmed(ct))) {
nf_ct_put(ct);
goto begin;
}
--
1.8.4.2
^ permalink raw reply related
* IPv6: Bug in net-next
From: François-Xavier Le Bail @ 2014-01-08 13:02 UTC (permalink / raw)
To: netdev; +Cc: davem
Hello,
I think there is a bug in actual net-next.
When I execute this code and press Crtl-C, all the IPv6 Link-Layer addresses are deleted.
This happened between c1ddf295f5183a5189196a8035546842caa2055a and HEAD.
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <net/if.h>
int main (int argc, char **argv)
{
struct ipv6_mreq mreq = { 0 };
int sockfd1 = socket (AF_INET6, SOCK_DGRAM, 0);
inet_pton (AF_INET6, "2a01:999::1", &mreq.ipv6mr_multiaddr);
mreq.ipv6mr_interface = if_nametoindex ("dummy0");
setsockopt (sockfd1, IPPROTO_IPV6, IPV6_JOIN_ANYCAST, &mreq, sizeof (mreq));
pause ();
}
BR,
Francois-Xavier
^ permalink raw reply
* Re: [PATCH net 1/2] macvlan: forbid L2 fowarding offload for macvtap
From: Michael S. Tsirkin @ 2014-01-08 12:55 UTC (permalink / raw)
To: Jason Wang
Cc: John Fastabend, John Fastabend, Neil Horman, davem, netdev,
linux-kernel, Vlad Yasevich
In-Reply-To: <52CBC22D.3050002@redhat.com>
On Tue, Jan 07, 2014 at 05:00:29PM +0800, Jason Wang wrote:
> On 01/07/2014 03:26 PM, John Fastabend wrote:
> > [...]
> >
> >>>> Unfortunately not. This commit has a side effect that it in fact
> >>>> disables the multiqueue macvtap transmission. Since all macvtap queues
> >>>> will contend on a single qdisc lock.
> >>>>
> >>>
> >>> They will only contend on a single qdisc lock if the lower device has
> >>> 1 queue.
> >>
> >> I think we are talking about 6acf54f1cf0a6747bac9fea26f34cfc5a9029523.
> >
> > Yes.
> >
> >>
> >> The qdisc or txq lock were macvlan device itself since dev_queue_xmit()
> >> was called for macvlan device itself. So even if lower device has
> >> multiple txqs, if you just create a one queue macvlan device, you will
> >> get lock contention on macvlan device. And even if you explicitly
> >> specifying the txq numbers ( though I don't believe most management
> >> software will do this) when creating the macvlan/macvtap device, you
> >> must also configure the XPS for macvlan to make sure it has the
> >> possibility of using multiple transmit queues.
> >>
> >
> > OK I think I'm finally putting all the pieces together thanks.
> >
> > Do you know why macvtap is setting dev->tx_queue_len by default? If you
> > zero this then the noqueue_qdisc is used and the q->enqueue check in
> > dev_queue_xmit will fail.
>
> It was introduced in commit 8a35747a5d13b99e076b0222729e0caa48cb69b6
> ("macvtap: Limit packet queue length") to limit the length of socket
> receive queue of macvtap. But I'm not sure whether the qdisc is a
> byproduct of this commit, maybe we can switch to use another name
> instead of just reuse dev->tx_queue_length.
You mean tx_queue_len really, right?
Problem is tx_queue_len can be accessed using netlink sysfs or ioctl,
so if someone uses these to control or check the # of packets that
can be queued by device, this will break.
How about adding ndo_set_tx_queue_len then?
At some point we wanted to decouple queue length from tx_queue_length
for tun as well, so that would be benefitial there as well.
> >
> > Also if XPS is not configured then skb_tx_hash is used so multiple
> > transmit queues will still be used.
> >
>
> True.
> >>> Perhaps defaulting the L2 forwarding devices to 1queue was a
> >>> mistake. But the same issue arises when running macvtap over a
> >>> non-multiqueue nic. Or even if you have a multiqueue device and create
> >>> many more macvtap queues than the lower device has queues.
> >>>
> >>> Shouldn't the macvtap configuration take into account the lowest level
> >>> devices queues?
> >>
> >> See commit 8ffab51b3dfc54876f145f15b351c41f3f703195 ("macvlan: lockless
> >> tx path"). It allows the management to create a device without worrying
> >> the underlying device.
> >
> > OK.
> >
> >>> How does using the L2 forwarding device change the
> >>> contention issues? Without the L2 forwarding LLTX is enabled but the
> >>> qdisc lock, etc is still acquired on the device below the macvlan.
> >>>
> >>
> >> That's the point. We need make sure the txq selection and qdisc lock
> >> were done for the lower device not for the macvlan device itself. Then
> >> macvlan can automatically benefit from the multi-queue capable lower
> >> devices. But L2 forwarding needs to contend on the txq lock on macvlan
> >> device itself, which is unnecessary and can complex the management.
> >
> > If I make the l2 forwarding defaults a bit better then using the L2
> > forwarding case should not be any more complex. And because the queues
> > are dedicated to the macvtap device any contention from qdisc lock, etc
> > comes from the upper device only.
>
> At very least the txq of lower device should be held in order to be
> synchronized with management path. Consider txq lock were often held by
> netif_tx_disable() before trying to down the card. Current cold does not
> hold txq lock, so it loses the synchronization which may cause issues.
> And the code also does not check whether the txq has been stopped before
> trying to start the transmission.
>
>
> > Also if I get the bandwidth controls
> > in we can set the max/min bandwidth per macvtap device this way. That
> > is future work though.
> >
>
> That will be a nice feature.
> >>> The ixgbe driver as it is currently written can be configured for up to
> >>> 4 queues by setting numtxqueues when the device is created. I assume
> >>> when creating macvtap queues the user needs to account for the number
> >>> of queues supported by the lower device.
> >>>
> >>
> >> We'd better not complicate the task of management, lockless tx path work
> >> very well so we can just keep it. Btw, there's no way for the user to
> >> know the maximum number of queues that L2 forwarding supports.
> >
> > Good point I'll add an attribute to query it.
> >
> >>>> For L2 forwarding offload itself, more issues need to be addressed for
> >>>> multiqueue macvtap:
> >>>>
> >>>> - ndo_dfwd_add_station() can only create queues per device at
> >>>> ndo_open,
> >>>> but multiqueue macvtap allows user to create and destroy queues at
> >>>> their
> >>>> will and at any time.
> >>>
> >>> same argument as above, isn't this the same when running macvtap
> >>> without
> >>> the l2 offloads over a real device? I expect you hit the same
> >>> contention
> >>> points when running over a real device.
> >>
> >> Not true and not only for contention.
> >>
> >> Macvtap allows user to create or destroy a queue by simply open or close
> >> to character device /dev/tapX. But currently, we do nothing when a new
> >> queue was created or destroyed for L2 forwarding offload.
> >>
> >> For contention, lockless tx path make the contention only happens for
> >> the txq or qdisc for the lower device, but L2 forwarding offload make
> >> contention also happen for the macvlan device itself.
> >
> > Right, but there will be less contention there because those queues
> > are a dedicated resource for the upper device.
>
> Yes and this is also true if we only do synchronization on the lower
> device since only dedicated queues could be selected.
> >
> > At this point I think I need to put together a real testbed and
> > benchmark some of this with netperf and perf running to get real
> > numbers. When I originally did the l2 forwarding I did not do any
> > testing with multiple macvtap queues and only very limited work with
> > macvtap.
> >
>
> As I said above, holding the txq lock of lower device seems a must and
> we should not get regression if NETIF_F_LLTX is kept. But I agree we
> need some test.
> >>
> >>>
> >>>
> >>>> - it looks that ixgbe has a upper limit of 4 queues per station, but
> >>>> macvtap currently allows up to 16 queues per device.
> >>>>
> >>>
> >>> The 4 limit was to simplify the code because the queue mapping in the
> >>> driver gets complicated if it is greater than 4. We can probably
> >>> increase this latter. But sorry reiterating how is this different than
> >>> a macvtap on a real device that supports a max of 4 queues?
> >>
> >> Well, it maybe easy. I just point out possible issues we may meet
> >> currently.
> >
> > Right.
> >
> >>>
> >>>> So more works need to be done and unless those above 3 issues were
> >>>> addressed, this patch is really needed to make sure macvtap works.
> >>>>
> >>>
> >>> Agreed there is a lot more work here to improve things I'm just not
> >>> sure we need to disable this now. Also note its the l2 forwarding
> >>> should be disabled by default so a user would have to enable the
> >>> feature flag.
> >>
> >> Even if it was disabled by default. We should not surprise the user who
> >> want to enable it for macvtap.
> >
> > So the question is what to do in net while we improve net-next. Either
> > we fix the crash from the null txq and note that with l2 forwarding
> > some non default configuration is needed for optimal performance OR
> > for now disable it as your patch does. I would prefer to fix the crash
> > and note the configuration but I see your point about surprising users
> > so could go either way.
> >
>
> It's much safer to disable l2 forwarding offload for macvtap temporarily
> consider it has several issues.We can re-enable it when everything is
> ready in net-next. We we really need to hold the txq lock of lower
> device, only add more check of NULL pointer is not sufficient. So
> explicitly select a txq is still needed. And I don't see any conflicts
> between this and future enhancement.
>
> Also I don't see any drawback of using NETIF_F_LLTX for l2 forwarding.
> So we'd better keep it.
> > Neil any thoughts?
> >
> > To fix the null txq in the gso case adding a check for a non-null
> > txq before calling txq_trans_update() makes sense to me. We already
> > have the check in the non-gso case so making it symmetric fixes it.
> >
> >>>
> >>> Thanks,
> >>> John
> >>>
> >>
> >> Thanks
> >>
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH net-next v2] xen-netback: stop vif thread spinning if frontend is unresponsive
From: Paul Durrant @ 2014-01-08 12:41 UTC (permalink / raw)
To: netdev, xen-devel; +Cc: Paul Durrant, Wei Liu, Ian Campbell, David Vrabel
The recent patch to improve guest receive side flow control (ca2f09f2) had a
slight flaw in the wait condition for the vif thread in that any remaining
skbs in the guest receive side netback internal queue would prevent the
thread from sleeping. An unresponsive frontend can lead to a permanently
non-empty internal queue and thus the thread will spin. In this case the
thread should really sleep until the frontend becomes responsive again.
This patch adds an extra flag to the vif which is set if the shared ring
is full and cleared when skbs are drained into the shared ring. Thus,
if the thread runs, finds the shared ring full and can make no progress the
flag remains set. If the flag remains set then the thread will sleep,
regardless of a non-empty queue, until the next event from the frontend.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
v2:
- Use bool for ring_full
- Convert need_to_notify to bool for consistency
drivers/net/xen-netback/common.h | 1 +
drivers/net/xen-netback/netback.c | 14 +++++++++-----
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index c955fc3..4c76bcb 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -143,6 +143,7 @@ struct xenvif {
char rx_irq_name[IFNAMSIZ+4]; /* DEVNAME-rx */
struct xen_netif_rx_back_ring rx;
struct sk_buff_head rx_queue;
+ bool rx_queue_stopped;
/* Set when the RX interrupt is triggered by the frontend.
* The worker thread may need to wake the queue.
*/
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 4f81ac0..2738563 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -476,7 +476,8 @@ static void xenvif_rx_action(struct xenvif *vif)
int ret;
unsigned long offset;
struct skb_cb_overlay *sco;
- int need_to_notify = 0;
+ bool need_to_notify = false;
+ bool ring_full = false;
struct netrx_pending_operations npo = {
.copy = vif->grant_copy_op,
@@ -508,7 +509,8 @@ static void xenvif_rx_action(struct xenvif *vif)
/* If the skb may not fit then bail out now */
if (!xenvif_rx_ring_slots_available(vif, max_slots_needed)) {
skb_queue_head(&vif->rx_queue, skb);
- need_to_notify = 1;
+ need_to_notify = true;
+ ring_full = true;
break;
}
@@ -521,6 +523,8 @@ static void xenvif_rx_action(struct xenvif *vif)
BUG_ON(npo.meta_prod > ARRAY_SIZE(vif->meta));
+ vif->rx_queue_stopped = !npo.copy_prod && ring_full;
+
if (!npo.copy_prod)
goto done;
@@ -592,8 +596,7 @@ static void xenvif_rx_action(struct xenvif *vif)
RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&vif->rx, ret);
- if (ret)
- need_to_notify = 1;
+ need_to_notify |= !!ret;
npo.meta_cons += sco->meta_slots_used;
dev_kfree_skb(skb);
@@ -1724,7 +1727,8 @@ static struct xen_netif_rx_response *make_rx_response(struct xenvif *vif,
static inline int rx_work_todo(struct xenvif *vif)
{
- return !skb_queue_empty(&vif->rx_queue) || vif->rx_event;
+ return (!skb_queue_empty(&vif->rx_queue) && !vif->rx_queue_stopped) ||
+ vif->rx_event;
}
static inline int tx_work_todo(struct xenvif *vif)
--
1.7.10.4
^ permalink raw reply related
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