Re: [PATCH 03/12] __wr_after_init: generic functionality

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 21/12/2018 20:41, Matthew Wilcox wrote:
On Fri, Dec 21, 2018 at 08:14:14PM +0200, Igor Stoppa wrote:
+static inline int memtst(void *p, int c, __kernel_size_t len)

I don't understand why you're verifying that writes actually happen
in production code.  Sure, write lib/test_wrmem.c or something, but
verifying every single rare write seems like a mistake to me.

This is actually something I wrote more as a stop-gap.
I have the feeling there should be already something similar available.
And probably I could not find it. Unless it's so trivial that it doesn't deserve to become a function?

But if there is really no existing alternative, I can put it in a separate file.


+#ifndef CONFIG_PRMEM

So is this PRMEM or wr_mem?  It's not obvious that CONFIG_PRMEM controls
wrmem.

In my mind (maybe still clinging to the old implementation), PRMEM is the master toggle, for protected memory.

Then there are various types and the first one being now implemented is write rare after init (because ro after init already exists).

However, the same levels of protection should then follow for dynamically allocated memory (ye old pmalloc).

PRMEM would then become the moniker for the whole shebang.

+#define wr_assign(var, val)	((var) = (val))

The hamming distance between 'var' and 'val' is too small.  The convention
in the line immediately below (p and v) is much more readable.

ok, I'll fix it

+#define wr_rcu_assign_pointer(p, v)	rcu_assign_pointer(p, v)
+#define wr_assign(var, val) ({			\
+	typeof(var) tmp = (typeof(var))val;	\
+						\
+	wr_memcpy(&var, &tmp, sizeof(var));	\
+	var;					\
+})

Doesn't wr_memcpy return 'var' anyway?

It should return the destination, which is &var.

But I wanted to return the actual value of the assignment, val

Like if I do  (a = 7)  it evaluates to 7,

similarly wr_assign(a, 7) would also evaluate to 7

The reason why i returned var instead of val is that it would allow to detect any error.

+/**
+ * wr_memcpy() - copyes size bytes from q to p

typo

:-( thanks

+ * @p: beginning of the memory to write to
+ * @q: beginning of the memory to read from
+ * @size: amount of bytes to copy
+ *
+ * Returns pointer to the destination

+ * The architecture code must provide:
+ *   void __wr_enable(wr_state_t *state)
+ *   void *__wr_addr(void *addr)
+ *   void *__wr_memcpy(void *p, const void *q, __kernel_size_t size)
+ *   void __wr_disable(wr_state_t *state)

This section shouldn't be in the user documentation of wr_memcpy().

ok

+ */
+void *wr_memcpy(void *p, const void *q, __kernel_size_t size)
+{
+	wr_state_t wr_state;
+	void *wr_poking_addr = __wr_addr(p);
+
+	if (WARN_ONCE(!wr_ready, "No writable mapping available") ||

Surely not.  If somebody's called wr_memcpy() before wr_ready is set,
that means we can just call memcpy().


What I was trying to catch is the case where, after a failed init, the writable mapping doesn't exist. In that case wr_ready is also not set.

The problem is that I just don't know what to do in a case where there has been such a major error which prevents he creation of hte alternate mapping.

I understand that we still want to continue, to provide as much debug info as possible, but I am at a loss about finding the saner course of actions.

--
igor




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux