Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next v2 5/7] rhashtable: avoid unnecessary wakeup for worker queue
From: Ying Xue @ 2015-01-07 10:28 UTC (permalink / raw)
  To: Thomas Graf; +Cc: jon.maloy, netdev, Paul.Gortmaker, tipc-discussion, davem
In-Reply-To: <20150107095013.GJ21820@casper.infradead.org>

On 01/07/2015 05:50 PM, Thomas Graf wrote:
> On 01/07/15 at 01:41pm, Ying Xue wrote:
>> Move condition statements of verifying whether hash table size exceeds
>> its maximum threshold or reaches its minimum threshold from resizing
>> functions to resizing decision functions, avoiding unnecessary wakeup
>> for worker queue thread.
>>
>> Signed-off-by: Ying Xue <ying.xue@windriver.com>
>> Cc: Thomas Graf <tgraf@suug.ch>
> 
> Good optimization, thanks!
> 
> Acked-by: Thomas Graf <tgraf@suug.ch>
> 
> Can you do a follow-up patch and add a note in rhashtable.h to
> indicate the implementation of the grow and shrink decision function
> must enforce min/max shift?
> 
> 

Thanks for the reminder, and I will do this later.

Regards,
Ying


------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net

^ permalink raw reply

* Re: [PATCH 1/6] vxlan: Allow for VXLAN extensions to be implemented
From: Thomas Graf @ 2015-01-07 10:27 UTC (permalink / raw)
  To: Tom Herbert
  Cc: David Miller, Jesse Gross, Stephen Hemminger, Pravin B Shelar,
	Linux Netdev List, dev@openvswitch.org
In-Reply-To: <CA+mtBx_mp1mf_HPp5u9wDfgeOc8pt26Mf1VHFZTAqDhTdZe7cw@mail.gmail.com>

On 01/06/15 at 07:46pm, Tom Herbert wrote:
> On Tue, Jan 6, 2015 at 6:05 PM, Thomas Graf <tgraf@suug.ch> wrote:
> > The VXLAN receive code is currently conservative in what it accepts and
> > will reject any frame that uses any of the reserved VXLAN protocol fields.
> > The VXLAN draft specifies that "reserved fields MUST be set to zero on
> > transmit and ignored on receive.".
> >
> IMO it is an unfortunate decision in VXLAN to ignore set reserved
> fields on receive. Had they not done this, then adding a protocol
> field to the header would have been feasible and we wouldn't need yet
> another encapsulation protocol (i.e. VXLAN-GPE). Rejecting frames with
> reserved bits set is the better behavior, but I think the comment
> about this needs to be clear about why this diverges from RFC7348.

Agreed. Do you want to give it a shot? I know you have been involved
on all aspects of this topic for a long time in NVO3 ;-)

> > Retain the current conservative parsing behaviour by default but allows
> > these fields to be used by VXLAN extensions which are explicitly enabled
> > on the VXLAN socket respectively VXLAN net_device.
> >
> Conceptually, VXLAN has both mandatory flags and optional flags for
> extensions. You may want to look at the VXLAN RCO patches that added
> an extension and infrastructure for them.

I've seen your proposed changes. I think they could be easily rebased
on top of this and use the enablement infrastructure. In fact, I see
this as the only feasible option to deal with VXLAN extensions: Leave
it to the user to decide which extensions to run. That way we can
support any combination of extensions, even conflicting ones. All we
have to do is catch incompatible extension configurations on a socket
and reject them at configuration time. Expecting that NVO3/IETF will
find consensus on a list of compatible set of extensions with perfect
forward and backwards compatibility is unrealistic in my eyes. We have
Geneve and GUE to solve it properly in the future.

^ permalink raw reply

* Re: [PATCH net-next v2 6/7] rhashtable: initialize atomic nelems variable
From: Ying Xue @ 2015-01-07 10:26 UTC (permalink / raw)
  To: Thomas Graf
  Cc: davem, jon.maloy, Paul.Gortmaker, erik.hugne, netdev,
	tipc-discussion
In-Reply-To: <20150107095332.GK21820@casper.infradead.org>

On 01/07/2015 05:53 PM, Thomas Graf wrote:
> On 01/07/15 at 01:41pm, Ying Xue wrote:
>> Signed-off-by: Ying Xue <ying.xue@windriver.com>
>> Cc: Thomas Graf <tgraf@suug.ch>
> 
> Is this really needed at all? We initialize the full rhashtable
> struct to 0 in rhashtable_init().
> 
> 

I am not sure whether we really need to reinitialize atomic variable
again although we have reset it with memset() or something else. But I
see many places in kernel where we do this, for example:

Although we use kmem_cache_zalloc() to allocate "net" structure instance
in net_alloc(), there are still several places where to reinitialize its
atomic variables again:

setup_net()
  atomic_set(&net->use_count, 0);

rt_genid_init()
  atomic_set(&net->ipv4.rt_genid, 0);
  atomic_set(&net->fnhe_genid, 0);

Can you please definitely confirm that the reinitialisation is redundant
for us?

Regards,
Ying

^ permalink raw reply

* Re: [PATCH iproute2 -next] ip: route: add congestion control metric
From: Daniel Borkmann @ 2015-01-07 10:21 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: fw, netdev
In-Reply-To: <20150106170926.5bb2d816@urahara>

