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

Groups > linux.kernel > #1533391 > unrolled thread

[PATCH] intelrdt: resctrl: recommend locking for resctrlfs

Back to article view | Back to linux.kernel

Contents

  [PATCH] intelrdt: resctrl: recommend locking for resctrlfs  Marcelo Tosatti <mtosatti@redhat.com> - 2016-11-30 16:50 +0100
    Re: [PATCH] intelrdt: resctrl: recommend locking for resctrlfs  Thomas Gleixner <tglx@linutronix.de> - 2016-11-30 22:20 +0100
    Re: [PATCH] intelrdt: resctrl: recommend locking for resctrlfs Fenghua Yu <fenghua.yu@intel.com> - 2016-11-30 23:10 +0100
      Re: [PATCH] intelrdt: resctrl: recommend locking for resctrlfs Marcelo Tosatti <mtosatti@redhat.com> - 2016-12-01 14:00 +0100
        Re: [PATCH] intelrdt: resctrl: recommend locking for resctrlfs Fenghua Yu <fenghua.yu@intel.com> - 2016-12-01 23:00 +0100
      [PATCH v2] intelrdt: resctrl: recommend locking for resctrlfs  Marcelo Tosatti <mtosatti@redhat.com> - 2016-12-02 12:00 +0100
        Re: [PATCH v2] intelrdt: resctrl: recommend locking for resctrlfs  Thomas Gleixner <tglx@linutronix.de> - 2016-12-02 12:30 +0100
          Re: [PATCH v2] intelrdt: resctrl: recommend locking for resctrlfs Marcelo Tosatti <mtosatti@redhat.com> - 2016-12-02 23:10 +0100
            Re: [PATCH v2] intelrdt: resctrl: recommend locking for resctrlfs Thomas Gleixner <tglx@linutronix.de> - 2016-12-09 10:40 +0100

#1533391 — [PATCH] intelrdt: resctrl: recommend locking for resctrlfs

There is a locking problem between different applications
reading/writing to resctrlfs directory at the same time (read the patch
below for details).

Suggest a standard locking scheme for applications to use.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

--- Documentation/x86/intel_rdt_ui.txt.orig	2016-11-30 13:40:33.080233101 -0200
+++ Documentation/x86/intel_rdt_ui.txt	2016-11-30 13:45:01.253703259 -0200
@@ -212,3 +212,30 @@ Finally we move core 4-7 over to the new
 kernel and the tasks running there get 50% of the cache.
 
 # echo C0 > p0/cpus
+
+4) Locking between applications
+
+The allocation of an exclusive reservation
+of L3 cache involves:
+
+        1. read list of cbmmasks for each directory
+        2. find a contiguous set of bits in the global CBM bitmask
+          that is clear in any of the directory cbmmasks
+        3. create a new directory
+        4. set the bits found in step 2 to the new directory "schemata"
+           file
+
+If two applications attempt to allocate space race with each other
+(if two processes execute the steps above in a interlocked fashion),
+they can end up using the same bits of CBMMASK, which renders the
+reservations non-exclusive but shared.
+
+To coordinate creation of reservations on resctrl and avoid the problem
+above, the following locking procedure is recommended:
+
+A) open /var/lock/resctrl/fs.lock with O_CREAT|O_EXCL.
+B) if success, write pid of program accessing the directory
+   structure to this file.
+C) read/write the directory structure.
+D) remove file.
+

[toc] | [next] | [standalone]

#1533582

On Wed, 30 Nov 2016, Marcelo Tosatti wrote:
> 
> There is a locking problem between different applications
> reading/writing to resctrlfs directory at the same time (read the patch
> below for details).
> 
> Suggest a standard locking scheme for applications to use.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> --- Documentation/x86/intel_rdt_ui.txt.orig	2016-11-30 13:40:33.080233101 -0200
> +++ Documentation/x86/intel_rdt_ui.txt	2016-11-30 13:45:01.253703259 -0200

I can't remember that we changed the -p1 patch format to -p0 :(

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

#1533623

