efi_pstore: question about how to remove create_sysfs_entry() from a write callback.

efi_pstore: question about how to remove create_sysfs_entry() from a write callback.

[Seiji Aguchi]

Hi,

I'm sending an email to discuss how to remove create_sysfs_entry() from a write callback.

[Problem]

Current efi_pstore creates sysfs entries ,which enable users to access to NVRAM, in a write callback.
If a kernel panic happens in interrupt contexts, pstore may fail because it could sleep due to dynamic 
memory allocations during creating sysfs entries.

To resolve the problem above, my goal here is removing create_sysfs_entry() from a write callback.

[Ideas]

 (1) Introduce a workqueue updating sysfs entries

     To remove create_sysfs_entry() from a write callback,
     It seems to be possible if efi_pstore updates its sysfs files 
     by scanning existing entries in NVRAM with a GetNextVariable()
     in a workqueue.
     

     I created a prototype patch based on an idea above but can't avoid a race 
     between SetVariable() in a write callback and GetNextVariable() in a workqueue.
     It is not guaranteed by EFI specification.

     EFI 2.3.1 specification, page 217. 
     <snip>
     Calls to SetVariable() between calls to
     GetNextVariableName() may produce unpredictable results.
     <snip>


 (2) Don't support sysfs entries in efi_pstore.

     Another idea is _not_ updating sysfs entries at all in efi_pstore.
     This can avoid a race SetVariable() and GetNextVariable().

     write callback
       - simply write a new entry with SetVariable().
         - This fits a discussion about holding multiple logs in a thread below.
           http://marc.info/?l=linux-kernel&m=134316268011854&w=2 

     erase callback
       - simply erase an existing entry with SetVariable().

     read callback
       - Scaning existing entries with GetNextVariable().
         We can avoid a race between GetNextVariable() in a read callback 
         and SetVariable() in a write/erase callback by protecting them with efi_lock.

 IMO, idea (2) is reasonable because we already have an interface, /dev/pstore, which users can access
 to NVRAM and we don't need to support multiple user interfaces.

Any comments are welcome.

Seiji

Re: efi_pstore: question about how to remove create_sysfs_entry() from a write callback.

[Mike Waychison]

On Tue, Aug 14, 2012 at 9:05 AM, Seiji Aguchi <seiji.aguchi@hds.com> wrote:
> Hi,
>
> I'm sending an email to discuss how to remove create_sysfs_entry() from a write callback.
>
> [Problem]
>
> Current efi_pstore creates sysfs entries ,which enable users to access to NVRAM, in a write callback.
> If a kernel panic happens in interrupt contexts, pstore may fail because it could sleep due to dynamic
> memory allocations during creating sysfs entries.
>
> To resolve the problem above, my goal here is removing create_sysfs_entry() from a write callback.
>
> [Ideas]
>
>  (1) Introduce a workqueue updating sysfs entries
>
>      To remove create_sysfs_entry() from a write callback,
>      It seems to be possible if efi_pstore updates its sysfs files
>      by scanning existing entries in NVRAM with a GetNextVariable()
>      in a workqueue.
>
>
>      I created a prototype patch based on an idea above but can't avoid a race
>      between SetVariable() in a write callback and GetNextVariable() in a workqueue.
>      It is not guaranteed by EFI specification.
>
>      EFI 2.3.1 specification, page 217.
>      <snip>
>      Calls to SetVariable() between calls to
>      GetNextVariableName() may produce unpredictable results.
>      <snip>

Can we not serialize this with &efivars->lock if it is updated to
disable interrupts?  A loop in the workqueue that locks, iterates
through ->get_next_variable, and compares against searches in
efivars->list should work, no?

>
>
>  (2) Don't support sysfs entries in efi_pstore.
>
>      Another idea is _not_ updating sysfs entries at all in efi_pstore.
>      This can avoid a race SetVariable() and GetNextVariable().
>
>      write callback
>        - simply write a new entry with SetVariable().
>          - This fits a discussion about holding multiple logs in a thread below.
>            http://marc.info/?l=linux-kernel&m=134316268011854&w=2
>
>      erase callback
>        - simply erase an existing entry with SetVariable().
>
>      read callback
>        - Scaning existing entries with GetNextVariable().
>          We can avoid a race between GetNextVariable() in a read callback
>          and SetVariable() in a write/erase callback by protecting them with efi_lock.
>
>  IMO, idea (2) is reasonable because we already have an interface, /dev/pstore, which users can access
>  to NVRAM and we don't need to support multiple user interfaces.
>
> Any comments are welcome.
>
> Seiji

RE: efi_pstore: question about how to remove create_sysfs_entry() from a write callback.

[Seiji Aguchi]

> Can we not serialize this with &efivars->lock if it is updated to disable interrupts?  A loop in the workqueue that locks, iterates through
> ->get_next_variable, and compares against searches in
> efivars->list should work, no?

If my understanding is correct, your pseudo code is as follows.

spin_lock_irqsave(&efivars_lock);

do {
	get_next_variable();
	if (found new entry)
 		efivars_create_sysfs_entries();

} while()

spin_lock_irqrestore(&efivars_lock);

But, memory is allocated dynamically with kzalloc() in efivars_create_sysfs_entries().
I don't want to allocate memory  while holding spin_lock.

Please let me know if I'm missing something.

Seiji 

RE: efi_pstore: question about how to remove create_sysfs_entry() from a write callback.

[Seiji Aguchi]

Mike,