On 01/07/2015 02:09 AM, Stephen Hemminger wrote:
> On Wed,  7 Jan 2015 00:52:37 +0100
> Daniel Borkmann <dborkman@redhat.com> wrote:
>
>> +		} else if (matches(*argv, "congctl") == 0) {
>> +			char cc[16];
>> +			NEXT_ARG();
>> +			memset(cc, 0, sizeof(cc));
>> +			if (strcmp(*argv, "lock") == 0) {
>> +				mxlock |= (1<<RTAX_CC_ALGO);
>
> Unneeded paren

Yep, I kept it consistent across all mxlock assignments of this file,
but I can remove it, sure.

>> +				NEXT_ARG();
>> +			}
>> +			strncpy(cc, *argv, sizeof(cc) - 1);
>> +			if (strlen(cc) == 0)
>> +				invarg("\"conctl\" value must be an algorithm name\n", *argv
>
> Silently truncating the string is not odd. Can't we just let kernel impose
> length restrictions.

Sure, will respin, thanks.

^ permalink raw reply

* [PATCH] wireless extensions should default to Y
From: Pavel Machek @ 2015-01-07 10:19 UTC (permalink / raw)
  To: jikos, kernel list, Linus Torvalds
  Cc: johannes, davem, linux-wireless, netdev

Do we need following patch on top of
24a0aa212ee2dbe44360288684478d76a8e20a0a revert?

I updated kernel today, and (probably because extensions were not
selectable before), the default choice was "N", which is wrong:
oldconfig should result in compatible choices being made, for example
to help bisect.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig
index 29c8675..8951dba 100644
--- a/net/wireless/Kconfig
+++ b/net/wireless/Kconfig
@@ -177,6 +177,7 @@ config CFG80211_INTERNAL_REGDB
 config CFG80211_WEXT
 	bool "cfg80211 wireless extensions compatibility"
 	depends on CFG80211
+	default y
 	select WEXT_CORE
 	help
 	  Enable this option if you need old userspace for wireless


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply related

* Re: [PATCH net-next] net: sched: use pinned timers
From: Cosmin GIRADU @ 2015-01-07 10:06 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev
In-Reply-To: <20140926.002710.1595423408687413602.davem@davemloft.net>

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

On 26/09/14 07:27, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 20 Sep 2014 18:01:30 -0700
>
>> From: Eric Dumazet <edumazet@google.com>
>>
>> While using a MQ + NETEM setup, I had confirmation that the default
>> timer migration ( /proc/sys/kernel/timer_migration ) is killing us.
>>
>> Installing this on a receiver side of a TCP_STREAM test, (NIC has 8 TX
>> queues) :
>  ...
>> Current Qdiscs that benefit from this change are :
>>
>> 	netem, cbq, fq, hfsc, tbf, htb.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Looks great, applied, thanks Eric.
> --
> 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
>
Hi Eric,

    I saw that this patch didn't make it's way into the stable branches.
So I have two questions:
        - Would it be safe to apply to linux-3.12.x stable?
        - If yes, would there be any [noticeable] efects on a [pretty
complex] HTB setup? (I know, test and I'll see,
           but if theory sais I won't, then there would be no point to
the test, would there?)

Thank you!

-- 

Cosmin GIRADU
OSS Engineer
RCS & RDS - Unified Services
Phone:  +40-31-400-6323
Mobile: +40-77-020-0858
http://www.rcs-rds.ro

..........................................................................
Privileged/Confidential Information may be contained in this message. If
you are not the addressee indicated in this message (or responsible for
delivery of the message to such person), you may not copy or deliver
this message to anyone. In such a case, you should destroy this message
and kindly notify the sender by reply e-mail.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply

* RE: [PATCH 1/1] update ip-sysctl.txt documentation
From: David Laight @ 2015-01-07 10:11 UTC (permalink / raw)
  To: 'Ani Sinha', corbet@lwn.net, davem@davemloft.net,
	edumazet@google.com, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, P@draigbrady.com,
	netdev@vger.kernel.org, fruggeri@arista.com
In-Reply-To: <1420587382-24612-1-git-send-email-ani@arista.com>

From: Ani Sinha
> Update documentation to reflect the fact that
> /proc/sys/net/ipv4/route/max_size is no longer used for ipv4.
> 
> Signed-off-by: Ani Sinha <ani@arista.com>
> ---
>  Documentation/networking/ip-sysctl.txt |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
> index 9bffdfc..2a28261 100644
> --- a/Documentation/networking/ip-sysctl.txt
> +++ b/Documentation/networking/ip-sysctl.txt
> @@ -64,8 +64,10 @@ fwmark_reflect - BOOLEAN
>  	Default: 0
> 
>  route/max_size - INTEGER
> -	Maximum number of routes allowed in the kernel.  Increase
> -	this when using large numbers of interfaces and/or routes.
> +	Post linux kernel 3.6, this is deprecated for ipv4 as route cache is no
> +	longer used. For ipv6, this is used to limit the maximum number of ipv6
> +	routes allowed in the kernel.  Increase this when using large numbers of
> +	interfaces and/or routes.

Now imagine you are reading ip-sysctl.txt, the new text is all wrong.
Something like:
	Limit on the size of the ip route caches.
	Ignored for ipv4 after kernel 3.6 because the ipv4 route cache was removed.
	Increase this when using large numbers of interfaces and/or routes.
would read better.

	David

^ permalink raw reply

* Re: [net-next PATCH v1 01/11] net: flow_table: create interface for hw match/action tables
From: Or Gerlitz @ 2015-01-07 10:07 UTC (permalink / raw)
  To: John Fastabend
  Cc: Thomas Graf, Scott Feldman, Jiří Pírko,
	Jamal Hadi Salim, simon.horman, Linux Netdev List, David Miller,
	Andy Gospodarek
In-Reply-To: <54AADEFF.3090306@gmail.com>

On Mon, Jan 5, 2015 at 8:59 PM, John Fastabend <john.fastabend@gmail.com> wrote:
>>> +struct sk_buff *net_flow_build_actions_msg(struct net_flow_action **a,
>>> +                                          struct net_device *dev,
>>> +                                          u32 portid, int seq, u8 cmd)
>>> +{
>>> +       struct genlmsghdr *hdr;
>>> +       struct sk_buff *skb;
>>> +       int err = -ENOBUFS;
>>> +
>>> +       skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>
>>
>> genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>
>
> fixed along with the other cases.

small nit here, net_flow_build_actions_msg can be made static, it's
called only from within this file

few more nits... checkpatch --strict produces bunch of "CHECK: Please
use a blank line after function/struct/union/enum declarations"
comments, I guess worth fixing too.

^ permalink raw reply

* Re: [RFC PATCH] unlock rtnl mutex in ic_open_devs while waiting
From: Maarten Lankhorst @ 2015-01-07 10:06 UTC (permalink / raw)
  To: David Miller
  Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel,
	Jay Cliburn, Chris Snook, Sabrina Dubroca, Johannes Berg
In-Reply-To: <20150106.172152.2034122593693302134.davem@davemloft.net>

Op 06-01-15 om 23:21 schreef David Miller:
> From: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Date: Mon, 05 Jan 2015 14:52:06 +0100
>
>> This fixes a deadlock with alx_link_check, which takes the rtnl_mutex in
>> a work item to check the link.
>>
>> I have no idea whether alx should be fixed or ipconfig.c,
>> but this saves 120 seconds off my boot time. ;-)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> I genuinely think that alx_link_check() needs to use a smaller hammer
> to do it's locking, there is no reason to use the RTNL mutex.
>
> A driver private mutex will probably work just as well and not have
> this problem.

I guess alx_check_link uses the rtnl_lock for serializing against any possible alx_reset call.
The alternative is stopping check_link work before running anything that changes the device state.
Does the below patch look sane instead?
---
diff --git a/drivers/net/ethernet/atheros/alx/alx.h b/drivers/net/ethernet/atheros/alx/alx.h
index 8fc93c5f6abc..354f155b3144 100644
--- a/drivers/net/ethernet/atheros/alx/alx.h
+++ b/drivers/net/ethernet/atheros/alx/alx.h
@@ -88,6 +88,8 @@ struct alx_priv {
 		unsigned int size;
 	} descmem;
 
+	bool stop_link_check;
+
 	/* protect int_mask updates */
 	spinlock_t irq_lock;
 	u32 int_mask;
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index e398eda07298..ae93b8052cbf 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -315,7 +316,8 @@ static irqreturn_t alx_intr_handle(struct alx_priv *alx, u32 intr)
 		 */
 		alx->int_mask &= ~ALX_ISR_PHY;
 		write_int_mask = true;
-		alx_schedule_link_check(alx);
+		if (!alx->stop_link_check)
+			alx_schedule_link_check(alx);
 	}
 
 	if (intr & (ALX_ISR_TX_Q0 | ALX_ISR_RX_Q0)) {
@@ -742,6 +744,17 @@ static netdev_features_t alx_fix_features(struct net_device *netdev,
 	return features;
 }
 
+static void alx_disable_link_check(struct alx_priv *alx)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&alx->irq_lock, flags);
+	alx->stop_link_check = true;
+	spin_unlock_irqrestore(&alx->irq_lock, flags);
+
+	cancel_work_sync(&alx->link_check_wk);
+}
+
 static void alx_netif_stop(struct alx_priv *alx)
 {
 	alx->dev->trans_start = jiffies;
@@ -756,6 +769,7 @@ static void alx_halt(struct alx_priv *alx)
 {
 	struct alx_hw *hw = &alx->hw;
 
+	alx_disable_link_check(alx);
 	alx_netif_stop(alx);
 	hw->link_speed = SPEED_UNKNOWN;
 	hw->duplex = DUPLEX_UNKNOWN;
@@ -788,6 +802,7 @@ static void alx_activate(struct alx_priv *alx)
 	/* clear old interrupts */
 	alx_write_mem32(&alx->hw, ALX_ISR, ~(u32)ALX_ISR_DIS);
 
+	alx->stop_link_check = false;
 	alx_irq_enable(alx);
 
 	alx_schedule_link_check(alx);
@@ -850,6 +865,7 @@ static int __alx_open(struct alx_priv *alx, bool resume)
 	/* clear old interrupts */
 	alx_write_mem32(&alx->hw, ALX_ISR, ~(u32)ALX_ISR_DIS);
 
+	alx->stop_link_check = false;
 	alx_irq_enable(alx);
 
 	if (!resume)
@@ -966,9 +982,7 @@ static void alx_link_check(struct work_struct *work)
 
 	alx = container_of(work, struct alx_priv, link_check_wk);
 
-	rtnl_lock();
 	alx_check_link(alx);
-	rtnl_unlock();
 }
 

^ permalink raw reply related

* RE: [PATCH 2/6] vxlan: Group Policy extension
From: David Laight @ 2015-01-07 10:03 UTC (permalink / raw)
  To: 'Alexei Starovoitov', Thomas Graf
  Cc: David S. Miller, Jesse Gross, Stephen Hemminger, Pravin Shelar,
	netdev@vger.kernel.org, dev@openvswitch.org
In-Reply-To: <CAADnVQ+EO34qYbK+sji-Da3CMkn+-V-9Uvdar_WgxRQ4_+JFXA@mail.gmail.com>

From: Alexei Starovoitov
> On Tue, Jan 6, 2015 at 6:05 PM, Thomas Graf <tgraf@suug.ch> wrote:
> > +struct vxlan_gbp {
> > +#ifdef __LITTLE_ENDIAN_BITFIELD
> > +       __u8    reserved_flags1:3,
> ...
> > +       __be16 policy_id;
> > +} __packed;
> 
> are you sure that compiler will be smart enough
> to do single short load when you pack
> u8 + struct vxlan_gbp inside struct vxlanhdr ?
> I suspect compiler will use two byte loads
> with shifts and ors every time to access policy_id.
> Even it works ok, I think this struct layout is ugly.
> imo would be much easier to read if you replace
> the whole vxlanhdr with vxlanhdr_gbp
> or split vxlanhdr into two 32-bit structs.
> then __packed hacks won't be needed.

Also, if you are writing the values then you need to write
all the members of the bitfield in order to get a single write.

Basically you are much better off using explicit masks.

	David


^ permalink raw reply

* Re: [PATCH net-next v2 0/7] Involve rhashtable_lookup_insert routine
From: Thomas Graf @ 2015-01-07  9:54 UTC (permalink / raw)
  To: Ying Xue
  Cc: davem, jon.maloy, Paul.Gortmaker, erik.hugne, netdev,
	tipc-discussion
In-Reply-To: <1420609318-3261-1-git-send-email-ying.xue@windriver.com>

On 01/07/15 at 01:41pm, Ying Xue wrote:
> The series aims to involve rhashtable_lookup_insert() to guarantee
> that the process of lookup and insertion of an object from/into hash
> table is finished atomically, allowing rhashtable's users not to
> introduce an extra lock during search and insertion. For example,
> tipc socket is the first user benefiting from this enhancement. 
> 
> v2 changes:

Thanks, this looks excellent. I think patch 6/7 can be dropped,
otherwise this looks ready to go. I acked all other changes.

^ permalink raw reply

* Re: [PATCH net-next v2 6/7] rhashtable: initialize atomic nelems variable
From: Thomas Graf @ 2015-01-07  9:53 UTC (permalink / raw)
  To: Ying Xue
  Cc: davem, jon.maloy, Paul.Gortmaker, erik.hugne, netdev,
	tipc-discussion
In-Reply-To: <1420609318-3261-7-git-send-email-ying.xue@windriver.com>

On 01/07/15 at 01:41pm, Ying Xue wrote:
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> Cc: Thomas Graf <tgraf@suug.ch>

Is this really needed at all? We initialize the full rhashtable
struct to 0 in rhashtable_init().

^ permalink raw reply

* Re: [PATCH net-next v2 5/7] rhashtable: avoid unnecessary wakeup for worker queue
From: Thomas Graf @ 2015-01-07  9:50 UTC (permalink / raw)
  To: Ying Xue
  Cc: davem, jon.maloy, Paul.Gortmaker, erik.hugne, netdev,
	tipc-discussion
In-Reply-To: <1420609318-3261-6-git-send-email-ying.xue@windriver.com>

On 01/07/15 at 01:41pm, Ying Xue wrote:
> Move condition statements of verifying whether hash table size exceeds
> its maximum threshold or reaches its minimum threshold from resizing
> functions to resizing decision functions, avoiding unnecessary wakeup
> for worker queue thread.
> 
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> Cc: Thomas Graf <tgraf@suug.ch>

Good optimization, thanks!

Acked-by: Thomas Graf <tgraf@suug.ch>

Can you do a follow-up patch and add a note in rhashtable.h to
indicate the implementation of the grow and shrink decision function
must enforce min/max shift?

^ permalink raw reply

* [PATCH -next] r8169: add support for xmit_more
From: Florian Westphal @ 2015-01-07  9:49 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

Delay update of hw tail descriptor if we know that another skb is going
to be sent.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 drivers/net/ethernet/realtek/r8169.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 14a1c5c..3a28059 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7049,6 +7049,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 	u32 status, len;
 	u32 opts[2];
 	int frags;
+	bool stop_queue;
 
 	if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) {
 		netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n");
@@ -7105,11 +7106,16 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 
 	tp->cur_tx += frags + 1;
 
-	RTL_W8(TxPoll, NPQ);
+	stop_queue = !TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS);
 
-	mmiowb();
+	if (!skb->xmit_more || stop_queue ||
+	    netif_xmit_stopped(netdev_get_tx_queue(dev, 0))) {
+		RTL_W8(TxPoll, NPQ);
+
+		mmiowb();
+	}
 
-	if (!TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS)) {
+	if (stop_queue) {
 		/* Avoid wrongly optimistic queue wake-up: rtl_tx thread must
 		 * not miss a ring update when it notices a stopped queue.
 		 */
-- 
2.0.5

^ permalink raw reply related

* [PATCH next] net: e1000e: support txtd update delay via xmit_more
From: Florian Westphal @ 2015-01-07  9:49 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal

Don't update tx tail descriptor if queue hasn't been stopped
and we know at least one more skb will be sent right away.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 332a298..214cb78 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5444,16 +5444,6 @@ static void e1000_tx_queue(struct e1000_ring *tx_ring, int tx_flags, int count)
 	wmb();
 
 	tx_ring->next_to_use = i;
-
-	if (adapter->flags2 & FLAG2_PCIM2PCI_ARBITER_WA)
-		e1000e_update_tdt_wa(tx_ring, i);
-	else
-		writel(i, tx_ring->tail);
-
-	/* we need this if more than one processor can write to our tail
-	 * at a time, it synchronizes IO on IA64/Altix systems
-	 */
-	mmiowb();
 }
 
 #define MINIMUM_DHCP_PACKET_SIZE 282
@@ -5653,6 +5643,19 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
 				    (MAX_SKB_FRAGS *
 				     DIV_ROUND_UP(PAGE_SIZE,
 						  adapter->tx_fifo_limit) + 2));