On Wed, Nov 30, 2016 at 01:48:10PM -0200, Marcelo Tosatti wrote:
> 
> There is a locking problem between different applications
> reading/writing to resctrlfs directory at the same time (read the patch
> below for details).
> 
> Suggest a standard locking scheme for applications to use.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> --- Documentation/x86/intel_rdt_ui.txt.orig	2016-11-30 13:40:33.080233101 -0200
> +++ Documentation/x86/intel_rdt_ui.txt	2016-11-30 13:45:01.253703259 -0200
> @@ -212,3 +212,30 @@ Finally we move core 4-7 over to the new
>  kernel and the tasks running there get 50% of the cache.
>  
>  # echo C0 > p0/cpus
> +
> +4) Locking between applications
> +
> +The allocation of an exclusive reservation
> +of L3 cache involves:
> +
> +        1. read list of cbmmasks for each directory
> +        2. find a contiguous set of bits in the global CBM bitmask
> +          that is clear in any of the directory cbmmasks
> +        3. create a new directory
> +        4. set the bits found in step 2 to the new directory "schemata"
> +           file

This is one example of why locking is needed. There are other scenarios
that need the locking as well. For example, two applications scan each
directory to find an empty/less loaded "tasks". Both of them find that
directory p1 has empty "tasks" and write their own thread ids into the
"tasks" in p1. Turns out the "tasks" in p1 will have crowded threads or
workloads. A locking can solve this race scenario too.

As a user interface document, maybe we need a generic explanation why
locking plus the example.

> +
> +If two applications attempt to allocate space race with each other
> +(if two processes execute the steps above in a interlocked fashion),
> +they can end up using the same bits of CBMMASK, which renders the
> +reservations non-exclusive but shared.
> +
> +To coordinate creation of reservations on resctrl and avoid the problem
> +above, the following locking procedure is recommended:
> +
> +A) open /var/lock/resctrl/fs.lock with O_CREAT|O_EXCL.
> +B) if success, write pid of program accessing the directory
> +   structure to this file.
> +C) read/write the directory structure.
> +D) remove file.
> +

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

#1534076

On Wed, Nov 30, 2016 at 02:05:31PM -0800, Fenghua Yu wrote:
> On Wed, Nov 30, 2016 at 01:48:10PM -0200, Marcelo Tosatti wrote:
> > 
> > There is a locking problem between different applications
> > reading/writing to resctrlfs directory at the same time (read the patch
> > below for details).
> > 
> > Suggest a standard locking scheme for applications to use.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > --- Documentation/x86/intel_rdt_ui.txt.orig	2016-11-30 13:40:33.080233101 -0200
> > +++ Documentation/x86/intel_rdt_ui.txt	2016-11-30 13:45:01.253703259 -0200
> > @@ -212,3 +212,30 @@ Finally we move core 4-7 over to the new
> >  kernel and the tasks running there get 50% of the cache.
> >  
> >  # echo C0 > p0/cpus
> > +
> > +4) Locking between applications
> > +
> > +The allocation of an exclusive reservation
> > +of L3 cache involves:
> > +
> > +        1. read list of cbmmasks for each directory
> > +        2. find a contiguous set of bits in the global CBM bitmask
> > +          that is clear in any of the directory cbmmasks
> > +        3. create a new directory
> > +        4. set the bits found in step 2 to the new directory "schemata"
> > +           file
> 
> This is one example of why locking is needed. There are other scenarios
> that need the locking as well. For example, two applications scan each
> directory to find an empty/less loaded "tasks". Both of them find that
> directory p1 has empty "tasks" and write their own thread ids into the
> "tasks" in p1. Turns out the "tasks" in p1 will have crowded threads or
> workloads. A locking can solve this race scenario too.
> 
> As a user interface document, maybe we need a generic explanation why
> locking plus the example.

Well, agreed there are other races, but in this particular example
taking the file lock does not solve the "tasks" race: the contents of
the tasks file can change in face of fork.

So i've added your suggestion but can't use this example, if you have
another one you'd like to see added, please let me know... Replying with
V2.

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

#1534479

On Wed, Nov 30, 2016 at 08:25:28PM -0200, Marcelo Tosatti wrote:
> On Wed, Nov 30, 2016 at 02:05:31PM -0800, Fenghua Yu wrote:
> > On Wed, Nov 30, 2016 at 01:48:10PM -0200, Marcelo Tosatti wrote:
> > > 
> > > There is a locking problem between different applications
> > > reading/writing to resctrlfs directory at the same time (read the patch
> > > below for details).
> > > 
> > > Suggest a standard locking scheme for applications to use.
> > > 
> > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > > 
> > > --- Documentation/x86/intel_rdt_ui.txt.orig	2016-11-30 13:40:33.080233101 -0200
> > > +++ Documentation/x86/intel_rdt_ui.txt	2016-11-30 13:45:01.253703259 -0200
> > > @@ -212,3 +212,30 @@ Finally we move core 4-7 over to the new
> > >  kernel and the tasks running there get 50% of the cache.
> > >  
> > >  # echo C0 > p0/cpus
> > > +
> > > +4) Locking between applications
> > > +
> > > +The allocation of an exclusive reservation
> > > +of L3 cache involves:
> > > +
> > > +        1. read list of cbmmasks for each directory
> > > +        2. find a contiguous set of bits in the global CBM bitmask
> > > +          that is clear in any of the directory cbmmasks
> > > +        3. create a new directory
> > > +        4. set the bits found in step 2 to the new directory "schemata"
> > > +           file
> > 
> > This is one example of why locking is needed. There are other scenarios
> > that need the locking as well. For example, two applications scan each
> > directory to find an empty/less loaded "tasks". Both of them find that
> > directory p1 has empty "tasks" and write their own thread ids into the
> > "tasks" in p1. Turns out the "tasks" in p1 will have crowded threads or
> > workloads. A locking can solve this race scenario too.
> > 
> > As a user interface document, maybe we need a generic explanation why
> > locking plus the example.
> 
> Well, agreed there are other races, but in this particular example
> taking the file lock does not solve the "tasks" race: the contents of
> the tasks file can change in face of fork.

The "tasks" example is only for resolving the race when allocating two tasks
to an empty rdtgroup. Once a task is allocated to a "tasks", the task's forked
children will automatically stay with the task unless they are moved.
Without the locking, task A and task B are allocated to the same directory
because both of them thought "tasks" in the directory is empty. Then
all forked children of both A and B will populate the directory and cause
crowded cache.

Sure the contents of the "tasks" can change in face of fork. But the race
of the allocating two groups of tasks can cause wrong decision to allocate
them at the beginning.

> 
> So i've added your suggestion but can't use this example, if you have
> another one you'd like to see added, please let me know... Replying with
> V2.

Thanks.

-Fenghua

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

#1534836 — [PATCH v2] intelrdt: resctrl: recommend locking for resctrlfs

There is a locking problem between different applications
reading/writing to resctrlfs directory at the same time (read the patch
below for details).

Suggest a standard locking scheme for applications to use.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

---
v2: Improve commentary about generality of locking problem ("Yu, Fenghua")
    Proper patch -p level (Thomas Gleixner)

diff --git a/Documentation/x86/intel_rdt_ui.txt b/Documentation/x86/intel_rdt_ui.txt
index d918d26..14b3214 100644
--- a/Documentation/x86/intel_rdt_ui.txt
+++ b/Documentation/x86/intel_rdt_ui.txt
@@ -212,3 +212,33 @@ Finally we move core 4-7 over to the new group and make sure that the
 kernel and the tasks running there get 50% of the cache.
 
 # echo C0 > p0/cpus
+
+4) Locking between applications
+
+Certain operations on the resctrl filesystem, composed of
+read / writes to multiple files, must be atomic.
+
+As an example, the allocation of an exclusive reservation
+of L3 cache involves:
+
+        1. read list of cbmmasks for each directory
+        2. find a contiguous set of bits in the global CBM bitmask
+           that is clear in any of the directory cbmmasks
+        3. create a new directory
+        4. set the bits found in step 2 to the new directory "schemata"
+           file
+
+If two applications attempting to allocate space race with each other
+(if two processes execute the steps above in a interlocked fashion),
+they can end up using the same bits of CBMMASK, which renders the
+reservations non-exclusive but shared.
+
+To coordinate atomic operations on resctrl and avoid the problem
+above, the following locking procedure is recommended:
+
+A) open /var/lock/resctrl/fs.lock with O_CREAT|O_EXCL.
+B) if success, write pid of program accessing the directory
+   structure to this file.
+C) read/write the directory structure.
+D) remove file.
+

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

