From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Yidong Ren <Yidong.Ren@microsoft.com>
Cc: KY Srinivasan <kys@microsoft.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Stephen Hemminger <sthemmin@microsoft.com>,
"David S. Miller" <davem@davemloft.net>,
"devel\@linuxdriverproject.org" <devel@linuxdriverproject.org>,
"netdev\@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Madhan Sivakumar <madhans@microsoft.com>
Subject: Re: [PATCH v3] hv_netvsc: Add per-cpu ethtool stats for netvsc
Date: Tue, 24 Jul 2018 13:00:59 +0200 [thread overview]
Message-ID: <87k1pkrj84.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <SN6PR2101MB11359728E41C4B2AB8E7736D84550@SN6PR2101MB1135.namprd21.prod.outlook.com> (Yidong Ren's message of "Tue, 24 Jul 2018 01:42:06 +0000")
Yidong Ren <Yidong.Ren@microsoft.com> writes:
>> From: Yidong Ren <yidren@linuxonhyperv.com>
>> Sent: Monday, July 23, 2018 6:26 PM
>> + pcpu_sum = kvmalloc(sizeof(struct netvsc_ethtool_pcpu_stats) *
>> + num_present_cpus(), GFP_KERNEL);
>
> Since there is no plan for CPU hotplug in Hyper-V in short term, it is fine
> to use num_present_cpus for now. We can move to debugfs later if necessary.
While you do for_each_present_cpu() in netvsc_get_ethtool_stats(),
netvsc_get_pcpu_stats() does for_each_possible_cpu(). This looks
inconsistent.
The allocation you're doing here is short-lived so I would suggest you
use possible_cpus everywhere. Even knowing there's no CPU hotplug on
Hyper-V at this moment, it can appear later and we'll get a hard-to-find
issue. Moreover, we may consider using netvsc driver on e.g. KVM with
Hyper-V enlightenments and KVM has CPU hotplug already.
--
Vitaly
WARNING: multiple messages have this Message-ID (diff)
From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Yidong Ren <Yidong.Ren@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Haiyang Zhang <haiyangz@microsoft.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Madhan Sivakumar <madhans@microsoft.com>,
"devel@linuxdriverproject.org" <devel@linuxdriverproject.org>,
"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v3] hv_netvsc: Add per-cpu ethtool stats for netvsc
Date: Tue, 24 Jul 2018 13:00:59 +0200 [thread overview]
Message-ID: <87k1pkrj84.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <SN6PR2101MB11359728E41C4B2AB8E7736D84550@SN6PR2101MB1135.namprd21.prod.outlook.com> (Yidong Ren's message of "Tue, 24 Jul 2018 01:42:06 +0000")
Yidong Ren <Yidong.Ren@microsoft.com> writes:
>> From: Yidong Ren <yidren@linuxonhyperv.com>
>> Sent: Monday, July 23, 2018 6:26 PM
>> + pcpu_sum = kvmalloc(sizeof(struct netvsc_ethtool_pcpu_stats) *
>> + num_present_cpus(), GFP_KERNEL);
>
> Since there is no plan for CPU hotplug in Hyper-V in short term, it is fine
> to use num_present_cpus for now. We can move to debugfs later if necessary.
While you do for_each_present_cpu() in netvsc_get_ethtool_stats(),
netvsc_get_pcpu_stats() does for_each_possible_cpu(). This looks
inconsistent.
The allocation you're doing here is short-lived so I would suggest you
use possible_cpus everywhere. Even knowing there's no CPU hotplug on
Hyper-V at this moment, it can appear later and we'll get a hard-to-find
issue. Moreover, we may consider using netvsc driver on e.g. KVM with
Hyper-V enlightenments and KVM has CPU hotplug already.
--
Vitaly
next prev parent reply other threads:[~2018-07-24 11:01 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-13 19:36 [PATCH v2] hv_netvsc: Add per-cpu ethtool stats for netvsc Yidong Ren
2018-06-13 19:36 ` Yidong Ren
2018-06-13 20:57 ` Eric Dumazet
2018-06-13 20:57 ` Eric Dumazet
2018-06-13 21:07 ` Yidong Ren
2018-06-13 21:48 ` Stephen Hemminger
2018-06-13 22:03 ` Yidong Ren
2018-06-13 22:18 ` Stephen Hemminger
2018-07-24 1:26 ` [PATCH v3] " Yidong Ren
2018-07-24 1:26 ` Yidong Ren
2018-07-24 1:42 ` Yidong Ren
2018-07-24 11:00 ` Vitaly Kuznetsov [this message]
2018-07-24 11:00 ` Vitaly Kuznetsov
2018-07-25 22:54 ` Yidong Ren
2018-07-25 22:54 ` Yidong Ren
2018-07-30 17:09 ` [PATCH v4 net-next] " Yidong Ren
2018-07-30 17:34 ` Stephen Hemminger
2018-07-30 19:34 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k1pkrj84.fsf@vitty.brq.redhat.com \
--to=vkuznets@redhat.com \
--cc=Yidong.Ren@microsoft.com \
--cc=davem@davemloft.net \
--cc=devel@linuxdriverproject.org \
--cc=haiyangz@microsoft.com \
--cc=kys@microsoft.com \
--cc=linux-kernel@vger.kernel.org \
--cc=madhans@microsoft.com \
--cc=netdev@vger.kernel.org \
--cc=sthemmin@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.