+
+		if (!skb->xmit_more ||
+		    netif_xmit_stopped(netdev_get_tx_queue(netdev, 0))) {
+			if (adapter->flags2 & FLAG2_PCIM2PCI_ARBITER_WA)
+				e1000e_update_tdt_wa(tx_ring, tx_ring->next_to_use);
+			else
+				writel(tx_ring->next_to_use, tx_ring->tail);
+
+			/* we need this if more than one processor can write to our tail
+			 * at a time, it synchronizes IO on IA64/Altix systems
+			 */
+			mmiowb();
+		}
 	} else {
 		dev_kfree_skb_any(skb);
 		tx_ring->buffer_info[first].time_stamp = 0;
-- 
2.0.5

^ permalink raw reply related

* Re: [PATCH net-next v2 4/7] rhashtable: future table needs to be traversed when remove an object
From: Thomas Graf @ 2015-01-07  9:39 UTC (permalink / raw)
  To: Ying Xue
  Cc: davem, jon.maloy, Paul.Gortmaker, erik.hugne, netdev,
	tipc-discussion
In-Reply-To: <1420609318-3261-5-git-send-email-ying.xue@windriver.com>

On 01/07/15 at 01:41pm, Ying Xue wrote:
> When remove an object from hash table, we currently only traverse old
> bucket table to check whether the object exists. If the object is not
> found in it, we will try again. But in the second search loop, we still
> search the object from the old table instead of future table. As a
> result, the object may be not removed from hash table especially when
> resizing is currently in progress and the object is just saved in the
> future table.
> 
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> Cc: Thomas Graf <tgraf@suug.ch>

Nice catch!
Acked-by: Thomas Graf <tgraf@suug.ch>

^ permalink raw reply

* Re: [PATCH net-next v2 2/7] rhashtable: introduce rhashtable_wakeup_worker helper function
From: Thomas Graf @ 2015-01-07  9:34 UTC (permalink / raw)
  To: Ying Xue
  Cc: davem, jon.maloy, Paul.Gortmaker, erik.hugne, netdev,
	tipc-discussion
In-Reply-To: <1420609318-3261-3-git-send-email-ying.xue@windriver.com>

On 01/07/15 at 01:41pm, Ying Xue wrote:
> Introduce rhashtable_wakeup_worker() helper function to reduce
> duplicated code where to wake up worker.
> 
> By the way, as long as the both "future_tbl" and "tbl" bucket table
> pointers point to the same bucket array, we should try to wake up
> the resizing worker thread, otherwise, it indicates the work of
> resizing hash table is not finished yet. However, currently we will
> wake up the worker thread only when the two pointers point to
> different bucket array. Obviously this is wrong. So, the issue is
> also fixed as well in the patch.
> 
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> Cc: Thomas Graf <tgraf@suug.ch>

Acked-by: Thomas Graf <tgraf@suug.ch>

^ permalink raw reply

* Re: [PATCH 6/6] driver/net/irda: Replace timeval with ktime_t in vlsi_ir
From: Arnd Bergmann @ 2015-01-07  8:59 UTC (permalink / raw)
  To: Chunyan Zhang; +Cc: samuel, zhang.lyra, netdev, linux-kernel
In-Reply-To: <1420601978-15866-7-git-send-email-zhang.chunyan@linaro.org>

On Wednesday 07 January 2015 11:39:38 Chunyan Zhang wrote:
> This patch changes the 32-bit time type (timeval) to the 64-bit one
> (ktime_t), since 32-bit time types will break in the year 2038.
> So, I use ktime_t instead of all uses of timeval.
> 
> This patch also changes do_gettimeofday() to ktime_get() accordingly,
> since ktime_get returns a ktime_t, but do_gettimeofday returns a
> struct timeval, and the other reason is that ktime_get() uses
> the monotonic clock.
> 
> This patch uses ktime_us_delta to get the elapsed time of microsecond,
> and uses div_s64_rem to get what seconds & microseconds time elapsed
> for printing.
> 
> This patch also changes the code of function 'vlsi_hard_start_xmit' to
> do the same things as the others drivers, that is passing the remaining
> time into udelay() instead of looping until enough time has passed.
> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>

All six patches look correct to me now, nice work!

I have a few very minor comments still, some of which apply to the
other patches as well:

* Your patch changelog starts each paragraph with 'This patch', which is
  correct but is a little strange to read. You could start the first paragraph
  explaining what the driver currently does and why we want to change it,
  like "The vlsi ir driver uses 'timeval', which we try to remove in the
  kernel because all 32-bit time types will break in the year 2038".

* It would also be good to explain that none of these drivers are actually
  broken regarding y2038 at the moment and that the change is only done to
  remove the need for auditing this fact again.

> @@ -180,8 +181,8 @@ static void vlsi_proc_ndev(struct seq_file *seq, struct net_device *ndev)
>  	vlsi_irda_dev_t *idev = netdev_priv(ndev);
>  	u8 byte;
>  	u16 word;
> -	unsigned delta1, delta2;
> -	struct timeval now;
> +	ktime_t now;
> +	s32 sec, usec;
>  	unsigned iobase = ndev->base_addr;

* you now have a local variable that is only used to be passed into ktime_us_delta.
  While there is no difference to the compiler, I would probably shorten this
  and avoid the local variable by passing the result of ktime_get() directly
  into ktime_us_delta().
  For the drivers that store the 'now' variable in a data structure, doing the
  same change shrinks the driver specific data structure, which is an added
  benefit.

> -		for(;;) {
> -			do_gettimeofday(&now);
> -			if (now.tv_sec > ready.tv_sec ||
> -			    (now.tv_sec==ready.tv_sec && now.tv_usec>=ready.tv_usec))
> -			    	break;
> -			udelay(100);
> -			/* must not sleep here - called under netif_tx_lock! */
> -		}
> +		now = ktime_get();
> +		diff = ktime_us_delta(now, idev->last_rx);
> +		if (mtt > diff)
> +			udelay(mtt - diff);
>  	}