#1534850 — Re: [PATCH v2] intelrdt: resctrl: recommend locking for resctrlfs

On Thu, 1 Dec 2016, Marcelo Tosatti wrote:
> 
> There is a locking problem between different applications
> reading/writing to resctrlfs directory at the same time (read the patch
> below for details).
> 
> Suggest a standard locking scheme for applications to use.

....

> +To coordinate atomic operations on resctrl and avoid the problem
> +above, the following locking procedure is recommended:
> +
> +A) open /var/lock/resctrl/fs.lock with O_CREAT|O_EXCL.
> +B) if success, write pid of program accessing the directory
> +   structure to this file.
> +C) read/write the directory structure.
> +D) remove file.

What's wrong with using flock, which works from shell scripts as well?

Thanks,

	tglx

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

#1535250 — Re: [PATCH v2] intelrdt: resctrl: recommend locking for resctrlfs

On Fri, Dec 02, 2016 at 12:20:29PM +0100, Thomas Gleixner wrote:
> On Thu, 1 Dec 2016, Marcelo Tosatti wrote:
> > 
> > There is a locking problem between different applications
> > reading/writing to resctrlfs directory at the same time (read the patch
> > below for details).
> > 
> > Suggest a standard locking scheme for applications to use.
> 
> ....
> 
> > +To coordinate atomic operations on resctrl and avoid the problem
> > +above, the following locking procedure is recommended:
> > +
> > +A) open /var/lock/resctrl/fs.lock with O_CREAT|O_EXCL.
> > +B) if success, write pid of program accessing the directory
> > +   structure to this file.
> > +C) read/write the directory structure.
> > +D) remove file.
> 
> What's wrong with using flock, which works from shell scripts as well?
> 
> Thanks,
> 
> 	tglx

Hi Thomas,

Nothing wrong with it... I'm just copying the behaviour of other
programs.

Actually, using flock(2) allows one to use LOCK_SH for readers and 
this allows consistent writer/reader behaviour (say, a reader
won't see a partially written directory).

NAME
       flock - apply or remove an advisory lock on an open file

SYNOPSIS
       #include <sys/file.h>

       int flock(int fd, int operation);

DESCRIPTION
       Apply  or remove an advisory lock on the open file specified by
fd.  The argument operation is one of
       the following:

           LOCK_SH  Place a shared lock.  More than one process may hold
a shared lock for a given file at a
                    given time.

           LOCK_EX  Place  an  exclusive lock.  Only one process may
hold an exclusive lock for a given file
                    at a given time.

           LOCK_UN  Remove an existing lock held by this process.

--- 

So the procedure would be:

    /var/lock/resctrl/fs.lock created previously in the filesystem.

WRITE LOCK:

A) Take flock(EXCLUSIVE) on /var/lock/resctrl/fs.lock
B) If success, write pid of the program to the file.
C) read/write the directory structure.
D) funlock

READ LOCK:

A) Take flock(SHARED) on /var/lock/resctrl/fs.lock
B) If success read the directory structure.
C) funlock


How about that?

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

#1539219 — Re: [PATCH v2] intelrdt: resctrl: recommend locking for resctrlfs

On Fri, 2 Dec 2016, Marcelo Tosatti wrote:
> Actually, using flock(2) allows one to use LOCK_SH for readers and 
> this allows consistent writer/reader behaviour (say, a reader
> won't see a partially written directory).

Indeed.

> So the procedure would be:
> 
>     /var/lock/resctrl/fs.lock created previously in the filesystem.
> 
> WRITE LOCK:
> 
> A) Take flock(EXCLUSIVE) on /var/lock/resctrl/fs.lock
> B) If success, write pid of the program to the file.
> C) read/write the directory structure.
> D) funlock
> 
> READ LOCK:
> 
> A) Take flock(SHARED) on /var/lock/resctrl/fs.lock
> B) If success read the directory structure.
> C) funlock
> 
> 
> How about that?

Looks sane. Adding a small example in C and [ba]sh would be nice.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web