Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280889 > unrolled thread

Re: [PATCH v3 1/3] resource: Add @flags to region_intersects()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Borislav Petkov <bp@alien8.de> - 2015-12-01 15:00 +0100
    Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Dan Williams <dan.j.williams@intel.com> - 2015-12-01 18:00 +0100
      Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Jeff Moyer <jmoyer@redhat.com> - 2015-12-01 18:10 +0100
      Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-01 18:20 +0100
        Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Toshi Kani <toshi.kani@hpe.com> - 2015-12-03 19:00 +0100
          Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Borislav Petkov <bp@alien8.de> - 2015-12-03 19:50 +0100
            Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-03 20:10 +0100
              Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Toshi Kani <toshi.kani@hpe.com> - 2015-12-03 20:50 +0100
                Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Dan Williams <dan.j.williams@gmail.com> - 2015-12-09 17:30 +0100
                  Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Toshi Kani <toshi.kani@hpe.com> - 2015-12-09 22:50 +0100
      Re: [PATCH v3 1/3] resource: Add @flags to region_intersects() Borislav Petkov <bp@alien8.de> - 2015-12-01 18:20 +0100

#1280889 — Re: [PATCH v3 1/3] resource: Add @flags to region_intersects()

On Tue, Nov 24, 2015 at 03:33:36PM -0700, Toshi Kani wrote:
> region_intersects() checks if a specified region partially overlaps
> or fully eclipses a resource identified by @name.  It currently sets
> resource flags statically, which prevents the caller from specifying
> a non-RAM region, such as persistent memory.  Add @flags so that
> any region can be specified to the function.
> 
> A helper function, region_intersects_ram(), is added so that the
> callers that check a RAM region do not have to specify its iomem
> resource name and flags.  This interface is exported for modules,
> such as the EINJ driver.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  include/linux/mm.h |    4 +++-
>  kernel/memremap.c  |    5 ++---
>  kernel/resource.c  |   23 ++++++++++++++++-------
>  3 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 00bad77..c776af3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -362,7 +362,9 @@ enum {
>  	REGION_MIXED,
>  };
>  
> -int region_intersects(resource_size_t offset, size_t size, const char *type);
> +int region_intersects(resource_size_t offset, size_t size, const char *type,
> +			unsigned long flags);
> +int region_intersects_ram(resource_size_t offset, size_t size);
>  
>  /* Support for virtually mapped pages */
>  struct page *vmalloc_to_page(const void *addr);
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index 7658d32..98f52f1 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
>   */
>  void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>  {
> -	int is_ram = region_intersects(offset, size, "System RAM");

Ok, question: why do those resource things types gets identified with
a string?! We have here "System RAM" and next patch adds "Persistent
Memory".

And "persistent memory" or "System RaM" won't work and this is just
silly.

Couldn't struct resource have gained some typedef flags instead which we
can much easily test? Using the strings looks really yucky.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1281045

On Tue, Dec 1, 2015 at 5:50 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Nov 24, 2015 at 03:33:36PM -0700, Toshi Kani wrote:
>> region_intersects() checks if a specified region partially overlaps
>> or fully eclipses a resource identified by @name.  It currently sets
>> resource flags statically, which prevents the caller from specifying
>> a non-RAM region, such as persistent memory.  Add @flags so that
>> any region can be specified to the function.
>>
>> A helper function, region_intersects_ram(), is added so that the
>> callers that check a RAM region do not have to specify its iomem
>> resource name and flags.  This interface is exported for modules,
>> such as the EINJ driver.
>>
>> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> ---
>>  include/linux/mm.h |    4 +++-
>>  kernel/memremap.c  |    5 ++---
>>  kernel/resource.c  |   23 ++++++++++++++++-------
>>  3 files changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 00bad77..c776af3 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -362,7 +362,9 @@ enum {
>>       REGION_MIXED,
>>  };
>>
>> -int region_intersects(resource_size_t offset, size_t size, const char *type);
>> +int region_intersects(resource_size_t offset, size_t size, const char *type,
>> +                     unsigned long flags);
>> +int region_intersects_ram(resource_size_t offset, size_t size);
>>
>>  /* Support for virtually mapped pages */
>>  struct page *vmalloc_to_page(const void *addr);
>> diff --git a/kernel/memremap.c b/kernel/memremap.c
>> index 7658d32..98f52f1 100644
>> --- a/kernel/memremap.c
>> +++ b/kernel/memremap.c
>> @@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
>>   */
>>  void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>>  {
>> -     int is_ram = region_intersects(offset, size, "System RAM");
>
> Ok, question: why do those resource things types gets identified with
> a string?! We have here "System RAM" and next patch adds "Persistent
> Memory".
>
> And "persistent memory" or "System RaM" won't work and this is just
> silly.
>
> Couldn't struct resource have gained some typedef flags instead which we
> can much easily test? Using the strings looks really yucky.
>

At least in the case of region_intersects() I was just following
existing strcmp() convention from walk_system_ram_range.

We could define 'const char *system_ram = "System RAM"' somewhere and
then do pointer comparisons to cut down on the thrash of adding new
flags to 'struct resource'?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281054

Dan Williams <dan.j.williams@intel.com> writes:

>>> @@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
>>>   */
>>>  void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>>>  {
>>> -     int is_ram = region_intersects(offset, size, "System RAM");
>>
>> Ok, question: why do those resource things types gets identified with
>> a string?! We have here "System RAM" and next patch adds "Persistent
>> Memory".
>>
>> And "persistent memory" or "System RaM" won't work and this is just
>> silly.
>>
>> Couldn't struct resource have gained some typedef flags instead which we
>> can much easily test? Using the strings looks really yucky.
>>
>
> At least in the case of region_intersects() I was just following
> existing strcmp() convention from walk_system_ram_range.

...which is done in the page fault path.  I agree with the suggestion to
get strcmp out of that path.

-Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281057

On Tue, Dec 1, 2015 at 9:13 AM, Borislav Petkov <bp@alien8.de> wrote:
>
> Oh sure, I didn't mean you. I was simply questioning that whole
> identify-resource-by-its-name approach. And that came with:
>
> 67cf13ceed89 ("x86: optimize resource lookups for ioremap")
>
> I just think it is silly and that we should be identifying resource
> things in a more robust way.

I could easily imagine just adding a IORESOURCE_RAM flag (or SYSMEM or
whatever). That sounds sane. I agree that comparing the string is
ugly.

> Btw, the ->name thing in struct resource has been there since a *long*
> time

It's pretty much always been there.  It is indeed meant for things
like /proc/iomem etc, and as a debug aid when printing conflicts,
yadda yadda. Just showing the numbers is usually useless for figuring
out exactly *what* something conflicts with.

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283221

On Tue, 2015-12-01 at 09:19 -0800, Linus Torvalds wrote:
> On Tue, Dec 1, 2015 at 9:13 AM, Borislav Petkov <bp@alien8.de> wrote:
> > 
> > Oh sure, I didn't mean you. I was simply questioning that whole
> > identify-resource-by-its-name approach. And that came with:
> > 
> > 67cf13ceed89 ("x86: optimize resource lookups for ioremap")
> > 
> > I just think it is silly and that we should be identifying resource
> > things in a more robust way.
> 
> I could easily imagine just adding a IORESOURCE_RAM flag (or SYSMEM or
> whatever). That sounds sane. I agree that comparing the string is
> ugly.
> 
> > Btw, the ->name thing in struct resource has been there since a *long*
> > time
> 
> It's pretty much always been there.  It is indeed meant for things
> like /proc/iomem etc, and as a debug aid when printing conflicts,
> yadda yadda. Just showing the numbers is usually useless for figuring
> out exactly *what* something conflicts with.

I agree that regular memory should have its own type, which separates
itself from MMIO.  By looking at how IORESOURCE types are used, this change
has the following challenges, and I am sure I missed some more.

1. Large number of IORESOURCE_MEM usage
Adding a new type for regular memory will require inspecting the codes
using IORESOURCE_MEM currently, and modify them to use the new type if
their target ranges are regular memory.  There are many references to this
type across multiple architectures and drivers, which make this inspection
and testing challenging.

http://lxr.free-electrons.com/ident?i=IORESOURCE_MEM

2. Lack of free flags bit in resource
The flags bits are defined in include/linux/ioport.h.  The flags are
defined as unsigned long, which is 32-bit in 32-bit config.  The most of
the bits have been assigned already.  Bus-specific bits for IORESOURCE_MEM
have been assigned mostly as well (line 82).

3. Interaction with pnp subsystem
The same IORESOURCE types and bus-specific flags are used by the pnp
subsystem.  pnp_mem objects represent IORESOURCE_MEM type listed by
pnp_dev.  Adding a new IORESOURCE type likely requires adding a new object
type and its interfaces to pnp.

4. I/O resource names represent allocation types
While IORESOURCE types represent hardware types and capabilities, the
string names represent resource allocation types and usages.  For instance,
regular memory is allocated for the OS as "System RAM", kdump as "Crash
kernel", FW as "ACPI Tables", and so on.  Hence, a new type representing
"System RAM" needs to be usage based, which is different from the current
IORESOURCE types.

I think this work will require a separate patch series at least.  For this
patch series, supporting error injections to NVDIMM, I propose that we make
the change suggested by Dan:

"We could define 'const char *system_ram = "System RAM"' somewhere andthen
do pointer comparisons to cut down on the thrash of adding newflags to
'struct resource'?"

Let me know if you have any suggestions/concerns. 

Thanks,
-Toshi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283261

On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
> Adding a new type for regular memory will require inspecting the codes
> using IORESOURCE_MEM currently, and modify them to use the new type if
> their target ranges are regular memory.  There are many references to this
> type across multiple architectures and drivers, which make this inspection
> and testing challenging.

What's wrong with adding a new type_flags to struct resource and not
touching IORESOURCE_* at all?

They'll be called something like RES_TYPE_RAM, _PMEM, _SYSMEM...

Or would that confuse more...?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283268

On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
>> Adding a new type for regular memory will require inspecting the codes
>> using IORESOURCE_MEM currently, and modify them to use the new type if
>> their target ranges are regular memory.  There are many references to this
>> type across multiple architectures and drivers, which make this inspection
>> and testing challenging.
>
> What's wrong with adding a new type_flags to struct resource and not
> touching IORESOURCE_* at all?

Bah. Both of these ideas are bogus.

Just add a new flag. The bits are already modifiers that you can
*combine* to show what kind of resource it is, and we already have
things like IORESOURCE_PREFETCH etc, that are in *addition* to the
normal IORESOURCE_MEM bit.

Just add another modifier: IORESOURCE_RAM.

So it would still show up as IORESOURCE_MEM, but it would have
additional information specifying that it's actually RAM.

If somebody does something like

     if (res->flags == IORESOURCE_MEM)

then they are already completely broken and won't work *anyway*. It's
a bitmask, bit a set of values.

             Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283291

On Thu, 2015-12-03 at 11:01 -0800, Linus Torvalds wrote:
> On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <bp@alien8.de> wrote:
> > On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
> > > Adding a new type for regular memory will require inspecting the 
> > > codes using IORESOURCE_MEM currently, and modify them to use the new 
> > > type if their target ranges are regular memory.  There are many 
> > > references to this type across multiple architectures and drivers, 
> > > which make this inspection and testing challenging.
> > 
> > What's wrong with adding a new type_flags to struct resource and not
> > touching IORESOURCE_* at all?
> 
> Bah. Both of these ideas are bogus.
> 
> Just add a new flag. The bits are already modifiers that you can
> *combine* to show what kind of resource it is, and we already have
> things like IORESOURCE_PREFETCH etc, that are in *addition* to the
> normal IORESOURCE_MEM bit.
> 
> Just add another modifier: IORESOURCE_RAM.
> 
> So it would still show up as IORESOURCE_MEM, but it would have
> additional information specifying that it's actually RAM.
> 
> If somebody does something like
> 
>      if (res->flags == IORESOURCE_MEM)
> 
> then they are already completely broken and won't work *anyway*. It's
> a bitmask, bit a set of values.

Yes, if we can assign new modifiers, that will be quite simple. :-)  I
assume we can allocate new bits from the remaining free bits as follows.

+#define IORESOURCE_SYSTEM_RAM  0x01000000      /* System RAM */
+#define IORESOURCE_PMEM        0x02000000      /* Persistent memory */
 #define IORESOURCE_EXCLUSIVE   0x08000000      /* Userland may not map
this resource */

Note, SYSTEM_RAM represents the OS memory, i.e. "System RAM", not any RAM
ranges.

With the new modifiers, region_intersect() can check these ranges.  One
caveat is that the modifiers are not very extensible for new types as they
are bit maps.  region_intersect() will no longer be capable of checking any
regions with any given name.  I think this is OK since this function was
introduced recently, and is only used for checking "System RAM" and
"Persistent Memory" (with this patch series).

Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287626

On Thu, Dec 3, 2015 at 12:35 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Thu, 2015-12-03 at 11:01 -0800, Linus Torvalds wrote:
>> On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <bp@alien8.de> wrote:
>> > On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
>> > > Adding a new type for regular memory will require inspecting the
>> > > codes using IORESOURCE_MEM currently, and modify them to use the new
>> > > type if their target ranges are regular memory.  There are many
>> > > references to this type across multiple architectures and drivers,
>> > > which make this inspection and testing challenging.
>> >
>> > What's wrong with adding a new type_flags to struct resource and not
>> > touching IORESOURCE_* at all?
>>
>> Bah. Both of these ideas are bogus.
>>
>> Just add a new flag. The bits are already modifiers that you can
>> *combine* to show what kind of resource it is, and we already have
>> things like IORESOURCE_PREFETCH etc, that are in *addition* to the
>> normal IORESOURCE_MEM bit.
>>
>> Just add another modifier: IORESOURCE_RAM.
>>
>> So it would still show up as IORESOURCE_MEM, but it would have
>> additional information specifying that it's actually RAM.
>>
>> If somebody does something like
>>
>>      if (res->flags == IORESOURCE_MEM)
>>
>> then they are already completely broken and won't work *anyway*. It's
>> a bitmask, bit a set of values.
>
> Yes, if we can assign new modifiers, that will be quite simple. :-)  I
> assume we can allocate new bits from the remaining free bits as follows.
>
> +#define IORESOURCE_SYSTEM_RAM  0x01000000      /* System RAM */
> +#define IORESOURCE_PMEM        0x02000000      /* Persistent memory */
>  #define IORESOURCE_EXCLUSIVE   0x08000000      /* Userland may not map
> this resource */
>
> Note, SYSTEM_RAM represents the OS memory, i.e. "System RAM", not any RAM
> ranges.
>
> With the new modifiers, region_intersect() can check these ranges.  One
> caveat is that the modifiers are not very extensible for new types as they
> are bit maps.  region_intersect() will no longer be capable of checking any
> regions with any given name.  I think this is OK since this function was
> introduced recently, and is only used for checking "System RAM" and
> "Persistent Memory" (with this patch series).

IORESOURCE_PMEM is not descriptive enough for the two different types
of pmem in the kernel.  How about we go with just
IORESOURCE_SYSTEM_RAM for now since "is_ram()" checks are common.  Let
the rest continue to be checked by strcmp().

For example the nvdimm-e820 driver cares about "Persistent Memory
(legacy)", while other forms of pmem may just be "reserved" and only
the driver knows that it is pmem.  An IORESOURCE_PMEM would not be
reliable nor descriptive enough.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287898

On Wed, 2015-12-09 at 08:25 -0800, Dan Williams wrote:
> On Thu, Dec 3, 2015 at 12:35 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Thu, 2015-12-03 at 11:01 -0800, Linus Torvalds wrote:
> > > On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <bp@alien8.de>
> > > wrote:
> > > > On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
> > > > > Adding a new type for regular memory will require inspecting the
> > > > > codes using IORESOURCE_MEM currently, and modify them to use the 
> > > > > new type if their target ranges are regular memory.  There are 
> > > > > many references to this type across multiple architectures and
> > > > > drivers, which make this inspection and testing challenging.
> > > > 
> > > > What's wrong with adding a new type_flags to struct resource and 
> > > > not touching IORESOURCE_* at all?
> > > 
> > > Bah. Both of these ideas are bogus.
> > > 
> > > Just add a new flag. The bits are already modifiers that you can
> > > *combine* to show what kind of resource it is, and we already have
> > > things like IORESOURCE_PREFETCH etc, that are in *addition* to the
> > > normal IORESOURCE_MEM bit.
> > > 
> > > Just add another modifier: IORESOURCE_RAM.
> > > 
> > > So it would still show up as IORESOURCE_MEM, but it would have
> > > additional information specifying that it's actually RAM.
> > > 
> > > If somebody does something like
> > > 
> > >      if (res->flags == IORESOURCE_MEM)
> > > 
> > > then they are already completely broken and won't work *anyway*. It's
> > > a bitmask, bit a set of values.
> > 
> > Yes, if we can assign new modifiers, that will be quite simple. :-)  I
> > assume we can allocate new bits from the remaining free bits as
> > follows.
> > 
> > +#define IORESOURCE_SYSTEM_RAM  0x01000000      /* System RAM */
> > +#define IORESOURCE_PMEM        0x02000000      /* Persistent memory */
> >  #define IORESOURCE_EXCLUSIVE   0x08000000      /* Userland may not map
> > this resource */
> > 
> > Note, SYSTEM_RAM represents the OS memory, i.e. "System RAM", not any 
> > RAM ranges.
> > 
> > With the new modifiers, region_intersect() can check these ranges.  One
> > caveat is that the modifiers are not very extensible for new types as 
> > they are bit maps.  region_intersect() will no longer be capable of 
> > checking any regions with any given name.  I think this is OK since 
> > this function was introduced recently, and is only used for checking 
> > "System RAM" and "Persistent Memory" (with this patch series).
> 
> IORESOURCE_PMEM is not descriptive enough for the two different types
> of pmem in the kernel.  How about we go with just
> IORESOURCE_SYSTEM_RAM for now since "is_ram()" checks are common.  Let
> the rest continue to be checked by strcmp().
> 
> For example the nvdimm-e820 driver cares about "Persistent Memory
> (legacy)", while other forms of pmem may just be "reserved" and only
> the driver knows that it is pmem.  An IORESOURCE_PMEM would not be
> reliable nor descriptive enough.