The change looks good, but you remove a useful comment about sleeping inside of
netif_tx_lock. I would not bother adding the same comment to the other drivers,
but it still applies to the udelay call here so I would move it there.
Note that generally we try hard to avoid the use of long 'udelay' calls, but
I wouldn't expect you to change the drivers for this.

When you submit the next version and you have addressed my last comments,
please add

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

	Arnd

^ permalink raw reply

* Re: [PATCH] brcm80211: brcmsmac: dma: Remove some unused functions
From: Arend van Spriel @ 2015-01-07  8:58 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Rickard Strandqvist, Kalle Valo, Larry Finger, Brett Rudley,
	Hante Meuleman, Fabian Frederick, linux-wireless@vger.kernel.org,
	brcm80211-dev-list, Network Development,
	Linux Kernel Mailing List
In-Reply-To: <alpine.DEB.2.02.1501070726320.2058@localhost6.localdomain6>

On 01/07/15 07:29, Julia Lawall wrote:
>
>
> On Wed, 7 Jan 2015, Rickard Strandqvist wrote:
>
>> 2015-01-05 12:06 GMT+01:00 Arend van Spriel<arend@broadcom.com>:
>>> On 01/05/15 11:49, Kalle Valo wrote:
>>>>
>>>> Rickard Strandqvist<rickard_strandqvist@spectrumdigital.se>   writes:
>>>>
>>>>> As I hope you can see I have made some changes regarding the
>>>>> subject-line. Thought it was an advantage to be able to see which file
>>>>> I actually removed something from. There seems to be a big focus on
>>>>> getting right on subject-line right in recent weeks.
>>>>>
>>>>> I wonder why there is a script that takes a file name, and respond
>>>>> with an appropriate subject line?
>>>
>>>
>>> Is there a script for this? Anyway, I would say driver name is enough.
>>> Enough about the subject line ;-) I would like to give some general remarks
>>> as you seem to touch a lot of kernel code. First off, I think it is good to
>>> remove unused stuff. However, I would like some more explanation on your
>>> methodology apart from "partially found by using a static code analysis
>>> program". So a cover-letter explaining that would have been nice (maybe
>>> still is). Things like Kconfig option can affect whether function are used
>>> or not so how did you cover that.
>>>
>>> Regards,
>>> Arend
>>>
>>>
>>>> I don't think you can really automate this as some drivers do this a bit
>>>> differently. You always need to manually check the commit log.
>>>>
>>>>> But ok, I change my script accordingly. Should I submit the patch again?
>>>>
>>>>
>>>> Yes, please resubmit.
>>>>
>>>
>>
>> Hi Arend
>>
>> Yes, a script that had been excellent, I think!
>> I have one as part of my git send-email script, until a week ago, it
>> was enough that I removed the "drivers/" and changed all "/" to ": "
>> I have now been expanded my sed pipe a lot (tell me if anyone is interested)
>> But now I've seen everything from uppercase and [DIR], etc.
>> So I can not understand how anyone should be able to get the right
>> name without a good help.
>>
>> Sure i like to share how I use cppcheck, but is very hesitant to write
>> this with each patch mails I send though!
>>
>> I run:
>> cppcheck --force --quiet --enable=all .
>>
>> Or a specific file instead of .
>>
>> This will include, among other things get a lot of error message such,
>> +4000 for the kernel.
>> (style) The function 'xxx' is never used
>>
>> For these I made a script that searched through all the files after
>> the function name (cppcheck missed a few). And save the rest so I go
>> through them and possibly send patches.
>
> I think that the question was about what methodology is cppcheck using to
> find the given issue.  But probably cppcheck is a black box that does
> whatever it does, so the user doesn't know what the rationale is.

That would have been nice, but I also wanted to know what his subsequent 
steps were to validate the output from cppcheck. I went through some 
cppcheck web pages, but they only elaborate on what is can do and not 
the how. But hey, it is an open-source tool so there is always the code 
to check.

> However, I think you mentioned that cppcheck found only some of the
> issues.  You could thus describe what was the methodology for finding the
> other ones.

Maybe upon removing an unused function it had a ripple effect on others 
becoming unused as well? Still this is speculating and with this kind of 
cleanup effort all over the place it is better to review the methodology.

Regards,
Arend

> julia

^ permalink raw reply

* Re: [PATCH] brcm80211: brcmsmac: dma: Remove some unused functions
From: Arend van Spriel @ 2015-01-07  8:57 UTC (permalink / raw)
  To: Rickard Strandqvist
  Cc: Kalle Valo, Larry Finger, Brett Rudley, Hante Meuleman,
	Fabian Frederick, linux-wireless@vger.kernel.org,
	brcm80211-dev-list, Network Development,
	Linux Kernel Mailing List, Julia Lawall
In-Reply-To: <CAKXHbyNBV6=DPF4n0onzhEqAuS-PqLS5pwZj_OitRuYMvgpjvw@mail.gmail.com>

On 01/07/15 00:33, Rickard Strandqvist wrote:
> 2015-01-05 12:06 GMT+01:00 Arend van Spriel<arend@broadcom.com>:
>> On 01/05/15 11:49, Kalle Valo wrote:
>>>
>>> Rickard Strandqvist<rickard_strandqvist@spectrumdigital.se>   writes:
>>>
>>>> As I hope you can see I have made some changes regarding the
>>>> subject-line. Thought it was an advantage to be able to see which file
>>>> I actually removed something from. There seems to be a big focus on
>>>> getting right on subject-line right in recent weeks.
>>>>
>>>> I wonder why there is a script that takes a file name, and respond
>>>> with an appropriate subject line?
>>
>>
>> Is there a script for this? Anyway, I would say driver name is enough.
>> Enough about the subject line ;-) I would like to give some general remarks
>> as you seem to touch a lot of kernel code. First off, I think it is good to
>> remove unused stuff. However, I would like some more explanation on your
>> methodology apart from "partially found by using a static code analysis
>> program". So a cover-letter explaining that would have been nice (maybe
>> still is). Things like Kconfig option can affect whether function are used
>> or not so how did you cover that.
>>
>> Regards,
>> Arend
>>
>>
>>> I don't think you can really automate this as some drivers do this a bit
>>> differently. You always need to manually check the commit log.
>>>
>>>> But ok, I change my script accordingly. Should I submit the patch again?
>>>
>>>
>>> Yes, please resubmit.
>>>
>>
>
> Hi Arend
>
> Yes, a script that had been excellent, I think!
> I have one as part of my git send-email script, until a week ago, it
> was enough that I removed the "drivers/" and changed all "/" to ": "
> I have now been expanded my sed pipe a lot (tell me if anyone is interested)
> But now I've seen everything from uppercase and [DIR], etc.
> So I can not understand how anyone should be able to get the right
> name without a good help.
>
> Sure i like to share how I use cppcheck, but is very hesitant to write
> this with each patch mails I send though!
>
> I run:
> cppcheck --force --quiet --enable=all .