Previous pseudo code was not correct.
More precisely, it is as follows.
But, we still need to alloc memory dynamically in the part of <Save information about new entry>
because a workqueue can't know how many new entries in advance.

 spin_lock_irqsave(&efivars_lock);
 
 do {
 	get_next_variable();
 	if (found new entry)
  		<Save information about new entry>
 
 } while()
 
 spin_lock_irqrestore(&efivars_lock);

  efivars_create_sysfs_entries();

Seiji

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Seiji Aguchi
> Sent: Tuesday, August 14, 2012 2:52 PM
> To: Mike Waychison
> Cc: linux-kernel@vger.kernel.org; Luck, Tony (tony.luck@intel.com); Matthew Garrett (mjg@redhat.com); dzickus@redhat.com; dle-
> develop@lists.sourceforge.net; Satoru Moriya
> Subject: RE: efi_pstore: question about how to remove create_sysfs_entry() from a write callback.
> 
> > Can we not serialize this with &efivars->lock if it is updated to
> > disable interrupts?  A loop in the workqueue that locks, iterates
> > through
> > ->get_next_variable, and compares against searches in
> > efivars->list should work, no?
> 
> If my understanding is correct, your pseudo code is as follows.
> 
> spin_lock_irqsave(&efivars_lock);
> 
> do {
> 	get_next_variable();
> 	if (found new entry)
>  		efivars_create_sysfs_entries();
> 
> } while()
> 
> spin_lock_irqrestore(&efivars_lock);
> 
> But, memory is allocated dynamically with kzalloc() in efivars_create_sysfs_entries().
> I don't want to allocate memory  while holding spin_lock.
> 
> Please let me know if I'm missing something.
> 
> Seiji
> --
> 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/

Re: efi_pstore: question about how to remove create_sysfs_entry() from a write callback.

[Mike Waychison]

On Tue, Aug 14, 2012 at 12:29 PM, Seiji Aguchi <seiji.aguchi@hds.com> wrote:
> Mike,
>
> Previous pseudo code was not correct.
> More precisely, it is as follows.
> But, we still need to alloc memory dynamically in the part of <Save information about new entry>
> because a workqueue can't know how many new entries in advance.
>
>  spin_lock_irqsave(&efivars_lock);
>
>  do {
>         get_next_variable();
>         if (found new entry)
>                 <Save information about new entry>

If the work item is changed to instead do a loop, storage for this may
be saved elsewhere, either the stack (though 1KiB is sorta pushing it)
or on the heap.

>
>  } while()
>
>  spin_lock_irqrestore(&efivars_lock);
>
>   efivars_create_sysfs_entries();

Something like this (pseudo) may work:

/* Loop until we have all entries in efivars. */
while (1) {
  variable_name = kzalloc(1024, GFP_KERNEL );
  spin_lock_irqsave(&efivars->lock);
  bool found = false;
  while (1) {
     ret = ops->get_next_variable(variable_name)
     if (ret == EFI_NOT_FOUND)
        break;
     if (!variable_is_already_present(variable, &efivars->list) {
       found = true;
       break;
     }
  }
  if (!found) {
    kfree(variable_name);
    break;
  }
  <Register in sysfs>
}


If we are calling into pstore_info ops though from interrupt context,
it seems we'll need to fix the immediate problem of updating the
locking throughout to be interrupt safe.


>
> Seiji
>
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Seiji Aguchi
>> Sent: Tuesday, August 14, 2012 2:52 PM
>> To: Mike Waychison
>> Cc: linux-kernel@vger.kernel.org; Luck, Tony (tony.luck@intel.com); Matthew Garrett (mjg@redhat.com); dzickus@redhat.com; dle-
>> develop@lists.sourceforge.net; Satoru Moriya
>> Subject: RE: efi_pstore: question about how to remove create_sysfs_entry() from a write callback.
>>
>> > Can we not serialize this with &efivars->lock if it is updated to
>> > disable interrupts?  A loop in the workqueue that locks, iterates
>> > through
>> > ->get_next_variable, and compares against searches in
>> > efivars->list should work, no?
>>
>> If my understanding is correct, your pseudo code is as follows.
>>
>> spin_lock_irqsave(&efivars_lock);
>>
>> do {
>>       get_next_variable();
>>       if (found new entry)
>>               efivars_create_sysfs_entries();
>>
>> } while()
>>
>> spin_lock_irqrestore(&efivars_lock);
>>
>> But, memory is allocated dynamically with kzalloc() in efivars_create_sysfs_entries().
>> I don't want to allocate memory  while holding spin_lock.
>>
>> Please let me know if I'm missing something.
>>
>> Seiji
>> --
>> 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/

RE: efi_pstore: question about how to remove create_sysfs_entry() from a write callback.

[Seiji Aguchi]

> 
> Something like this (pseudo) may work:
> 
> /* Loop until we have all entries in efivars. */ while (1) {
>   variable_name = kzalloc(1024, GFP_KERNEL );
>   spin_lock_irqsave(&efivars->lock);
>   bool found = false;
>   while (1) {
>      ret = ops->get_next_variable(variable_name)
>      if (ret == EFI_NOT_FOUND)
>         break;
>      if (!variable_is_already_present(variable, &efivars->list) {
>        found = true;
>        break;
>      }
>   }
>   if (!found) {
>     kfree(variable_name);
>     break;
>   }
>   <Register in sysfs>
> }
> 

Thanks. It seems to work.
I will create a patch based on your pseudo code above.

> 
> If we are calling into pstore_info ops though from interrupt context, it seems we'll need to fix the immediate problem of updating the
> locking throughout to be interrupt safe.
> 

I agree with you. It should be fixing.  I will update the locking to be interrupt safe as well.

Seiji