Agreed.  I will introduce a new type for System RAM, and leave the strcmp
check for other types.

Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281061

On Tue, Dec 01, 2015 at 08:54:23AM -0800, Dan Williams wrote:
> On Tue, Dec 1, 2015 at 5:50 AM, Borislav Petkov <bp@alien8.de> wrote:
> > On Tue, Nov 24, 2015 at 03:33:36PM -0700, Toshi Kani wrote:
> >> region_intersects() checks if a specified region partially overlaps
> >> or fully eclipses a resource identified by @name.  It currently sets
> >> resource flags statically, which prevents the caller from specifying
> >> a non-RAM region, such as persistent memory.  Add @flags so that
> >> any region can be specified to the function.
> >>
> >> A helper function, region_intersects_ram(), is added so that the
> >> callers that check a RAM region do not have to specify its iomem
> >> resource name and flags.  This interface is exported for modules,
> >> such as the EINJ driver.
> >>
> >> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> >> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: Vishal Verma <vishal.l.verma@intel.com>
> >> ---
> >>  include/linux/mm.h |    4 +++-
> >>  kernel/memremap.c  |    5 ++---
> >>  kernel/resource.c  |   23 ++++++++++++++++-------
> >>  3 files changed, 21 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> index 00bad77..c776af3 100644
> >> --- a/include/linux/mm.h
> >> +++ b/include/linux/mm.h
> >> @@ -362,7 +362,9 @@ enum {
> >>       REGION_MIXED,
> >>  };
> >>
> >> -int region_intersects(resource_size_t offset, size_t size, const char *type);
> >> +int region_intersects(resource_size_t offset, size_t size, const char *type,
> >> +                     unsigned long flags);
> >> +int region_intersects_ram(resource_size_t offset, size_t size);
> >>
> >>  /* Support for virtually mapped pages */
> >>  struct page *vmalloc_to_page(const void *addr);
> >> diff --git a/kernel/memremap.c b/kernel/memremap.c
> >> index 7658d32..98f52f1 100644
> >> --- a/kernel/memremap.c
> >> +++ b/kernel/memremap.c
> >> @@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
> >>   */
> >>  void *memremap(resource_size_t offset, size_t size, unsigned long flags)
> >>  {
> >> -     int is_ram = region_intersects(offset, size, "System RAM");
> >
> > Ok, question: why do those resource things types gets identified with
> > a string?! We have here "System RAM" and next patch adds "Persistent
> > Memory".
> >
> > And "persistent memory" or "System RaM" won't work and this is just
> > silly.
> >
> > Couldn't struct resource have gained some typedef flags instead which we
> > can much easily test? Using the strings looks really yucky.
> >
> 
> At least in the case of region_intersects() I was just following
> existing strcmp() convention from walk_system_ram_range.

Oh sure, I didn't mean you. I was simply questioning that whole
identify-resource-by-its-name approach. And that came with:

67cf13ceed89 ("x86: optimize resource lookups for ioremap")

I just think it is silly and that we should be identifying resource
things in a more robust way.

Btw, the ->name thing in struct resource has been there since a *long*
time, added by:

commit 40f6b7cc623f95d2a08b9adae7a6793055af4768
Author: linus1 <torvalds@linuxfoundation.org>
Date:   Wed Jun 30 11:00:00 1999 -0600

    Import 2.3.11pre1

I'm not sure what it was used for, perhaps for human-readable output in
/proc/iomem.

Let me CC Linus, he would know, most likely. akpm is already on CC.

> We could define 'const char *system_ram = "System RAM"' somewhere and
> then do pointer comparisons to cut down on the thrash of adding new
> flags to 'struct resource'?

See above. I think flags or type_flags or so should be cleaner/better...

I could be missing some aspect though, according to which, the name is
the proper way to ident those but I can't think of one...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web