And . is the top-level directory in the kernel repo? I am not familiar 
with cppcheck, but does it invoke the kernel Makefile. From a quick 
glance on cppcheck webpage I guess you could enable only the unused 
function checker.

> Or a specific file instead of .
>
> This will include, among other things get a lot of error message such,
> +4000 for the kernel.
> (style) The function 'xxx' is never used
>
> For these I made a script that searched through all the files after
> the function name (cppcheck missed a few). And save the rest so I go
> through them and possibly send patches.

All the file? Within the same driver or kernel-wide. So now "go through 
them" means compile testing with applicable Kconfig selections?

Gr. AvS

^ permalink raw reply

* Re: [PATCH v8 34/50] vhost/net: virtio 1.0 byte swap
From: Michael S. Tsirkin @ 2015-01-07  8:57 UTC (permalink / raw)
  To: Greg Kurz
  Cc: Alex Williamson, thuth, kvm, rusty, netdev, linux-kernel,
	virtualization, dahi, pbonzini, David Miller
In-Reply-To: <20150107093105.6ef918f1@bahia.local>

On Wed, Jan 07, 2015 at 09:31:05AM +0100, Greg Kurz wrote:
> On Tue, 06 Jan 2015 16:55:30 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > On Mon, 2014-12-01 at 18:05 +0200, Michael S. Tsirkin wrote:
> > > I had to add an explicit tag to suppress compiler warning:
> > > gcc isn't smart enough to notice that
> > > len is always initialized since function is called with size > 0.
> > 
> > I'm getting a panic inside a guest when this change is applied on the
> > host.  I identified this patch via bisect and confirmed by reverting it
> > from v3.19-rc2.  Guest is centos6.  Thanks,
> > 
> > Alex
> > 
> > commit 8b38694a2dc8b18374310df50174f1e4376d6824
> > Author: Michael S. Tsirkin <mst@redhat.com>
> > Date:   Fri Oct 24 14:19:48 2014 +0300
> > 
> >     vhost/net: virtio 1.0 byte swap
> >     
> >     I had to add an explicit tag to suppress compiler warning:
> >     gcc isn't smart enough to notice that
> >     len is always initialized since function is called with size > 0.
> >     
> >     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >     Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > 
> 
> This chunk looks suspicious:
> 
> -	heads[headcount - 1].len += datalen;
> +	heads[headcount - 1].len = cpu_to_vhost32(vq, len - datalen);
> 
> s/len - datalen/len + datalen/ ?

Indeed!
I just sent a patch fixing this, thanks a lot.


