* [PATCH v3] hv_netvsc: Add per-cpu ethtool stats for netvsc
From: Yidong Ren @ 2018-07-24 1:26 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
David S. Miller, devel, netdev, linux-kernel
Cc: madhans, Yidong Ren
In-Reply-To: <20180613193608.444-1-yidren@linuxonhyperv.com>
From: Yidong Ren <yidren@microsoft.com>
This patch implements following ethtool stats fields for netvsc:
cpu<n>_tx/rx_packets/bytes
cpu<n>_vf_tx/rx_packets/bytes
Corresponding per-cpu counters already exist in current code. Exposing
these counters will help troubleshooting performance issues.
Signed-off-by: Yidong Ren <yidren@microsoft.com>
---
Changes since v2:
* Reimplemented with kvmalloc instead of alloc_percpu
drivers/net/hyperv/hyperv_net.h | 11 ++++
drivers/net/hyperv/netvsc_drv.c | 103 +++++++++++++++++++++++++++++++-
2 files changed, 112 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 4b6e308199d2..a32ded5b4f41 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -873,6 +873,17 @@ struct netvsc_ethtool_stats {
unsigned long wake_queue;
};
+struct netvsc_ethtool_pcpu_stats {
+ u64 rx_packets;
+ u64 rx_bytes;
+ u64 tx_packets;
+ u64 tx_bytes;
+ u64 vf_rx_packets;
+ u64 vf_rx_bytes;
+ u64 vf_tx_packets;
+ u64 vf_tx_bytes;
+};
+
struct netvsc_vf_pcpu_stats {
u64 rx_packets;
u64 rx_bytes;
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index dd1d6e115145..51f9cab2da5b 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1118,6 +1118,64 @@ static void netvsc_get_vf_stats(struct net_device *net,
}
}
+static void netvsc_get_pcpu_stats(struct net_device *net,
+ struct netvsc_ethtool_pcpu_stats *pcpu_tot)
+{
+ struct net_device_context *ndev_ctx = netdev_priv(net);
+ struct netvsc_device *nvdev = rcu_dereference_rtnl(ndev_ctx->nvdev);
+ int i;
+
+ /* fetch percpu stats of vf */
+ for_each_possible_cpu(i) {
+ const struct netvsc_vf_pcpu_stats *stats =
+ per_cpu_ptr(ndev_ctx->vf_stats, i);
+ struct netvsc_ethtool_pcpu_stats *this_tot = &pcpu_tot[i];
+ unsigned int start;
+
+ do {
+ start = u64_stats_fetch_begin_irq(&stats->syncp);
+ this_tot->vf_rx_packets = stats->rx_packets;
+ this_tot->vf_tx_packets = stats->tx_packets;
+ this_tot->vf_rx_bytes = stats->rx_bytes;
+ this_tot->vf_tx_bytes = stats->tx_bytes;
+ } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+ this_tot->rx_packets = this_tot->vf_rx_packets;
+ this_tot->tx_packets = this_tot->vf_tx_packets;
+ this_tot->rx_bytes = this_tot->vf_rx_bytes;
+ this_tot->tx_bytes = this_tot->vf_tx_bytes;
+ }
+
+ /* fetch percpu stats of netvsc */
+ for (i = 0; i < nvdev->num_chn; i++) {
+ const struct netvsc_channel *nvchan = &nvdev->chan_table[i];
+ const struct netvsc_stats *stats;
+ struct netvsc_ethtool_pcpu_stats *this_tot =
+ &pcpu_tot[nvchan->channel->target_cpu];
+ u64 packets, bytes;
+ unsigned int start;
+
+ stats = &nvchan->tx_stats;
+ do {
+ start = u64_stats_fetch_begin_irq(&stats->syncp);
+ packets = stats->packets;
+ bytes = stats->bytes;
+ } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+
+ this_tot->tx_bytes += bytes;
+ this_tot->tx_packets += packets;
+
+ stats = &nvchan->rx_stats;
+ do {
+ start = u64_stats_fetch_begin_irq(&stats->syncp);
+ packets = stats->packets;
+ bytes = stats->bytes;
+ } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+
+ this_tot->rx_bytes += bytes;
+ this_tot->rx_packets += packets;
+ }
+}
+
static void netvsc_get_stats64(struct net_device *net,
struct rtnl_link_stats64 *t)
{
@@ -1215,6 +1273,23 @@ static const struct {
{ "rx_no_memory", offsetof(struct netvsc_ethtool_stats, rx_no_memory) },
{ "stop_queue", offsetof(struct netvsc_ethtool_stats, stop_queue) },
{ "wake_queue", offsetof(struct netvsc_ethtool_stats, wake_queue) },
+}, pcpu_stats[] = {
+ { "cpu%u_rx_packets",
+ offsetof(struct netvsc_ethtool_pcpu_stats, rx_packets) },
+ { "cpu%u_rx_bytes",
+ offsetof(struct netvsc_ethtool_pcpu_stats, rx_bytes) },
+ { "cpu%u_tx_packets",
+ offsetof(struct netvsc_ethtool_pcpu_stats, tx_packets) },
+ { "cpu%u_tx_bytes",
+ offsetof(struct netvsc_ethtool_pcpu_stats, tx_bytes) },
+ { "cpu%u_vf_rx_packets",
+ offsetof(struct netvsc_ethtool_pcpu_stats, vf_rx_packets) },
+ { "cpu%u_vf_rx_bytes",
+ offsetof(struct netvsc_ethtool_pcpu_stats, vf_rx_bytes) },
+ { "cpu%u_vf_tx_packets",
+ offsetof(struct netvsc_ethtool_pcpu_stats, vf_tx_packets) },
+ { "cpu%u_vf_tx_bytes",
+ offsetof(struct netvsc_ethtool_pcpu_stats, vf_tx_bytes) },
}, vf_stats[] = {
{ "vf_rx_packets", offsetof(struct netvsc_vf_pcpu_stats, rx_packets) },
{ "vf_rx_bytes", offsetof(struct netvsc_vf_pcpu_stats, rx_bytes) },
@@ -1226,6 +1301,9 @@ static const struct {
#define NETVSC_GLOBAL_STATS_LEN ARRAY_SIZE(netvsc_stats)
#define NETVSC_VF_STATS_LEN ARRAY_SIZE(vf_stats)
+/* statistics per queue (rx/tx packets/bytes) */
+#define NETVSC_PCPU_STATS_LEN (num_present_cpus() * ARRAY_SIZE(pcpu_stats))
+
/* 4 statistics per queue (rx/tx packets/bytes) */
#define NETVSC_QUEUE_STATS_LEN(dev) ((dev)->num_chn * 4)
@@ -1241,6 +1319,7 @@ static int netvsc_get_sset_count(struct net_device *dev, int string_set)
case ETH_SS_STATS:
return NETVSC_GLOBAL_STATS_LEN
+ NETVSC_VF_STATS_LEN
+ + NETVSC_PCPU_STATS_LEN
+ NETVSC_QUEUE_STATS_LEN(nvdev);
default:
return -EINVAL;
@@ -1255,9 +1334,10 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
const void *nds = &ndc->eth_stats;
const struct netvsc_stats *qstats;
struct netvsc_vf_pcpu_stats sum;
+ struct netvsc_ethtool_pcpu_stats *pcpu_sum;
unsigned int start;
u64 packets, bytes;
- int i, j;
+ int i, j, cpu;
if (!nvdev)
return;
@@ -1269,6 +1349,18 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
for (j = 0; j < NETVSC_VF_STATS_LEN; j++)
data[i++] = *(u64 *)((void *)&sum + vf_stats[j].offset);
+ pcpu_sum = kvmalloc(sizeof(struct netvsc_ethtool_pcpu_stats) *
+ num_present_cpus(), GFP_KERNEL);
+ netvsc_get_pcpu_stats(dev, pcpu_sum);
+ for_each_present_cpu(cpu) {
+ struct netvsc_ethtool_pcpu_stats *this_sum = &pcpu_sum[cpu];
+
+ for (j = 0; j < ARRAY_SIZE(pcpu_stats); j++)
+ data[i++] = *(u64 *)((void *)this_sum
+ + pcpu_stats[j].offset);
+ }
+ kvfree(pcpu_sum);
+
for (j = 0; j < nvdev->num_chn; j++) {
qstats = &nvdev->chan_table[j].tx_stats;
@@ -1296,7 +1388,7 @@ static void netvsc_get_strings(struct net_device *dev, u32 stringset, u8 *data)
struct net_device_context *ndc = netdev_priv(dev);
struct netvsc_device *nvdev = rtnl_dereference(ndc->nvdev);
u8 *p = data;
- int i;
+ int i, cpu;
if (!nvdev)
return;
@@ -1313,6 +1405,13 @@ static void netvsc_get_strings(struct net_device *dev, u32 stringset, u8 *data)
p += ETH_GSTRING_LEN;
}
+ for_each_present_cpu(cpu) {
+ for (i = 0; i < ARRAY_SIZE(pcpu_stats); i++) {
+ sprintf(p, pcpu_stats[i].name, cpu);
+ p += ETH_GSTRING_LEN;
+ }
+ }
+
for (i = 0; i < nvdev->num_chn; i++) {
sprintf(p, "tx_queue_%u_packets", i);
p += ETH_GSTRING_LEN;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] net/p9/trans_fd.c: fix double list_del()
From: jiangyiwen @ 2018-07-24 1:40 UTC (permalink / raw)
To: Tomas Bortoli, ericvh, rminnich, lucho
Cc: asmadeus, davem, v9fs-developer, netdev, linux-kernel, syzkaller
In-Reply-To: <20180723121902.20201-1-tomasbortoli@gmail.com>
On 2018/7/23 20:19, Tomas Bortoli wrote:
> A double list_del(&req->req_list) is possible in p9_fd_cancel() as
> shown by Syzbot. To prevent it we have to ensure that we have the
> client->lock when deleting the list. Furthermore, we have to update
> the status of the request before releasing the lock, to prevent the
> race.
>
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+735d926e9d1317c3310c@syzkaller.appspotmail.com
> ---
> net/9p/trans_fd.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index a64b01c56e30..370c6c69a05c 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -199,15 +199,14 @@ static void p9_mux_poll_stop(struct p9_conn *m)
> static void p9_conn_cancel(struct p9_conn *m, int err)
> {
> struct p9_req_t *req, *rtmp;
> - unsigned long flags;
> LIST_HEAD(cancel_list);
>
> p9_debug(P9_DEBUG_ERROR, "mux %p err %d\n", m, err);
>
> - spin_lock_irqsave(&m->client->lock, flags);
> + spin_lock(&m->client->lock);
>
> if (m->err) {
> - spin_unlock_irqrestore(&m->client->lock, flags);
> + spin_unlock(&m->client->lock);
> return;
> }
>
> @@ -219,7 +218,6 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
> list_for_each_entry_safe(req, rtmp, &m->unsent_req_list, req_list) {
> list_move(&req->req_list, &cancel_list);
> }
> - spin_unlock_irqrestore(&m->client->lock, flags);
>
> list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) {
> p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req);
> @@ -228,6 +226,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
> req->t_err = err;
> p9_client_cb(m->client, req, REQ_STATUS_ERROR);
> }
> + spin_unlock(&m->client->lock);
If you want to expand the ranges of client->lock, the cancel_list will not
be necessary, you can optimize this code.
Thanks,
Yiwen.
> }
>
> static __poll_t
> @@ -370,12 +369,12 @@ static void p9_read_work(struct work_struct *work)
> if (m->req->status != REQ_STATUS_ERROR)
> status = REQ_STATUS_RCVD;
> list_del(&m->req->req_list);
> - spin_unlock(&m->client->lock);
> p9_client_cb(m->client, m->req, status);
> m->rc.sdata = NULL;
> m->rc.offset = 0;
> m->rc.capacity = 0;
> m->req = NULL;
> + spin_unlock(&m->client->lock);
> }
>
> end_clear:
>
^ permalink raw reply
* Re: [PATCH 4/7] x86/umwait_contro: Set global umwait maximum time limit and umwait C0.2 state
From: Andy Lutomirski @ 2018-07-24 1:41 UTC (permalink / raw)
To: Fenghua Yu, Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86
In-Reply-To: <1532350557-98388-5-git-send-email-fenghua.yu@intel.com>
On 07/23/2018 05:55 AM, Fenghua Yu wrote:
> UMWAIT or TPAUSE called by user process makes processor to reside in
> a light-weight power/performance optimized state (C0.1 state) or an
> improved power/performance optimized state (C0.2 state).
>
> IA32_UMWAIT_CONTROL MSR register allows OS to set global maximum umwait
> time and disable C0.2 on the processor.
>
> The maximum time value in IA32_UMWAIT_CONTROL[31-2] is set as zero which
> means there is no global time limit for UMWAIT and TPAUSE instructions.
> Each process sets its own umwait maximum time as the instructions operand.
> We don't set a non-zero global umwait maximum time value to enforce user
> wait timeout because we couldn't find any usage for it.
Do you know what the instruction designers had in mind? I assume they
were thinking of *something*, but I'm seriously mystified by three things:
- Why does CF work the way it does? It seems like it would be
genuinely useful for CF to indicate whether the in-register timeout has
expired, but that's not what CF does.
- Why does the global timeout apply even at CPL 0?
- Why does the C0.2 control apply at CPL 0?
And I'm also a bit surprised that the instruction can't be turned off
entirely for CPL 3.
^ permalink raw reply
* Re: [RFC][PATCH 0/5] Mount, Filesystem and Keyrings notifications
From: Ian Kent @ 2018-07-24 0:37 UTC (permalink / raw)
To: Casey Schaufler, David Howells, viro
Cc: linux-fsdevel, linux-kernel, keyrings, linux-security-module
In-Reply-To: <675e5c24-36ef-4cc5-846c-1414c1195d85@schaufler-ca.com>
On Mon, 2018-07-23 at 09:31 -0700, Casey Schaufler wrote:
> On 7/23/2018 8:25 AM, David Howells wrote:
> > Hi Al,
> >
> > Here's a set of patches to add a general variable-length notification queue
> > concept and to add sources of events for:
>
> Overall I approve. The interface is a bit clunky. Some concerns below.
>
> >
> > (1) Mount topology and reconfiguration change events.
>
> With the possibility of unprivileged mounting you're
> going to have to address access control on events.
> If root in a user namespace mounts a filesystem you
> may have a case where the "real" user wouldn't want the
> listener to receive a notification.
>
> > (2) Superblocks EIO, ENOSPC and EDQUOT events (not complete yet).
>
> Here, too. If SELinux (for example) policy says you can't see
> anything on a filesystem you shouldn't get notifications about
> things that happen to that filesystem.
>
> > (3) Key/keyring changes events
>
> And again, I should only get notifications about keys and
> keyrings I have access to.
>
> I expect that you intentionally left off
>
> (4) User injected events
>
> at this point, but it's an obvious extension. That is going
> to require access controls (remember kdbus) so I think you'd
> do well to design them in now rather than have some security
> module hack like me come along later and "fix" it.
I thought mount name space should be considered too even
though I wasn't considering the cloning of file handles
into a user mount name space.
But can this happen in other ways besides user mount name
space creation (I'm fishing here)?
And nsenter(1) doesn't require an exec for anything other
than a pid name space change so a forced close on exec
wouldn't be enough. Or am I mistaken in that nsenter(1)
actually requires running a program (even though the man
page implies it's optional) ...
Are there other consideration my limited understanding is
missing?
>
> > One of the reasons for this is so that we can remove the issue of processes
> > having to repeatedly and regularly scan /proc/mounts, which has proven to be
> > a
> > system performance problem.
> >
> >
> > Design decisions:
> >
> > (1) A misc chardev is used to create and open a ring buffer:
> >
> > fd = open("/dev/watch_queue", O_RDWR);
> >
> > which is then configured and mmap'd into userspace:
> >
> > ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE);
> > ioctl(fd, IOC_WATCH_QUEUE_SET_FILTER, &filter);
> > buf = mmap(NULL, BUF_SIZE * page_size, PROT_READ | PROT_WRITE,
> > MAP_SHARED, fd, 0);
> >
> > The fd cannot be read or written (though there is a facility to use
> > write
> > to inject records for debugging) and userspace just pulls data directly
> > out of the buffer.
> >
> > (2) The ring index pointers are stored inside the ring and are thus
> > accessible to userspace. Userspace should only update the tail pointer
> > and never the head pointer or risk breaking the buffer. The kernel
> > checks that the pointers appear valid before trying to use them. A
> > 'skip' record is maintained around the pointers.
> >
> > (3) poll() can be used to wait for data to appear in the buffer.
> >
> > (4) Records in the buffer are binary, typed and have a length so that they
> > can be of varying size.
> >
> > This means that multiple heterogeneous sources can share a common
> > buffer. Tags may be specified when a watchpoint is created to help
> > distinguish the sources.
> >
> > (5) The queue is reusable as there are 16 million types available, of which
> > I've used 4, so there is scope for others to be used.
> >
> > (6) Records are filterable as types have up to 256 subtypes that can be
> > individually filtered. Other filtration is also available.
> >
> > (7) Each time the buffer is opened, a new buffer is created - this means
> > that
> > there's no interference between watchers.
> >
> > (8) When recording a notification, the kernel will not sleep, but will
> > rather
> > mark a queue as overrun if there's insufficient space, thereby avoiding
> > userspace causing the kernel to hang.
> >
> > (9) The 'watchpoint' should be specific where possible, meaning that you
> > specify the object that you want to watch.
> >
> > (10) The buffer is created and then watchpoints are attached to it, using
> > one
> > of:
> >
> > keyctl_watch_key(KEY_SPEC_SESSION_KEYRING, fd, 0x01);
> > mount_notify(AT_FDCWD, "/", 0, fd, 0x02);
> > sb_notify(AT_FDCWD, "/mnt", 0, fd, 0x03);
> >
> > where in all three cases, fd indicates the queue and the number after
> > is
> > a tag between 0 and 255.
> >
> > (11) The watch must be removed if either the watch buffer is destroyed or
> > the
> > watched object is destroyed.
> >
> >
> > Things I want to avoid:
> >
> > (1) Introducing features that make the core VFS dependent on the network
> > stack or networking namespaces (ie. usage of netlink).
> >
> > (2) Dumping all this stuff into dmesg and having a daemon that sits there
> > parsing the output and distributing it as this then puts the
> > responsibility for security into userspace and makes handling
> > namespaces
> > tricky. Further, dmesg might not exist or might be inaccessible inside
> > a
> > container.
> >
> > (3) Letting users see events they shouldn't be able to see.
> >
> >
> > Further things that need to be done:
> >
> > (1) fsinfo() syscall needs to find superblocks by ID as well as by path so
> > that it can query a superblock for information without the need to try
> > and work out how to reach it - if the calling process even can.
> >
> > (2) A mount_info() syscall is needed that can enumerate all the children of
> > a
> > mount. This is necessary because mountpoints can hide each other by
> > stacking, so paths are not unique keys. This will require the ability
> > to
> > look up a mount by ID. This avoids the need to parse /proc/mounts.
> >
> > (3) A keyctl call is needed to allow a watch on a keyring to be extended to
> > "children" of that keyring, such that the watch is removed from the
> > child
> > if it is unlinked from the keyring.
> >
> > (4) A global superblock event queue maybe?
> >
> > (5) Propagating watches to child superblock over automounts?
> >
> >
> > The patches can be found here also:
> >
> > http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h
> > =notifications
> >
> > David
> > ---
> > David Howells (5):
> > General notification queue with user mmap()'able ring buffer
> > KEYS: Add a notification facility
> > vfs: Add a mount-notification facility
> > vfs: Add superblock notifications
> > Add sample notification program
> >
> >
> > Documentation/security/keys/core.rst | 59 ++
> > Documentation/watch_queue.rst | 305 ++++++++++++
> > arch/x86/entry/syscalls/syscall_32.tbl | 2
> > arch/x86/entry/syscalls/syscall_64.tbl | 2
> > drivers/misc/Kconfig | 9
> > drivers/misc/Makefile | 1
> > drivers/misc/watch_queue.c | 835
> > ++++++++++++++++++++++++++++++++
> > fs/Kconfig | 21 +
> > fs/Makefile | 1
> > fs/fs_context.c | 1
> > fs/mount.h | 26 +
> > fs/mount_notify.c | 178 +++++++
> > fs/namespace.c | 18 +
> > fs/super.c | 116 ++++
> > include/linux/dcache.h | 1
> > include/linux/fs.h | 77 +++
> > include/linux/key.h | 4
> > include/linux/syscalls.h | 4
> > include/linux/watch_queue.h | 87 +++
> > include/uapi/linux/keyctl.h | 1
> > include/uapi/linux/watch_queue.h | 156 ++++++
> > kernel/sys_ni.c | 6
> > mm/interval_tree.c | 2
> > mm/memory.c | 1
> > samples/Kconfig | 6
> > samples/Makefile | 2
> > samples/watch_queue/Makefile | 9
> > samples/watch_queue/watch_test.c | 232 +++++++++
> > security/keys/Kconfig | 10
> > security/keys/compat.c | 3
> > security/keys/gc.c | 5
> > security/keys/internal.h | 29 +
> > security/keys/key.c | 37 +
> > security/keys/keyctl.c | 90 +++
> > security/keys/keyring.c | 17 -
> > security/keys/request_key.c | 4
> > 36 files changed, 2332 insertions(+), 25 deletions(-)
> > create mode 100644 Documentation/watch_queue.rst
> > create mode 100644 drivers/misc/watch_queue.c
> > create mode 100644 fs/mount_notify.c
> > create mode 100644 include/linux/watch_queue.h
> > create mode 100644 include/uapi/linux/watch_queue.h
> > create mode 100644 samples/watch_queue/Makefile
> > create mode 100644 samples/watch_queue/watch_test.c
> >
> >
>
>
^ permalink raw reply
* RE: [PATCH v3] hv_netvsc: Add per-cpu ethtool stats for netvsc
From: Yidong Ren @ 2018-07-24 1:42 UTC (permalink / raw)
To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, David S. Miller,
devel@linuxdriverproject.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: Madhan Sivakumar
In-Reply-To: <20180724012622.26873-1-yidren@linuxonhyperv.com>
> 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.
^ permalink raw reply
* Re: [PATCH] block: ioprio: Replace GFP_ATOMIC with GFP_KERNEL in set_task_ioprio()
From: Jia-Ju Bai @ 2018-07-24 1:42 UTC (permalink / raw)
To: Tetsuo Handa, Christoph Hellwig; +Cc: axboe, linux-block, linux-kernel
In-Reply-To: <8dab79c6-eb75-b013-1f83-3af43a7052cb@I-love.SAKURA.ne.jp>
On 2018/7/24 5:49, Tetsuo Handa wrote:
> On 2018/07/23 16:18, Christoph Hellwig wrote:
>> Looks good,
> Looks bad. :-(
>
> SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
> {
> (...snipped...)
> rcu_read_lock();
> (...snipped...)
> ret = set_task_ioprio(p, ioprio);
> (...snipped...)
> rcu_read_unlock();
> (...snipped...)
Oops, my tool does not handle SYSCALL_DEFINE3()...
Sorry for my incorrect report.
Best wishes,
Jia-Ju Bai
^ permalink raw reply
* Re: [PATCH v2] perf/core: fix a possible deadlock scenario
From: Cong Wang @ 2018-07-24 1:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: LKML, Ingo Molnar, Linus Torvalds, Arnaldo Carvalho de Melo,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Andi Kleen
In-Reply-To: <CAM_iQpV33dj-_bwYp3QJdPKihR8HyPzKMOpi1Lrqm7VtN3bZ8w@mail.gmail.com>
On Mon, Jul 23, 2018 at 6:35 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Hi, Peter, Andi
>
> While reviewing the deadlock, I find out it looks like we could have the
> following infinite recursion too:
>
> perf_event_account_interrupt()
> __perf_event_account_interrupt()
> perf_adjust_period()
> event->pmu->stop
> x86_pmu_stop()
> x86_pmu.disable()
Hmm, x86_pmu_stop() calls __test_and_clear_bit(), so
we should not call x86_pmu.disable() twice here.
> intel_pmu_disable_event()
> intel_pmu_pebs_disable()
> intel_pmu_drain_pebs_buffer()
> intel_pmu_drain_pebs_nhm()
> <repeat....>
>
> This time is pure hardware events, attr.freq must be non-zero.
>
> And, we could enter this infinite recursion in NMI handler too:
>
> intel_pmu_handle_irq()
> perf_event_overflow()
> __perf_event_overflow()
> __perf_event_account_interrupt()
> ....
>
> Or this is impossible too?
>
> Thanks!
^ permalink raw reply
* [virtio-dev] Re: [PATCH v36 2/5] virtio_balloon: replace oom notifier with shrinker
From: Wei Wang @ 2018-07-24 1:49 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, linux-kernel, virtualization, kvm, linux-mm, mhocko,
akpm, torvalds, pbonzini, liliang.opensource, yang.zhang.wz,
quan.xu0, nilal, riel, peterx
In-Reply-To: <20180723170826-mutt-send-email-mst@kernel.org>
On 07/23/2018 10:13 PM, Michael S. Tsirkin wrote:
>>> vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops;
>>> #endif
>>> + err = virtio_balloon_register_shrinker(vb);
>>> + if (err)
>>> + goto out_del_vqs;
>>> So we can get scans before device is ready. Leak will fail
>>> then. Why not register later after device is ready?
>> Probably no.
>>
>> - it would be better not to set device ready when register_shrinker failed.
> That's very rare so I won't be too worried.
Just a little confused with the point here. "very rare" means it still
could happen (even it's a corner case), and if that happens, we got
something wrong functionally. So it will be a bug if we change like
that, right?
Still couldn't understand the reason of changing shrinker_register after
device_ready (the original oom notifier was registered before setting
device ready too)?
(I think the driver won't get shrinker_scan called if device isn't ready
because of the reasons below)
>> - When the device isn't ready, ballooning won't happen, that is,
>> vb->num_pages will be 0, which results in shrinker_count=0 and shrinker_scan
>> won't be called.
Best,
Wei
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply
* [U-Boot] [PATCH 11/17] fs: add mkdir interface
From: AKASHI Takahiro @ 2018-07-24 1:45 UTC (permalink / raw)
To: u-boot
In-Reply-To: <20180723235658.GC11755@bill-the-cat>
On Mon, Jul 23, 2018 at 07:56:58PM -0400, Tom Rini wrote:
> On Mon, Jul 23, 2018 at 05:48:13PM -0600, Simon Glass wrote:
> > Hi,
> >
> > On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > >> "mkdir" interface is added to file operations.
> > >> This is a preparatory change as mkdir support for FAT file system
> > >> will be added in next patch.
> > >>
> > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >> ---
> > >> fs/fs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >> include/fs.h | 10 ++++++++++
> > >> 2 files changed, 55 insertions(+)
> > >>
> >
> > We need to get a proper fs test in place before we add any more stuff.
>
> Agreed that we need more tests as part of this series.
Is this a new standard rule for new features?
Just kidding, but testing was definitely one of my concerns partly
because fs-test.sh is anyhow in an old fashion and partly because
I can't find any criteria for test coverage:
- white-box test or black-box test
- coverage for corner (or edge) cases
- some sort of stress test, etc.
> > Does someone waent to try converting fs-test.sh to pytest in test/py/tests ?
>
> I thought there was at least a first pass at that?
Well, I don't see any file system related scripts under test/py/tests.
Please advise me.
Thanks,
-Takahiro AKASHI
> --
> Tom
^ permalink raw reply
* Re: [RFC v2 1/4] mac80211: Add TXQ scheduling API
From: Rajkumar Manoharan @ 2018-07-24 0:42 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: linux-wireless, make-wifi-fast, Felix Fietkau,
linux-wireless-owner
In-Reply-To: <877elo48ea.fsf@toke.dk>
On 2018-07-21 13:54, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>
>> On 2018-07-19 07:18, Toke Høiland-Jørgensen wrote:
>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>>
>>>> On 2018-07-13 06:39, Toke Høiland-Jørgensen wrote:
>>>>> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
>>
>>>>> It is not generally predictable how many times this will loop
>>>>> before
>>>>> exiting...
>>>>>
>>>> Agree.. It would be better If the driver does not worry about txq
>>>> sequence numbering. Perhaps one more API (ieee80211_first_txq) could
>>>> solve this. Will leave it to you.
>>>
>>> That is basically what the second parameter to next_txq() does in the
>>> current patchset. It just needs to be renamed...
>>>
>> Agree. As next_txq() increments seqno while starting loop for each AC,
>> It seems bit confusing. i.e A single ath_txq_schedule_all call will
>> bump schedule_seqno by 4. right?
>
> Hmm, good point. Guess there should be one seqno per ac...
>
I would prefer to keep separate list for each AC ;) Prepared one on top
of
your earlier merged changes. Now it needs revisit.
>> Let assume below case where CPU1 is dequeuing skb from isr context and
>> CPU2 is enqueuing skbs into same txq.
>>
>> CPU1 CPU2
>> ---- ----
>> ath_txq_schedule
>> -> ieee80211_next_txq(first)
>> -> inc_seq
>> -> delete from list
>> -> txq->seqno = local->seqno
>> ieee80211_tx/fast_xmit
>> ->
>> ieee80211_queue_skb
>> ->
>> ieee80211_schedule_txq(reset_seqno)
>> -> list_add
>> ->
>> txqi->seqno
>> = local->seqno - 1
>>
>> In above sequence, it is quite possible that the same txq will be
>> served repeatedly and other txqs will be starved. am I right? IMHO
>> seqno mechanism will not guarantee that txqs will be processed only
>> once in an iteration.
>
> Yeah, ieee80211_queue_skb() shouldn't set reset_seqno; was
> experimenting
> with that, and guess I ended up picking the wrong value. Thanks for
> pointing that out :)
>
A simple change in argument may break algo. What would be seqno of first
packet of txq if queue_skb() isn't reset seqno? I am fine in passing
start
of loop as arg to next_ntx(). But prefer to keep loop detecting within
mac80211 itself by tracking txq same as ath10k. So that no change is
needed
in schedule_txq() arg list. thoughts?
>>>>> Well, it could conceivably be done in a way that doesn't require
>>>>> taking
>>>>> the activeq_lock. Adding another STOP flag to the TXQ, for
>>>>> instance.
>>>>> From an API point of view I think that is more consistent with what
>>>>> we
>>>>> have already...
>>>>>
>>>>
>>>> Make sense. ieee80211_txq_may_pull would be better place to decide
>>>> whether given txq is allowed for transmission. It also makes drivers
>>>> do not have to worry about deficit. Still I may need
>>>> ieee80211_reorder_txq API after processing txq. isn't it?
>>>
>>> The way I was assuming this would work (and what ath9k does), is that
>>> a
>>> driver only operates on one TXQ at a time; so it can get that txq,
>>> process it, and re-schedule it. In which case no other API is needed;
>>> the rotating can be done in next_txq(), and schedule_txq() can insert
>>> the txq back into the rotation as needed.
>>>
>>> However, it sounds like this is not how ath10k does things? See
>>> below.
>>>
>> correct. The current rotation works only in push-only mode. i.e when
>> firmware is not deciding txqs and the driver picks priority txq from
>> active_txqs list. Unfortunately rotation won't work when the driver
>> selects txq other than first in the list. In any case separate API
>> needed for rotation when the driver is processing few packets from txq
>> instead of all pending frames.
>
> Rotation is not dependent on the TXQ draining completely. Dequeueing a
> few packets, then rescheduling the TXQ is fine.
>
The problem is that when the driver accesses txq directly, it wont call
next_txq(). So the txq will not dequeued from list and schedule_txq()
wont
be effective. right?
ieee80211_txq_may_pull() - check whether txq is allowed for tx_dequeue()
that helps to keep deficit check in mac80211
itself
ieee80211_reorder_txq() - after dequeuing skb (in direct txq access),
txq will be deleted from list and if there are
pending
frames, it will be requeued.
>>> And if so, how does that interact with ath10k_mac_tx_push_pending()?
>>> That function is basically doing the same thing that I explained
>>> above
>>> for ath9k; in the previous version of this patch series I modified
>>> that
>>> to use next_txq(). But is that a different TX path, or am I
>>> misunderstanding you?
>>>
>>> If you could point me to which parts of the ath10k code I should be
>>> looking at, that would be helpful as well :)
>>>
>> Depends on firmware htt_tx_mode (push/push_pull),
>> ath10k_mac_tx_push_pending() downloads all/partial frames to firmware.
>> Please take a look at ath10k_mac_tx_can_push() in push_pending(). Let
>> me know If you need any other details.
>
> Right, so looking at this, it looks like the driver will loop through
> all the available TXQs, trying to dequeue from each of them if
> ath10k_mac_tx_can_push() returns true, right? This should actually work
> fine with the next_txq() / schedule_txq() API. Without airtime fairness
> that will just translate into basically what the driver is doing now,
> and I don't think more API functions are needed, as long as that is the
> only point in the driver that pushes packets to the device...
>
In push-only mode, this will work with next_txq() / schedule_txq() APIs.
In ath10k, packets are downloaded to firmware in three places.
wake_tx_queue()
tx_push_pending()
htt_rx_tx_fetch_ind() - do txq_lookup() and push_txq()
the above mentioned new APIs are needed to take care of fetch_ind().
> With airtime fairness, what is going to happen is that the loop is only
> going to get a single TXQ (the first one with a positive deficit), then
> exit. Once that station has transmitted something and its deficit runs
> negative, it'll get rotated and the next one will get a chance. So I
> expect that airtime fairness will probably work, but MU-MIMO will
> break...
>
Agree.. In your earlier series, you did similar changes in ath10k
especially
in wake_tx_queue and push_pending().
> Don't think we can do MU-MIMO with a DRR airtime scheduler; we'll have
> to replace it with something different. But I think the same next_txq()
> / schedule_txq() API will work for that as well...
>
As mentioned above, have to take of fetch_ind(). All 10.4 based chips
(QCA99x0,
QCA9984, QCA9888, QCA4019, etc.) operates in push-pull mode, when the
number of
connected station increased. As target does not have enough memory for
buffering
frames for each station, it relied on host memory. ath10k driver can not
identify
that whether the received fetch_ind is for MU-MIMO or regular
transmission.
If we don't handle this case, then ath10k driver can not claim mac80211
ATF support.
Agree that MU-MIMO won't work with DDR scheduler. and it will impact
MU-MIMO performace
in ath10k when mac80211 ATF is claimed by ath10k.
-Rajkumar
^ permalink raw reply
* Re: [PATCH 5/7] x86/vdso: Add vDSO functions for direct store instructions
From: Andy Lutomirski @ 2018-07-24 1:48 UTC (permalink / raw)
To: Fenghua Yu, Thomas Gleixner, Ingo Molnar, H Peter Anvin
Cc: Ashok Raj, Alan Cox, Ravi V Shankar, linux-kernel, x86
In-Reply-To: <1532350557-98388-6-git-send-email-fenghua.yu@intel.com>
On 07/23/2018 05:55 AM, Fenghua Yu wrote:
> User wants to query if direct store instructions are supported and use
> the instructions. The vDSO functions provides fast interface for user
> to query the support and use the instructions.
>
> movdiri_supported and its alias __vdso_movdiri_supported check if
> movdiri instructions are supported.
>
> movdir64b_supported and its alias __vdso_movdir64b_supported checks
> if movdir64b instruction is supported.
>
> movdiri32 and its alias __vdso_movdiri32 provide user APIs for calling
> 32-bit movdiri instruction.
>
> movdiri64 and its alias __vdso_movdiri64 provide user APIs for calling
> 64-bit movdiri instruction.
>
> movdir64b and its alias __vdso_movdir64b provide user APIs to move
> 64-byte data through movdir64b instruction.
>
> The instructions can be implemented in intrinsic functions in future
> GCC. But the vDSO interfaces are available to user without the
> intrinsic functions support in GCC and the APIs movdiri_supported and
> movdir64b_supported cannot be implemented as GCC functions.
I'm not convinced that any of this belongs in the vDSO at all. You
could just add AT_HWCAP (or AT_HWCAP2) flags for the new instructions.
Or user code could use CPUID just like for any other new instruction.
But, if there really is some compelling reason to add this to the vDSO,
then see below:
+
> +notrace bool __vdso_movdiri_supported(void)
> +{
> + return _vdso_funcs_data->movdiri_supported;
return static_cpu_has(X86_FEATURE_MOVDIRI);
And all the VVAR stuff can be removed.
^ permalink raw reply
* Re: [PATCH v36 2/5] virtio_balloon: replace oom notifier with shrinker
From: Wei Wang @ 2018-07-24 1:49 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: yang.zhang.wz, virtio-dev, riel, quan.xu0, kvm, nilal,
liliang.opensource, linux-kernel, mhocko, linux-mm, pbonzini,
akpm, virtualization, torvalds
In-Reply-To: <20180723170826-mutt-send-email-mst@kernel.org>
On 07/23/2018 10:13 PM, Michael S. Tsirkin wrote:
>>> vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops;
>>> #endif
>>> + err = virtio_balloon_register_shrinker(vb);
>>> + if (err)
>>> + goto out_del_vqs;
>>> So we can get scans before device is ready. Leak will fail
>>> then. Why not register later after device is ready?
>> Probably no.
>>
>> - it would be better not to set device ready when register_shrinker failed.
> That's very rare so I won't be too worried.
Just a little confused with the point here. "very rare" means it still
could happen (even it's a corner case), and if that happens, we got
something wrong functionally. So it will be a bug if we change like
that, right?
Still couldn't understand the reason of changing shrinker_register after
device_ready (the original oom notifier was registered before setting
device ready too)?
(I think the driver won't get shrinker_scan called if device isn't ready
because of the reasons below)
>> - When the device isn't ready, ballooning won't happen, that is,
>> vb->num_pages will be 0, which results in shrinker_count=0 and shrinker_scan
>> won't be called.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v36 2/5] virtio_balloon: replace oom notifier with shrinker
From: Wei Wang @ 2018-07-24 1:49 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, linux-kernel, virtualization, kvm, linux-mm, mhocko,
akpm, torvalds, pbonzini, liliang.opensource, yang.zhang.wz,
quan.xu0, nilal, riel, peterx
In-Reply-To: <20180723170826-mutt-send-email-mst@kernel.org>
On 07/23/2018 10:13 PM, Michael S. Tsirkin wrote:
>>> vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops;
>>> #endif
>>> + err = virtio_balloon_register_shrinker(vb);
>>> + if (err)
>>> + goto out_del_vqs;
>>> So we can get scans before device is ready. Leak will fail
>>> then. Why not register later after device is ready?
>> Probably no.
>>
>> - it would be better not to set device ready when register_shrinker failed.
> That's very rare so I won't be too worried.
Just a little confused with the point here. "very rare" means it still
could happen (even it's a corner case), and if that happens, we got
something wrong functionally. So it will be a bug if we change like
that, right?
Still couldn't understand the reason of changing shrinker_register after
device_ready (the original oom notifier was registered before setting
device ready too)?
(I think the driver won't get shrinker_scan called if device isn't ready
because of the reasons below)
>> - When the device isn't ready, ballooning won't happen, that is,
>> vb->num_pages will be 0, which results in shrinker_count=0 and shrinker_scan
>> won't be called.
Best,
Wei
^ permalink raw reply
* Re: m68k allmodconfig build errors
From: Randy Dunlap @ 2018-07-24 1:49 UTC (permalink / raw)
To: Finn Thain; +Cc: LKML, Geert Uytterhoeven, linux-m68k
In-Reply-To: <alpine.LNX.2.21.1807201454590.8@nippy.intranet>
On 07/19/2018 10:17 PM, Finn Thain wrote:
> On Thu, 19 Jul 2018, Randy Dunlap wrote:
>
>> Hi Geert,
>>
>> I am seeing a few errors when cross-building m68k on x86_64, using the
>> toolchain at https://mirrors.edge.kernel.org/pub/tools/crosstool/
>> (thanks, Arnd). (so this is gcc 8.1.0)
>>
>> block/partitions/ldm.o: In function `ldm_partition':
>> ldm.c:(.text+0x1900): undefined reference to `strcmp'
>> ldm.c:(.text+0x1964): undefined reference to `strcmp'
>> drivers/rtc/rtc-proc.o: In function `is_rtc_hctosys':
>> rtc-proc.c:(.text+0x290): undefined reference to `strcmp'
>> drivers/watchdog/watchdog_pretimeout.o: In function `watchdog_register_governor':
>> (.text+0x142): undefined reference to `strcmp'
>>
>>
>> Adding #include <linux/string.h> does not help.
>>
>> Is this a toolchain problem or drivers or something else?
>>
>
> This gcc build was apparently configured like so:
>
> /home/arnd/git/gcc/configure --target=m68k-linux --enable-targets=all
> --prefix=/home/arnd/cross/x86_64/gcc-8.1.0-nolibc/m68k-linux
> --enable-languages=c --without-headers --disable-bootstrap --disable-nls
> --disable-threads --disable-shared --disable-libmudflap --disable-libssp
> --disable-libgomp --disable-decimal-float --disable-libquadmath
> --disable-libatomic --disable-libcc1 --disable-libmpx
> --enable-checking=release
>
> In my own cross toolchain builds strcmp comes from glibc but this
> toolchain has no libc at all.
>
>> help?
>>
>
> Linux will use the strcmp in lib/string.c unless __HAVE_ARCH_STRCMP is
> defined in the arch headers. Grep suggests that m68k, mips, x86, xtensa,
> arc, sh, arm64, s390 all define that macro. But maybe you could just patch
> out that definition for build testing.
Sure, that works. Thanks.
--
~Randy
^ permalink raw reply
* Re: refs/notes/amlog problems, was Re: [PATCH v3 01/20] linear-assignment: a function to solve least-cost assignment problems
From: Junio C Hamano @ 2018-07-24 1:50 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <20180723012552.GA26886@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> If I understand the situation correctly, Junio is saying that he will
> continue to produce the amlog mapping, and that it contains sufficient
> information to produce the reverse mapping (which, as an aside, I did
> not even know existed -- I mostly want to go the other way, from digging
> in history to a mailing list conversation).
Yes, the reverse mapping in amlog was an experiment that did not
work well in the end.
When I use "git am" to make a commit out of a message, a
post-applypatch hook picks up the "Message-Id:" from the original
message and adds a git note to the resulting commit. This is in
line with how the notes are meant to be used. We have a commit
object, and a piece of information that we want to associate with
the commit object, which is not recorded as a part of the commit
object. So we say "git notes add -m 'that piece of info' $commit"
(the message-id happens to be that piece of info in this example).
And with notes.rewriteRef, "git commit --amend" etc. would copy the
piece of info about the original commit to the rewritten commit.
Side Note: there are a few workflow elements I do want to
keep using but they currently *lose* the mapping info. An
obvious one is
$ git checkout -b to/pic master &&
... review in MUA and then ...
$ git am -s mbox &&
... review in tree, attempt to build, tweak, etc.
$ git format-patch --stdout master..to/pic >P &&
$ edit P &&
$ git reset --hard master &&
$ git am P
which is far more versatile and efficient when doing certain
transformations on the series than running "rebase -i" and
reopening and editing the target files of the patches one by
one in each step. But because format-patch does not
generate Message-Id header of the original one out of the
commit, the post-applypatch hook run by "am" at the end of
the steps would not have a chance to record that for the
newly created commit.
For this one, I think I can use "format-patch --notes=amlog"
to produce the patch file and then teach post-applypatch
script to pay attention to the Notes annotation without
changing anything else to record the message id of the
original. Other workflow elements that lose the notes need
to be identified and either a fix implemented or a
workaround found for each of them. For example, I suspect
there is no workaround for "cherry-pick" and it would take a
real fix.
A reverse mapping entry used to get created by post-applypatch to
map the blob that represents the notes text added to the $commit to
another text blob that contains the 40-hex of the commit object.
This is the experiment that did not work well. As none of the later
integrator's work e.g. "commit --amend", "rebase", "cherry-pick",
etc. is about rewriting that blob, notes.rewriteRef mechanism would
not kick in, and that is understandasble.
And these (incomplete) reverse mapping entries get in the way to
maintain and correct the forward mapping. When a commit that got
unreachable gets expired, I want "git notes prune" to remove notes
on them, and I do not want to even think about what should happen to
the entries in the notes tree that abuse the mechanism to map blobs
that are otherwise *not* even reachable from the main history.
A much more important task is to make sure that the forward mapping
that annotates invidual commits reachable from 'pu' and/or 'master'
is maintained correctly by various tools. From a correctly maintained
forward mapping, it should be straight forward to get a reverse mapping
if needed.
> Though personally, I do not know if there is much point in pushing it
> out, given that receivers can reverse the mapping themselves.
Before this thread, I was planning to construct and publish the
reverse mapping at the end of the day, but do so on a separate notes
ref (see above---the hacky abuse gets in the way of maintaining and
debugging the forward mapping, but a separate notes-ref that only
contains hacks is less worrysome). But I have changed my mind and
decided not to generate or publish one. It is sort of similar to
the way the pack .idx is constructed only by the receiver [*1*].
> Or is there some argument that there is information in the reverse map
> that _cannot_ be generated from the forward map?
I know there is no information loss (after all I was the only one
who ran that experimental hack), but there is one objection that is
still possible, even though I admit that is a weak argument.
If a plumbing "diff-{files,tree,index}" family had a sibling
"diff-notes" to compare two notes-shaped trees while pretending that
the object-name fan-out did not exist (i.e. instead, the trees being
compared is without a subtree and full of 40-hex filenames), then it
would be less cumbersome to incrementally update the reverse mapping
by reading forward mapping with something like:
git diff-notes --raw amlog@{1} amlog
to learn the commits whose notes have changed. But without such a
plumbing, it is cumbersome to do so correctly. "git diff-tree -r"
could serve as a rough substitute, until the note tree grows and get
rebalanced by reorganizing the fan-out, and on the day it happens
the reverse mapper needs to read and discard ghost changes that are
only due to tree reorganizing [*2*].
[Footnotes]
*1* Even if the sender could give one when it creates a .pack, the
receiver would not trust that it is matches the corresponding
.pack before using it, and the cost to validate is similar to
the cost to generate.
*2* That makes it less efficient on that day (which hopefully would
happen once in a blue moon) but would not affect correctness.
^ permalink raw reply
* Re: [PATCHv3 2/2] mtd: m25p80: restore the status of SPI flash when exiting
From: NeilBrown @ 2018-07-24 1:51 UTC (permalink / raw)
To: Boris Brezillon
Cc: Brian Norris, Zhiqiang Hou, linux-mtd, Linux Kernel,
David Woodhouse, Boris BREZILLON, Marek Vasut, Richard Weinberger,
Cyrille Pitchen
In-Reply-To: <20180724012226.307f578d@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 8719 bytes --]
On Tue, Jul 24 2018, Boris Brezillon wrote:
> On Tue, 24 Jul 2018 08:46:33 +1000
> NeilBrown <neilb@suse.com> wrote:
>
>> On Mon, Jul 23 2018, Brian Norris wrote:
>>
>> > Hi Boris,
>> >
>> > On Mon, Jul 23, 2018 at 1:10 PM, Boris Brezillon
>> > <boris.brezillon@bootlin.com> wrote:
>> >> On Mon, 23 Jul 2018 11:13:50 -0700
>> >> Brian Norris <computersforpeace@gmail.com> wrote:
>> >>> I noticed this got merged, but I wanted to put my 2 cents in here:
>> >>
>> >> I wish you had replied to this thread when it was posted (more than
>> >> 6 months ago). Reverting the patch now implies making some people
>> >> unhappy because they'll have to resort to their old out-of-tree
>> >> hacks :-(.
>> >
>> > I'd say I'm sorry for not following things closely these days, but I'm
>> > not really that sorry. There are plenty of other capable hands. And if
>> > y'all shoot yourselves in the foot, so be it. This patch isn't going
>> > to blow things up, but now that I did finally notice it (because it
>> > happened to show up in a list of backports I was looking at), I
>> > thought better late than never to remind you.
>> >
>> > For way of notification: Marek already noticed that we've started down
>> > a slippery slope months ago:
>> >
>> > https://lkml.org/lkml/2018/4/8/141
>> > Re: [PATCH] mtd: spi-nor: clear Extended Address Reg on switch to
>> > 3-byte addressing.
>> >
>> > I'm not quite sure why that wasn't taken to its logical conclusion --
>> > that the hack should be reverted.
>> >
>> > This problem has been noted many times already, and we've always
>> > stayed on the side of *avoiding* this hack. A few references from a
>> > search of my email:
>> >
>> > http://lists.infradead.org/pipermail/linux-mtd/2013-March/046343.html
>> > [PATCH 1/3] mtd: m25p80: utilize dedicated 4-byte addressing commands
>> >
>> > http://lists.infradead.org/pipermail/barebox/2014-September/020682.html
>> > [RFC] MTD m25p80 3-byte addressing and boot problem
>> >
>> > http://lists.infradead.org/pipermail/linux-mtd/2015-February/057683.html
>> > [PATCH 2/2] m25p80: if supported put chip to deep power down if not used
>> >
>> >>> On Wed, Dec 06, 2017 at 10:53:42AM +0800, Zhiqiang Hou wrote:
>> >>> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>> >>> >
>> >>> > Restore the status to be compatible with legacy devices.
>> >>> > Take Freescale eSPI boot for example, it copies (in 3 Byte
>> >>> > addressing mode) the RCW and bootloader images from SPI flash
>> >>> > without firing a reset signal previously, so the reboot command
>> >>> > will fail without reseting the addressing mode of SPI flash.
>> >>> > This patch implement .shutdown function to restore the status
>> >>> > in reboot process, and add the same operation to the .remove
>> >>> > function.
>> >>>
>> >>> We have previously rejected this patch multiple times, because the above
>> >>> comment demonstrates a broken product.
>> >>
>> >> If we were to only support working HW parts, I fear Linux would not
>> >> support a lot of HW (that's even more true when it comes to flashes :P).
>> >
>> > You stopped allowing UBI to attach to MLC NAND recently, no? That
>> > sounds like almost the same boat -- you've probably killed quite a few
>> > shitty products, if they were to use mainline directly.
>> >
>> > Anyway, that's derailing the issue. Supporting broken hardware isn't
>> > something you try to do by applying the same hack to all systems. You
>> > normally try to apply your hack as narrowly as possible. You seem to
>> > imply that below. So maybe that's a solution to move forward with. But
>> > I'd personally be just as happy to see the patch reverted.
>> >
>> >>> You cannot guarantee that all
>> >>> reboots will invoke the .shutdown() method -- what about crashes? What
>> >>> about watchdog resets? IIUC, those will hit the same broken behavior,
>> >>> and have unexepcted behavior in your bootloader.
>> >>
>> >> Yes, there are corner cases that are not addressed with this approach,
>> >
>> > Is a system crash really a corner case? :D
>> >
>> >> but it still seems to improve things. Of course, that means the
>> >> user should try to re-route all HW reset sources to SW ones (RESET input
>> >> pin muxed to the GPIO controller, watchdog generating an interrupt
>> >> instead of directly asserting the RESET output pin), which is not always
>> >> possible, but even when it's not, isn't it better to have a setup that
>> >> works fine 99% of the time instead of 50% of the time?
>> >
>> > Perhaps, but not at the expense of future development. And
>> > realistically, no one is doing that if they have this hack. Most
>> > people won't even know that this hack is protecting them at all (so
>> > again, they won't try to mitigate the problem any further).
>> >
>> >>> I suppose one could argue for doing this in remove(), but AIUI you're
>> >>> just papering over system bugs by introducing the shutdown() function
>> >>> here. Thus, I'd prefer we drop the shutdown() method to avoid misleading
>> >>> other users of this driver.
>> >>
>> >> I understand your point. But if the problem is about making sure people
>> >> designing new boards get that right, why not complaining at probe time
>> >> when things are wrong?
>> >>
>> >> I mean, spi_nor_restore() seems to only do something on very specific
>> >> NORs (those on which a SW RESET does not resets the addressing
>> >> mode).
>> >
>> > The point isn't that SW RESET doesn't reset the addressing mode -- it
>> > does on any flash I've seen. The point is that most systems are built
>> > around a stateless assumption in these flash. IIRC, there wasn't even
>> > a SW RESET command at all until these "huge" flash came around and
>> > stateful addressing modes came about. So boot ROMs and bootloaders
>> > would have to be updated to start figuring out when/how to do this SW
>> > RESET. And once two vendors start doing it differently (I'm not sure:
>> > have they done this already? I think so) it's no longer something a
>> > boot ROM will get right.
>> >
>> > The only way to get this stuff right is to have a hardware reset, or
>> > else to avoid all of the stateful modes in software.
>> >
>> >> So, how about adding a flag that says "my board has the NOR HW
>> >> RESET pin wired" (there would be a DT props to set that flag). Then you
>> >> add a WARN_ON() when this flag is not set and a NOR chip impacted by
>> >> this bug is detected.
>> >
>> > I'd kinda prefer the reverse. There really isn't a need to document
>> > anything for a working system (software usually can't control this
>> > RESET pin). The burden should be on the b0rked system to document
>> > where it needs unsound hacks to survive.
>> >
>> >> This way you make sure people are informed that
>> >> they're doing something wrong, and for those who can't change their HW
>> >> (because it's already widely deployed), you have a fix that improve
>> >> things.
>> >
>> > Or even better: put this hack behind a DT flag, so that one has to
>> > admit that their board design is broken before it will even do
>> > anything. Proposal: "linux,badly-designed-flash-reset".
>> >
>> > But, I'd prefer just (partially?) reverting this, and let the authors
>> > submit something that works. We're not obligated to keep bad hacks in
>> > the kernel.
>> >
>> > Brian
>>
>> One possibility that occurred to me when I was exploring this issue is
>> to revert to 3-byte mode whenever 4-byte was not actively in use.
>> So any access beyond 16Meg is:
>> switch-to-4-byte ; perform IO ; switch to 3-byte
>> or similar. On my hardware it would be more efficient to
>> use the 4-byte opcode to perform the IO, then reset the cached
>> 4th address byte that the NOR chip transparently remembered.
>>
>> This adds a little overhead, but should be fairly robust.
>> It doesn't help if something goes terribly wrong while IO is happening,
>> but I don't think any other software solution does either.
>>
>> How would you see that approach?
>
> I think the problem stands: people that have proper HW mitigation for
> this problem (NOR chip is reset when the Processor is reset) don't want
> to pay the overhead. So, even if we go for this approach, we probably
> want to only do that for broken HW.
I agree that a "my-hardware-is-suboptimal" flag is appropriate.
I was addressing the suggestion that the current approach doesn't handle
all corner cases and was suggesting a different approach that might
handle more corner-cases. I should have been more explicit about that.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: m68k allmodconfig build errors
From: Randy Dunlap @ 2018-07-24 1:52 UTC (permalink / raw)
To: Andreas Schwab; +Cc: LKML, Geert Uytterhoeven, linux-m68k
In-Reply-To: <878t66idai.fsf@igel.home>
On 07/20/2018 12:20 AM, Andreas Schwab wrote:
> On Jul 19 2018, Randy Dunlap <rdunlap@infradead.org> wrote:
>
>> block/partitions/ldm.o: In function `ldm_partition':
>> ldm.c:(.text+0x1900): undefined reference to `strcmp'
>> ldm.c:(.text+0x1964): undefined reference to `strcmp'
>> drivers/rtc/rtc-proc.o: In function `is_rtc_hctosys':
>> rtc-proc.c:(.text+0x290): undefined reference to `strcmp'
>> drivers/watchdog/watchdog_pretimeout.o: In function `watchdog_register_governor':
>> (.text+0x142): undefined reference to `strcmp'
>
> GCC has optimized strncmp to strcmp, but at a stage where macros are no
> longer available. I think the right fix is to use strcmp directly,
> since strncmp doesn't make sense here.
Hi Andreas,
I don't see that all of these string compare fields are null-terminated.
How does one convert strncmp() users to strcmp()?
thanks,
--
~Randy
^ permalink raw reply
* Re: [PATCH 3/3] microblaze: add endianness options to LDFLAGS instead of LD
From: Masahiro Yamada @ 2018-07-24 1:53 UTC (permalink / raw)
To: Michal Simek; +Cc: Masahiro Yamada, Linux Kernel Mailing List, linux-arch
In-Reply-To: <1530580921-23340-4-git-send-email-yamada.masahiro@socionext.com>
Hi Michal,
Ping?
2018-07-03 10:22 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> With the recent syntax extension, Kconfig is now able to evaluate the
> compiler / toolchain capability.
>
> However, accumulating flags to 'LD' is not compatible with the way
> it works; 'LD' must be passed to Kconfig to call $(ld-option,...)
> from Kconfig files. If you tweak 'LD' in arch Makefile depending on
> CONFIG_CPU_BIG_ENDIAN, this would end up with circular dependency
> between Makefile and Kconfig.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> arch/microblaze/Makefile | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/microblaze/Makefile b/arch/microblaze/Makefile
> index d269dd4b..7333036 100644
> --- a/arch/microblaze/Makefile
> +++ b/arch/microblaze/Makefile
> @@ -40,11 +40,11 @@ CPUFLAGS-$(CONFIG_XILINX_MICROBLAZE0_USE_PCMP_INSTR) += -mxl-pattern-compare
> ifdef CONFIG_CPU_BIG_ENDIAN
> KBUILD_CFLAGS += -mbig-endian
> KBUILD_AFLAGS += -mbig-endian
> -LD += -EB
> +LDFLAGS += -EB
> else
> KBUILD_CFLAGS += -mlittle-endian
> KBUILD_AFLAGS += -mlittle-endian
> -LD += -EL
> +LDFLAGS += -EL
> endif
>
> CPUFLAGS-1 += $(call cc-option,-mcpu=v$(CPU_VER))
> --
> 2.7.4
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* [PATCH net-next] tcp: ack immediately when a cwr packet arrives
From: Lawrence Brakmo @ 2018-07-24 0:49 UTC (permalink / raw)
To: netdev
Cc: Kernel Team, Alexei Starovoitov, Neal Cardwell, Yuchung Cheng,
Eric Dumazet
We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
problem is triggered when the last packet of a request arrives CE
marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
to 1 (because there are no packets in flight). When the 1st packet of
the next request arrives, the ACK was sometimes delayed even though it
is CWR marked, adding up to 40ms to the RPC latency.
This patch insures that CWR marked data packets arriving will be acked
immediately.
Packetdrill script to reproduce the problem:
0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
0.000 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
0.000 setsockopt(3, SOL_TCP, TCP_CONGESTION, "dctcp", 5) = 0
0.000 bind(3, ..., ...) = 0
0.000 listen(3, 1) = 0
0.100 < [ect0] SEW 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
0.100 > SE. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 8>
0.110 < [ect0] . 1:1(0) ack 1 win 257
0.200 accept(3, ..., ...) = 4
0.200 < [ect0] . 1:1001(1000) ack 1 win 257
0.200 > [ect01] . 1:1(0) ack 1001
0.200 write(4, ..., 1) = 1
0.200 > [ect01] P. 1:2(1) ack 1001
0.200 < [ect0] . 1001:2001(1000) ack 2 win 257
0.200 write(4, ..., 1) = 1
0.200 > [ect01] P. 2:3(1) ack 2001
0.200 < [ect0] . 2001:3001(1000) ack 3 win 257
0.200 < [ect0] . 3001:4001(1000) ack 3 win 257
0.200 > [ect01] . 3:3(0) ack 4001
0.210 < [ce] P. 4001:4501(500) ack 3 win 257
+0.001 read(4, ..., 4500) = 4500
+0 write(4, ..., 1) = 1
+0 > [ect01] PE. 3:4(1) ack 4501
+0.010 < [ect0] W. 4501:5501(1000) ack 4 win 257
// Previously the ACK sequence below would be 4501, causing a long RTO
+0.040~+0.045 > [ect01] . 4:4(0) ack 5501 // delayed ack
+0.311 < [ect0] . 5501:6501(1000) ack 4 win 257 // More data
+0 > [ect01] . 4:4(0) ack 6501 // now acks everything
+0.500 < F. 9501:9501(0) ack 4 win 257
Modified based on comments by Neal Cardwell <ncardwell@google.com>
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
net/ipv4/tcp_input.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 91dbb9afb950..2370fd79c5c5 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -246,8 +246,15 @@ static void tcp_ecn_queue_cwr(struct tcp_sock *tp)
static void tcp_ecn_accept_cwr(struct tcp_sock *tp, const struct sk_buff *skb)
{
- if (tcp_hdr(skb)->cwr)
+ if (tcp_hdr(skb)->cwr) {
tp->ecn_flags &= ~TCP_ECN_DEMAND_CWR;
+
+ /* If the sender is telling us it has entered CWR, then its
+ * cwnd may be very low (even just 1 packet), so we should ACK
+ * immediately.
+ */
+ tcp_enter_quickack_mode((struct sock *)tp, 2);
+ }
}
static void tcp_ecn_withdraw_cwr(struct tcp_sock *tp)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/2] block: move dif_prepare/dif_complete functions to block layer
From: Martin K. Petersen @ 2018-07-24 1:54 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Max Gurtovoy, martin.petersen, linux-block, axboe, keith.busch,
linux-nvme, sagi
In-Reply-To: <20180723072854.GA18365@lst.de>
Christoph,
>> +void blk_integrity_dif_prepare(struct request *rq, u8 protection_type,
>> + u32 ref_tag)
>> +{
>
> Maybe call this blk_t10_pi_prepare?
The rest of these functions have a blk_integrity_ prefix. So either
stick with that or put the functions in t10-pi.c and use a t10_pi_
prefix.
I'm a bit torn on placement since the integrity metadata could contain
other stuff than T10 PI. But the remapping is very specific to T10 PI.
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 9421d9877730..4186bf027c59 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1119,7 +1119,9 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>> SCpnt->cmnd[0] = WRITE_6;
>>
>> if (blk_integrity_rq(rq))
>> - sd_dif_prepare(SCpnt);
>> + blk_integrity_dif_prepare(SCpnt->request,
>> + sdkp->protection_type,
>> + scsi_prot_ref_tag(SCpnt));
>
> scsi_prot_ref_tag could be move to the block layer as it only uses
> the sector in the eequest and the sector size, which we can get
> from the gendisk as well. We then don't need to pass it to the function.
For Type 2, the PI can be at intervals different from the logical block
size (although we don't support that yet). We should use the
blk_integrity profile interval instead of assuming sector size.
And wrt. Keith's comment: The tuple_size should be the one from the
integrity profile as well, not sizeof(struct t10_pi_tuple).
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* [PATCH 1/2] block: move dif_prepare/dif_complete functions to block layer
From: Martin K. Petersen @ 2018-07-24 1:54 UTC (permalink / raw)
In-Reply-To: <20180723072854.GA18365@lst.de>
Christoph,
>> +void blk_integrity_dif_prepare(struct request *rq, u8 protection_type,
>> + u32 ref_tag)
>> +{
>
> Maybe call this blk_t10_pi_prepare?
The rest of these functions have a blk_integrity_ prefix. So either
stick with that or put the functions in t10-pi.c and use a t10_pi_
prefix.
I'm a bit torn on placement since the integrity metadata could contain
other stuff than T10 PI. But the remapping is very specific to T10 PI.
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 9421d9877730..4186bf027c59 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -1119,7 +1119,9 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
>> SCpnt->cmnd[0] = WRITE_6;
>>
>> if (blk_integrity_rq(rq))
>> - sd_dif_prepare(SCpnt);
>> + blk_integrity_dif_prepare(SCpnt->request,
>> + sdkp->protection_type,
>> + scsi_prot_ref_tag(SCpnt));
>
> scsi_prot_ref_tag could be move to the block layer as it only uses
> the sector in the eequest and the sector size, which we can get
> from the gendisk as well. We then don't need to pass it to the function.
For Type 2, the PI can be at intervals different from the logical block
size (although we don't support that yet). We should use the
blk_integrity profile interval instead of assuming sector size.
And wrt. Keith's comment: The tuple_size should be the one from the
integrity profile as well, not sizeof(struct t10_pi_tuple).
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH v2] systemd: Allow custom coredump config options
From: Andre McCurdy @ 2018-07-24 1:56 UTC (permalink / raw)
To: Alistair Francis; +Cc: OE Core mailing list
In-Reply-To: <CAKmqyKNrUdU9wcUH8no9J_XNE=4ztujQ95tap7Zk5HBWv9XT9g@mail.gmail.com>
On Mon, Jul 23, 2018 at 4:16 PM, Alistair Francis <alistair23@gmail.com> wrote:
> On Wed, Jul 18, 2018 at 4:48 PM, Andre McCurdy <armccurdy@gmail.com> wrote:
>> On Wed, Jul 18, 2018 at 3:53 PM, Alistair Francis
>> <alistair.francis@wdc.com> wrote:
>>> If the user has enabled coredump let's allow them to customise the
>>> config options.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>> meta/recipes-core/systemd/systemd_239.bb | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/meta/recipes-core/systemd/systemd_239.bb b/meta/recipes-core/systemd/systemd_239.bb
>>> index 7822548993..a625d060a9 100644
>>> --- a/meta/recipes-core/systemd/systemd_239.bb
>>> +++ b/meta/recipes-core/systemd/systemd_239.bb
>>> @@ -279,6 +279,18 @@ do_install() {
>>> chown polkitd:root ${D}${datadir}/polkit-1/rules.d
>>> fi
>>> fi
>>> +
>>> + # If coredump was enabled, enable it in the config.
>>> + # This just sets the default, but can be used to customise the values.
>>> + if ${@bb.utils.contains('PACKAGECONFIG', 'coredump', 'true', 'false', d)}; then
>>> + sed 's|#Storage.*|Storage=external|g' -i ${D}${sysconfdir}/systemd/coredump.conf
>>> + sed 's|#Compress.*|Compress=yes|g' -i ${D}${sysconfdir}/systemd/coredump.conf
>>> + sed 's|#ProcessSizeMax.*|ProcessSizeMax=2G|g' -i ${D}${sysconfdir}/systemd/coredump.conf
>>> + sed 's|#ExternalSizeMax.*|ExternalSizeMax=2G|g' -i ${D}${sysconfdir}/systemd/coredump.conf
>>> + sed 's|#JournalSizeMax.*|JournalSizeMax=767M|g' -i ${D}${sysconfdir}/systemd/coredump.conf
>>> + sed 's|#MaxUse.*|MaxUse=|g' -i ${D}${sysconfdir}/systemd/coredump.conf
>>> + sed 's|#KeepFree.*|KeepFree=|g' -i ${D}${sysconfdir}/systemd/coredump.conf
>>
>> How does this allow the user to customise the config options?
>
> They can edit the sed command line and have different options.
Editing the sed commands won't help users who prefer to customise
their builds via a .bbappend or by over-riding variables from
local.conf.
For users who edit recipes directly, providing a place-holder may not
be very useful either since those users can edit the recipe to their
liking and add any sed commands etc they need anyway. So this change
would be useful to quite a narrow audience.
>> By forcing the options to the _current_ default values you are
>> creating a maintenance problem - ie you'll still be forcing these
>> values even if/when the upstream defaults are changed.
>
> That is a good point, is there a usual way to allow edits to config
> files like this?
There are a few different ways. If an upstream reference config file
is really unusable then it might be OK to patch it from the recipe.
Perhaps the ideal case though is for the configure process to provide
control over any key values etc in the files which get installed. e.g.
instead of shipping and installing a fixed "options.conf" file, the
upstream source would ship an "options.conf.in" file which would get
transformed into "options.conf" as part of running the configure
process, based on configure options or auto detection etc in the
configure script. Configuring a build via configure options is
generally the preferred approach (better than patching or running sed
on output files etc).
^ permalink raw reply
* [PATCH v2 2/2] PCI: NVMe device specific reset quirk
From: Alex Williamson @ 2018-07-24 1:57 UTC (permalink / raw)
In-Reply-To: <abc86e6b-7df5-a84c-7e98-107c605812c6@kernel.org>
On Mon, 23 Jul 2018 17:40:02 -0700
Sinan Kaya <okaya@kernel.org> wrote:
> On 7/23/2018 5:13 PM, Alex Williamson wrote:
> > + * The NVMe specification requires that controllers support PCIe FLR, but
> > + * but some Samsung SM961/PM961 controllers fail to recover after FLR (-1
> > + * config space) unless the device is quiesced prior to FLR.
>
> Does disabling the memory bit in PCI config space as part of the FLR
> reset function help? (like the very first thing)
No, it does not. I modified this to only clear PCI_COMMAND_MEMORY and
call pcie_flr(), the Samsung controller dies just as it did previously.
> Can we do that in the pcie_flr() function to cover other endpoint types
> that might be pushing traffic while code is trying to do a reset?
Do you mean PCI_COMMAND_MASTER rather than PCI_COMMAND_MEMORY? I tried
that too, it doesn't work either. I'm not really sure the theory
behind clearing memory, clearing busmaster to stop DMA seems like a
sane thing to do, but doesn't help here. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v2 2/2] PCI: NVMe device specific reset quirk
From: Alex Williamson @ 2018-07-24 1:57 UTC (permalink / raw)
To: Sinan Kaya; +Cc: linux-pci, linux-kernel, linux-nvme
In-Reply-To: <abc86e6b-7df5-a84c-7e98-107c605812c6@kernel.org>
On Mon, 23 Jul 2018 17:40:02 -0700
Sinan Kaya <okaya@kernel.org> wrote:
> On 7/23/2018 5:13 PM, Alex Williamson wrote:
> > + * The NVMe specification requires that controllers support PCIe FLR, but
> > + * but some Samsung SM961/PM961 controllers fail to recover after FLR (-1
> > + * config space) unless the device is quiesced prior to FLR.
>
> Does disabling the memory bit in PCI config space as part of the FLR
> reset function help? (like the very first thing)
No, it does not. I modified this to only clear PCI_COMMAND_MEMORY and
call pcie_flr(), the Samsung controller dies just as it did previously.
> Can we do that in the pcie_flr() function to cover other endpoint types
> that might be pushing traffic while code is trying to do a reset?
Do you mean PCI_COMMAND_MASTER rather than PCI_COMMAND_MEMORY? I tried
that too, it doesn't work either. I'm not really sure the theory
behind clearing memory, clearing busmaster to stop DMA seems like a
sane thing to do, but doesn't help here. Thanks,
Alex
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCH v2 2/2] PCI: NVMe device specific reset quirk
From: Alex Williamson @ 2018-07-24 1:57 UTC (permalink / raw)
To: Sinan Kaya; +Cc: linux-pci, linux-kernel, linux-nvme
In-Reply-To: <abc86e6b-7df5-a84c-7e98-107c605812c6@kernel.org>
On Mon, 23 Jul 2018 17:40:02 -0700
Sinan Kaya <okaya@kernel.org> wrote:
> On 7/23/2018 5:13 PM, Alex Williamson wrote:
> > + * The NVMe specification requires that controllers support PCIe FLR, but
> > + * but some Samsung SM961/PM961 controllers fail to recover after FLR (-1
> > + * config space) unless the device is quiesced prior to FLR.
>
> Does disabling the memory bit in PCI config space as part of the FLR
> reset function help? (like the very first thing)
No, it does not. I modified this to only clear PCI_COMMAND_MEMORY and
call pcie_flr(), the Samsung controller dies just as it did previously.
> Can we do that in the pcie_flr() function to cover other endpoint types
> that might be pushing traffic while code is trying to do a reset?
Do you mean PCI_COMMAND_MASTER rather than PCI_COMMAND_MEMORY? I tried
that too, it doesn't work either. I'm not really sure the theory
behind clearing memory, clearing busmaster to stop DMA seems like a
sane thing to do, but doesn't help here. Thanks,
Alex
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
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.