> > XML chunk:
> > 
> >     <interface type='direct'>
> >       <mac address='52:54:00:64:f3:34'/>
> >       <source dev='iscsinet0' mode='bridge'/>
> >       <model type='virtio'/>
> >       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> >     </interface>
> > 
> > Panic log:
> > 
> > <1>BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> > <1>IP: [<ffffffffa0079469>] virtnet_poll+0x4f9/0x910 [virtio_net]
> > <4>PGD 1aa2f4067 PUD 1aa2f5067 PMD 0 
> > <4>Oops: 0000 [#1] SMP 
> > <4>last sysfs file: /sys/devices/pci0000:00/0000:00:03.0/virtio0/net/eth9/ifindex
> > <4>CPU 0 
> > <4>Modules linked in: 8021q garp stp llc ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4 iptable_filter ip_tables ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables ipv6 uinput microcode snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc igbvf nvidia(P)(U) i2c_core tg3 ptp pps_core virtio_balloon virtio_net virtio_console ext4 jbd2 mbcache virtio_blk virtio_pci virtio_ring virtio pata_acpi ata_generic ata_piix dm_mirror dm_region_hash dm_log dm_mod [last unloaded: speedstep_lib]
> > <4>
> > <4>Pid: 1374, comm: NetworkManager Tainted: P           ---------------    2.6.32-431.23.3.el6.centos.plus.x86_64 #1 QEMU Standard PC (i440FX + PIIX, 1996)
> > <4>RIP: 0010:[<ffffffffa0079469>]  [<ffffffffa0079469>] virtnet_poll+0x4f9/0x910 [virtio_net]
> > <4>RSP: 0018:ffff880028203e48  EFLAGS: 00010246
> > <4>RAX: ffff8801a3383d00 RBX: ffff8801a6aaf480 RCX: ffff8801aa20b6e0
> > <4>RDX: 00000000000000c0 RSI: ffff8801a3383c00 RDI: ffff8801a3383cc0
> > <4>RBP: ffff880028203ed8 R08: 000000000000009e R09: ffff8801aa1d800c
> > <4>R10: 0000000000000218 R11: 0000000000000000 R12: ffff8801aa20b6e0
> > <4>R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > <4>FS:  00007febf114d800(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
> > <4>CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > <4>CR2: 0000000000000010 CR3: 00000001aa793000 CR4: 00000000000006f0
> > <4>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > <4>DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > <4>Process NetworkManager (pid: 1374, threadinfo ffff8801a74ba000, task ffff8801a8d56040)
> > <4>Stack:
> > <4> ffff8801aa1d8000 000000000000009e ffff8801aa20b6e0 ffff8801aa20b718
> > <4><d> ffff8801aa20b780 ffff8801aa1d800c ffff8801a6aaf4b8 ffff8801aa20b020
> > <4><d> 0000000000000080 ffff8801aa20b708 0000000000000001 00001f5981a830c8
> > <4>Call Trace:
> > <4> <IRQ> 
> > <4> [<ffffffff8146ae33>] net_rx_action+0x103/0x2f0
> > <4> [<ffffffff8107a5f1>] __do_softirq+0xc1/0x1e0
> > <4> [<ffffffff8100c30c>] ? call_softirq+0x1c/0x30
> > <4> [<ffffffff8100c30c>] call_softirq+0x1c/0x30
> > <4> <EOI> 
> > <4> [<ffffffff8100fa75>] ? do_softirq+0x65/0xa0
> > <4> [<ffffffff8107b2ea>] local_bh_enable+0x9a/0xb0
> > <4> [<ffffffffa007813a>] virtnet_napi_enable+0x4a/0x60 [virtio_net]
> > <4> [<ffffffffa0078ebf>] virtnet_open+0x4f/0x60 [virtio_net]
> > <4> [<ffffffff81467691>] dev_open+0xa1/0x100
> > <4> [<ffffffff81466751>] dev_change_flags+0xa1/0x1d0
> > <4> [<ffffffff81474a59>] do_setlink+0x169/0x8b0
> > <4> [<ffffffff814770b6>] ? rtnl_fill_ifinfo+0x946/0xcb0
> > <4> [<ffffffff812a3d24>] ? nla_parse+0x34/0x110
> > <4> [<ffffffff8147659e>] rtnl_setlink+0xee/0x130
> > <4> [<ffffffff81475b67>] rtnetlink_rcv_msg+0x2d7/0x340
> > <4> [<ffffffff81231e14>] ? socket_has_perm+0x74/0x90
> > <4> [<ffffffff81475890>] ? rtnetlink_rcv_msg+0x0/0x340
> > <4> [<ffffffff814910a9>] netlink_rcv_skb+0xa9/0xd0
> > <4> [<ffffffff81475875>] rtnetlink_rcv+0x25/0x40
> > <4> [<ffffffff81490cdb>] netlink_unicast+0x2db/0x320
> > <4> [<ffffffff81491750>] netlink_sendmsg+0x2c0/0x3d0
> > <4> [<ffffffff814520c3>] sock_sendmsg+0x123/0x150
> > <4> [<ffffffff81453d73>] ? sock_recvmsg+0x133/0x160
> > <4> [<ffffffff8109afa0>] ? autoremove_wake_function+0x0/0x40
> > <4> [<ffffffff81136941>] ? lru_cache_add_lru+0x21/0x40
> > <4> [<ffffffff8115522d>] ? page_add_new_anon_rmap+0x9d/0xf0
> > <4> [<ffffffff8114aeef>] ? handle_pte_fault+0x4af/0xb00
> > <4> [<ffffffff81451f14>] ? move_addr_to_kernel+0x64/0x70
> > <4> [<ffffffff814538b6>] __sys_sendmsg+0x406/0x420
> > <4> [<ffffffff8104a98c>] ? __do_page_fault+0x1ec/0x480
> > <4> [<ffffffff814523d9>] ? sys_sendto+0x139/0x190
> > <4> [<ffffffff8103ea6c>] ? kvm_clock_read+0x1c/0x20
> > <4> [<ffffffff81453ad9>] sys_sendmsg+0x49/0x90
> > <4> [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
> > <4>Code: 83 e0 00 00 00 00 10 00 00 48 03 93 d0 00 00 00 66 83 42 04 01 8b 93 cc 00 00 00 48 8b b3 d0 00 00 00 80 4c 16 10 20 44 2b 68 0c <4d> 8b 76 10 75 89 e9 d1 fd ff ff 0f 1f 40 00 a8 02 74 0d 0f b6 
> > <1>RIP  [<ffffffffa0079469>] virtnet_poll+0x4f9/0x910 [virtio_net]
> > <4> RSP <ffff880028203e48>
> > <4>CR2: 0000000000000010
> > 
> > 
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> > 

^ permalink raw reply

* [PATCH] vhost/net: length miscalculation
From: Michael S. Tsirkin @ 2015-01-07  8:55 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alex Williamson, Greg Kurz, kvm, virtualization, netdev

commit 8b38694a2dc8b18374310df50174f1e4376d6824
    vhost/net: virtio 1.0 byte swap
had this chunk:
-       heads[headcount - 1].len += datalen;
+       heads[headcount - 1].len = cpu_to_vhost32(vq, len - datalen);

This adds datalen with the wrong sign, causing guest panics.

Fixes: 8b38694a2dc8b18374310df50174f1e4376d6824
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Suggested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Alex, could you please confirm this fixes the crash for you?

 drivers/vhost/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 14419a8..d415d69 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -538,7 +538,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
 		++headcount;
 		seg += in;
 	}
-	heads[headcount - 1].len = cpu_to_vhost32(vq, len - datalen);
+	heads[headcount - 1].len = cpu_to_vhost32(vq, len + datalen);
 	*iovcount = seg;
 	if (unlikely(log))
 		*log_num = nlogs;
-- 
MST

^ permalink raw reply related

* Re: [PATCH] sh_eth: Fix access to TRSCER register
From: Geert Uytterhoeven @ 2015-01-07  8:42 UTC (permalink / raw)
  To: Nobuhiro Iwamatsu
  Cc: netdev@vger.kernel.org, Yoshihiro Shimoda, Linux-sh list
In-Reply-To: <1420608947-6861-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com>

Hi Iwamatsu-san,

On Wed, Jan 7, 2015 at 6:35 AM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.yj@renesas.com> wrote:
> TRSCER register is configured differently by SoCs. TRSCER of R-Car is
> RINT8 bit only valid, other bits are reserved bits.
> This removes access to TRSCER register reserve bit.
>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
> ---
>  drivers/net/ethernet/renesas/sh_eth.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index c29ba80..59ee457 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -1294,7 +1294,11 @@ static int sh_eth_dev_init(struct net_device *ndev, bool start)
>         /* Frame recv control (enable multiple-packets per rx irq) */
>         sh_eth_write(ndev, RMCR_RNC, RMCR);
>
> -       sh_eth_write(ndev, DESC_I_RINT8 | DESC_I_RINT5 | DESC_I_TINT2, TRSCER);
> +       if (mdp->cd->register_type == SH_ETH_REG_FAST_RCAR)

This catches both the R-Car Gen1 and R-Car Gen2 cases.
According to the datasheets, r8a7778/9 do have the other bits?
Only R-Car Gen2 doesn't have them.

> +               sh_eth_write(ndev, DESC_I_RINT8, TRSCER);
> +       else
> +               sh_eth_write(ndev, DESC_I_RINT8 | DESC_I_RINT5 | DESC_I_TINT2,
> +                            TRSCER);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH v8 34/50] vhost/net: virtio 1.0 byte swap
From: Greg Kurz @ 2015-01-07  8:31 UTC (permalink / raw)
  To: Alex Williamson
  Cc: thuth, kvm, Michael S. Tsirkin, rusty, netdev, linux-kernel,
	virtualization, dahi, pbonzini, David Miller
In-Reply-To: <1420588530.3541.116.camel@redhat.com>

On Tue, 06 Jan 2015 16:55:30 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 2014-12-01 at 18:05 +0200, Michael S. Tsirkin wrote:
> > I had to add an explicit tag to suppress compiler warning:
> > gcc isn't smart enough to notice that
> > len is always initialized since function is called with size > 0.
> 
> I'm getting a panic inside a guest when this change is applied on the
> host.  I identified this patch via bisect and confirmed by reverting it
> from v3.19-rc2.  Guest is centos6.  Thanks,
> 
> Alex
> 
> commit 8b38694a2dc8b18374310df50174f1e4376d6824
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Fri Oct 24 14:19:48 2014 +0300
> 
>     vhost/net: virtio 1.0 byte swap
>     
>     I had to add an explicit tag to suppress compiler warning:
>     gcc isn't smart enough to notice that
>     len is always initialized since function is called with size > 0.
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 

This chunk looks suspicious:

-	heads[headcount - 1].len += datalen;
+	heads[headcount - 1].len = cpu_to_vhost32(vq, len - datalen);

s/len - datalen/len + datalen/ ?

> XML chunk:
> 
>     <interface type='direct'>
>       <mac address='52:54:00:64:f3:34'/>
>       <source dev='iscsinet0' mode='bridge'/>
>       <model type='virtio'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
>     </interface>
> 
> Panic log:
> 
> <1>BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> <1>IP: [<ffffffffa0079469>] virtnet_poll+0x4f9/0x910 [virtio_net]
> <4>PGD 1aa2f4067 PUD 1aa2f5067 PMD 0 
> <4>Oops: 0000 [#1] SMP 
> <4>last sysfs file: /sys/devices/pci0000:00/0000:00:03.0/virtio0/net/eth9/ifindex
> <4>CPU 0 
> <4>Modules linked in: 8021q garp stp llc ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4 iptable_filter ip_tables ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables ipv6 uinput microcode snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc igbvf nvidia(P)(U) i2c_core tg3 ptp pps_core virtio_balloon virtio_net virtio_console ext4 jbd2 mbcache virtio_blk virtio_pci virtio_ring virtio pata_acpi ata_generic ata_piix dm_mirror dm_region_hash dm_log dm_mod [last unloaded: speedstep_lib]
> <4>
> <4>Pid: 1374, comm: NetworkManager Tainted: P           ---------------    2.6.32-431.23.3.el6.centos.plus.x86_64 #1 QEMU Standard PC (i440FX + PIIX, 1996)
> <4>RIP: 0010:[<ffffffffa0079469>]  [<ffffffffa0079469>] virtnet_poll+0x4f9/0x910 [virtio_net]
> <4>RSP: 0018:ffff880028203e48  EFLAGS: 00010246
> <4>RAX: ffff8801a3383d00 RBX: ffff8801a6aaf480 RCX: ffff8801aa20b6e0
> <4>RDX: 00000000000000c0 RSI: ffff8801a3383c00 RDI: ffff8801a3383cc0
> <4>RBP: ffff880028203ed8 R08: 000000000000009e R09: ffff8801aa1d800c
> <4>R10: 0000000000000218 R11: 0000000000000000 R12: ffff8801aa20b6e0
> <4>R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> <4>FS:  00007febf114d800(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
> <4>CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>CR2: 0000000000000010 CR3: 00000001aa793000 CR4: 00000000000006f0
> <4>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> <4>DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> <4>Process NetworkManager (pid: 1374, threadinfo ffff8801a74ba000, task ffff8801a8d56040)
> <4>Stack:
> <4> ffff8801aa1d8000 000000000000009e ffff8801aa20b6e0 ffff8801aa20b718
> <4><d> ffff8801aa20b780 ffff8801aa1d800c ffff8801a6aaf4b8 ffff8801aa20b020
> <4><d> 0000000000000080 ffff8801aa20b708 0000000000000001 00001f5981a830c8
> <4>Call Trace:
> <4> <IRQ> 
> <4> [<ffffffff8146ae33>] net_rx_action+0x103/0x2f0
> <4> [<ffffffff8107a5f1>] __do_softirq+0xc1/0x1e0
> <4> [<ffffffff8100c30c>] ? call_softirq+0x1c/0x30
> <4> [<ffffffff8100c30c>] call_softirq+0x1c/0x30
> <4> <EOI> 
> <4> [<ffffffff8100fa75>] ? do_softirq+0x65/0xa0
> <4> [<ffffffff8107b2ea>] local_bh_enable+0x9a/0xb0
> <4> [<ffffffffa007813a>] virtnet_napi_enable+0x4a/0x60 [virtio_net]
> <4> [<ffffffffa0078ebf>] virtnet_open+0x4f/0x60 [virtio_net]
> <4> [<ffffffff81467691>] dev_open+0xa1/0x100
> <4> [<ffffffff81466751>] dev_change_flags+0xa1/0x1d0
> <4> [<ffffffff81474a59>] do_setlink+0x169/0x8b0
> <4> [<ffffffff814770b6>] ? rtnl_fill_ifinfo+0x946/0xcb0
> <4> [<ffffffff812a3d24>] ? nla_parse+0x34/0x110
> <4> [<ffffffff8147659e>] rtnl_setlink+0xee/0x130
> <4> [<ffffffff81475b67>] rtnetlink_rcv_msg+0x2d7/0x340
> <4> [<ffffffff81231e14>] ? socket_has_perm+0x74/0x90
> <4> [<ffffffff81475890>] ? rtnetlink_rcv_msg+0x0/0x340
> <4> [<ffffffff814910a9>] netlink_rcv_skb+0xa9/0xd0
> <4> [<ffffffff81475875>] rtnetlink_rcv+0x25/0x40
> <4> [<ffffffff81490cdb>] netlink_unicast+0x2db/0x320
> <4> [<ffffffff81491750>] netlink_sendmsg+0x2c0/0x3d0
> <4> [<ffffffff814520c3>] sock_sendmsg+0x123/0x150
> <4> [<ffffffff81453d73>] ? sock_recvmsg+0x133/0x160
> <4> [<ffffffff8109afa0>] ? autoremove_wake_function+0x0/0x40
> <4> [<ffffffff81136941>] ? lru_cache_add_lru+0x21/0x40
> <4> [<ffffffff8115522d>] ? page_add_new_anon_rmap+0x9d/0xf0
> <4> [<ffffffff8114aeef>] ? handle_pte_fault+0x4af/0xb00
> <4> [<ffffffff81451f14>] ? move_addr_to_kernel+0x64/0x70
> <4> [<ffffffff814538b6>] __sys_sendmsg+0x406/0x420
> <4> [<ffffffff8104a98c>] ? __do_page_fault+0x1ec/0x480
> <4> [<ffffffff814523d9>] ? sys_sendto+0x139/0x190
> <4> [<ffffffff8103ea6c>] ? kvm_clock_read+0x1c/0x20
> <4> [<ffffffff81453ad9>] sys_sendmsg+0x49/0x90
> <4> [<ffffffff8100b072>] system_call_fastpath+0x16/0x1b
> <4>Code: 83 e0 00 00 00 00 10 00 00 48 03 93 d0 00 00 00 66 83 42 04 01 8b 93 cc 00 00 00 48 8b b3 d0 00 00 00 80 4c 16 10 20 44 2b 68 0c <4d> 8b 76 10 75 89 e9 d1 fd ff ff 0f 1f 40 00 a8 02 74 0d 0f b6 
> <1>RIP  [<ffffffffa0079469>] virtnet_poll+0x4f9/0x910 [virtio_net]
> <4> RSP <ffff880028203e48>
> <4>CR2: 0000000000000010
> 
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

^ permalink raw reply

* My name is Gatan Magsino
From: Sanders M, Tjeu @ 2015-01-07  8:00 UTC (permalink / raw)






My name is Gatan Magsino, I work with Mediterranean Bank in Malta. Can i trust you with a business worth 8.3 million USD? Please reply ONLY to my private email: mgatan@rogers.com<mailto:mgatan@rogers.com> for more information.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox