Liu, Jinsong wrote:
> Just notice your reply (so quick :)
>
> Agree and will update later, except 1 concern below.
>
> Konrad Rzeszutek Wilk wrote:
>>>
>>> Hmm, it's good if it's convenient to do it automatically via
>>> dev->release. However, dev container (pcpu) would be free at some
>>> other error cases, so I prefer do it 'manually'.
>>
>> You could also call pcpu_release(..) to do it manually.
>>
>
> that means kfree(pcpu) would be done twice at some error cases, do
> you think it really good?
>
Ping.
I think error recovery should be kept inside error logic level itself, if try to recover upper level error would bring trouble.
In our example, there are 2 logic levels:
pcpu level (as container), and dev level (subfield used for sys)
dev->release should only recover error occurred at dev/sys level, and the pcpu error should be recovered at pcpu level.
If dev->release try to recover its container pcpu level error, like list_del/kfree(pcpu), it would make confusing. i.e., considering pcpu_sys_create(), 2 error cases:
device_register fail, and device_create_file fail --> how can the caller decide kfree(pcpu) or not?
So how about recover pcpu error manually and explicitly?
Thanks,
Jinsong-
On Fri, May 11, 2012 at 01:12:13PM +0000, Liu, Jinsong wrote:
> Liu, Jinsong wrote:
> > Just notice your reply (so quick :)
> >
> > Agree and will update later, except 1 concern below.
> >
> > Konrad Rzeszutek Wilk wrote:
> >>>
> >>> Hmm, it's good if it's convenient to do it automatically via
> >>> dev->release. However, dev container (pcpu) would be free at some
> >>> other error cases, so I prefer do it 'manually'.
> >>
> >> You could also call pcpu_release(..) to do it manually.
> >>
> >
> > that means kfree(pcpu) would be done twice at some error cases, do
> > you think it really good?
> >
>
> Ping.
>
> I think error recovery should be kept inside error logic level itself, if try to recover upper level error would bring trouble.
>
> In our example, there are 2 logic levels:
> pcpu level (as container), and dev level (subfield used for sys)
So you need to untangle free_pcpu from doing both. Meaning one does the
SysFS and the other deals with free-ing the structure and removing itself
from the list.
> dev->release should only recover error occurred at dev/sys level, and the pcpu error should be recovered at pcpu level.
>
> If dev->release try to recover its container pcpu level error, like list_del/kfree(pcpu), it would make confusing. i.e., considering pcpu_sys_create(), 2 error cases:
> device_register fail, and device_create_file fail --> how can the caller decide kfree(pcpu) or not?
Then you should free it manually. But you can do this by a wrapper
function:
__pcpu_release(..) {
..
/* Does the removing itself from the list and kfree the pcpu */
}
pcpu_release(..) {
struct pcpcu *p= container_of(..)
__pcpu_release(p);
}
dev->release = &pcpu_release;
>
> So how about recover pcpu error manually and explicitly?
>
> Thanks,
> Jinsong
Konrad Rzeszutek Wilk wrote:
> On Fri, May 11, 2012 at 01:12:13PM +0000, Liu, Jinsong wrote:
>> Liu, Jinsong wrote:
>>> Just notice your reply (so quick :)
>>>
>>> Agree and will update later, except 1 concern below.
>>>
>>> Konrad Rzeszutek Wilk wrote:
>>>>>
>>>>> Hmm, it's good if it's convenient to do it automatically via
>>>>> dev->release. However, dev container (pcpu) would be free at some
>>>>> other error cases, so I prefer do it 'manually'.
>>>>
>>>> You could also call pcpu_release(..) to do it manually.
>>>>
>>>
>>> that means kfree(pcpu) would be done twice at some error cases, do
>>> you think it really good?
>>>
>>
>> Ping.
>>
>> I think error recovery should be kept inside error logic level
>> itself, if try to recover upper level error would bring trouble.
>>
>> In our example, there are 2 logic levels:
>> pcpu level (as container), and dev level (subfield used for sys)
>
> So you need to untangle free_pcpu from doing both. Meaning one does
> the SysFS and the other deals with free-ing the structure and
> removing itself from the list.
>
free_cpu is very samll, just consist of the 2 parts your said:
* pcpu_sys_remove() deal with sysfs
* list_del/kfree(pcpu) deal with pcpu
>
>> dev->release should only recover error occurred at dev/sys level,
>> and the pcpu error should be recovered at pcpu level.
>>
>> If dev->release try to recover its container pcpu level error, like
>> list_del/kfree(pcpu), it would make confusing. i.e., considering
>> pcpu_sys_create(), 2 error cases: device_register fail, and
>> device_create_file fail --> how can the caller decide kfree(pcpu) or
>> not?
>
> Then you should free it manually. But you can do this by a wrapper
> function:
>
> __pcpu_release(..) {
> ..
> /* Does the removing itself from the list and kfree the pcpu */
> }
> pcpu_release(..) {
> struct pcpcu *p= container_of(..)
> __pcpu_release(p);
> }
>
> dev->release = &pcpu_release;
>
Too weird way. If we want to release dev itself it's good to use dev->release, but for pcpu I doubt it.
(consider the example I gave --> why we create issue (it maybe solved in weird method I agree), just for using dev->release?)
In kernel many dev->release keep NULL.
An example of using dev->release is cpu/mcheck/mce.c --> mce_device_release(), it *just* deal dev itself.
Thanks,
Jinsong
On Fri, May 11, 2012 at 06:04:21PM +0000, Liu, Jinsong wrote:
> Konrad Rzeszutek Wilk wrote:
> > On Fri, May 11, 2012 at 01:12:13PM +0000, Liu, Jinsong wrote:
> >> Liu, Jinsong wrote:
> >>> Just notice your reply (so quick :)
> >>>
> >>> Agree and will update later, except 1 concern below.
> >>>
> >>> Konrad Rzeszutek Wilk wrote:
> >>>>>
> >>>>> Hmm, it's good if it's convenient to do it automatically via
> >>>>> dev->release. However, dev container (pcpu) would be free at some
> >>>>> other error cases, so I prefer do it 'manually'.
> >>>>
> >>>> You could also call pcpu_release(..) to do it manually.
> >>>>
> >>>
> >>> that means kfree(pcpu) would be done twice at some error cases, do
> >>> you think it really good?
> >>>
> >>
> >> Ping.
> >>
> >> I think error recovery should be kept inside error logic level
> >> itself, if try to recover upper level error would bring trouble.
> >>
> >> In our example, there are 2 logic levels:
> >> pcpu level (as container), and dev level (subfield used for sys)
> >
> > So you need to untangle free_pcpu from doing both. Meaning one does
> > the SysFS and the other deals with free-ing the structure and
> > removing itself from the list.
> >
>
> free_cpu is very samll, just consist of the 2 parts your said:
> * pcpu_sys_remove() deal with sysfs
> * list_del/kfree(pcpu) deal with pcpu
>
> >
> >> dev->release should only recover error occurred at dev/sys level,
> >> and the pcpu error should be recovered at pcpu level.
> >>
> >> If dev->release try to recover its container pcpu level error, like
> >> list_del/kfree(pcpu), it would make confusing. i.e., considering
> >> pcpu_sys_create(), 2 error cases: device_register fail, and
> >> device_create_file fail --> how can the caller decide kfree(pcpu) or
> >> not?
> >
> > Then you should free it manually. But you can do this by a wrapper
> > function:
> >
> > __pcpu_release(..) {
> > ..
> > /* Does the removing itself from the list and kfree the pcpu */
> > }
> > pcpu_release(..) {
> > struct pcpcu *p= container_of(..)
> > __pcpu_release(p);
> > }
> >
> > dev->release = &pcpu_release;
> >
>
> Too weird way. If we want to release dev itself it's good to use dev->release, but for pcpu I doubt it.
> (consider the example I gave --> why we create issue (it maybe solved in weird method I agree), just for using dev->release?)
>
> In kernel many dev->release keep NULL.
> An example of using dev->release is cpu/mcheck/mce.c --> mce_device_release(), it *just* deal dev itself.
OK? I am not sure what are we arguing here anymore?
I think using 'kfree(pcpu)' on the error paths (as long as it is
done before device_register) is OK. I think that seperating
the SysFS deletion from the pcpu deletion should be done to
avoid races. Perhaps the SysFS deletion function should also
remove the pcpu from the list.
Konrad Rzeszutek Wilk wrote:
> On Fri, May 11, 2012 at 06:04:21PM +0000, Liu, Jinsong wrote:
>> Konrad Rzeszutek Wilk wrote:
>>> On Fri, May 11, 2012 at 01:12:13PM +0000, Liu, Jinsong wrote:
>>>> Liu, Jinsong wrote:
>>>>> Just notice your reply (so quick :)
>>>>>
>>>>> Agree and will update later, except 1 concern below.
>>>>>
>>>>> Konrad Rzeszutek Wilk wrote:
>>>>>>>
>>>>>>> Hmm, it's good if it's convenient to do it automatically via
>>>>>>> dev->release. However, dev container (pcpu) would be free at
>>>>>>> some other error cases, so I prefer do it 'manually'.
>>>>>>
>>>>>> You could also call pcpu_release(..) to do it manually.
>>>>>>
>>>>>
>>>>> that means kfree(pcpu) would be done twice at some error cases,
>>>>> do you think it really good?
>>>>>
>>>>
>>>> Ping.
>>>>
>>>> I think error recovery should be kept inside error logic level
>>>> itself, if try to recover upper level error would bring trouble.
>>>>
>>>> In our example, there are 2 logic levels:
>>>> pcpu level (as container), and dev level (subfield used for sys)
>>>
>>> So you need to untangle free_pcpu from doing both. Meaning one does
>>> the SysFS and the other deals with free-ing the structure and
>>> removing itself from the list.
>>>
>>
>> free_cpu is very samll, just consist of the 2 parts your said:
>> * pcpu_sys_remove() deal with sysfs
>> * list_del/kfree(pcpu) deal with pcpu
>>
>>>
>>>> dev->release should only recover error occurred at dev/sys level,
>>>> and the pcpu error should be recovered at pcpu level.
>>>>
>>>> If dev->release try to recover its container pcpu level error, like
>>>> list_del/kfree(pcpu), it would make confusing. i.e., considering
>>>> pcpu_sys_create(), 2 error cases: device_register fail, and
>>>> device_create_file fail --> how can the caller decide kfree(pcpu)
>>>> or not?
>>>
>>> Then you should free it manually. But you can do this by a wrapper
>>> function:
>>>
>>> __pcpu_release(..) {
>>> ..
>>> /* Does the removing itself from the list and kfree the pcpu */ }
>>> pcpu_release(..) {
>>> struct pcpcu *p= container_of(..)
>>> __pcpu_release(p);
>>> }
>>>
>>> dev->release = &pcpu_release;
>>>
>>
>> Too weird way. If we want to release dev itself it's good to use
>> dev->release, but for pcpu I doubt it. (consider the example I gave
>> --> why we create issue (it maybe solved in weird method I agree),
>> just for using dev->release?)
>>
>> In kernel many dev->release keep NULL.
>> An example of using dev->release is cpu/mcheck/mce.c -->
>> mce_device_release(), it *just* deal dev itself.
>
> OK? I am not sure what are we arguing here anymore?
> I think using 'kfree(pcpu)' on the error paths (as long as it is
> done before device_register) is OK. I think that seperating
> the SysFS deletion from the pcpu deletion should be done to
> avoid races. Perhaps the SysFS deletion function should also
> remove the pcpu from the list.
How about static array pcpu[NR_CPUS]?
It seems solve all issues we argued :)
Thanks,
Jinsong-
On Fri, May 11, 2012 at 08:31:15PM +0000, Liu, Jinsong wrote:
> Konrad Rzeszutek Wilk wrote:
> > On Fri, May 11, 2012 at 06:04:21PM +0000, Liu, Jinsong wrote:
> >> Konrad Rzeszutek Wilk wrote:
> >>> On Fri, May 11, 2012 at 01:12:13PM +0000, Liu, Jinsong wrote:
> >>>> Liu, Jinsong wrote:
> >>>>> Just notice your reply (so quick :)
> >>>>>
> >>>>> Agree and will update later, except 1 concern below.
> >>>>>
> >>>>> Konrad Rzeszutek Wilk wrote:
> >>>>>>>
> >>>>>>> Hmm, it's good if it's convenient to do it automatically via
> >>>>>>> dev->release. However, dev container (pcpu) would be free at
> >>>>>>> some other error cases, so I prefer do it 'manually'.
> >>>>>>
> >>>>>> You could also call pcpu_release(..) to do it manually.
> >>>>>>
> >>>>>
> >>>>> that means kfree(pcpu) would be done twice at some error cases,
> >>>>> do you think it really good?
> >>>>>
> >>>>
> >>>> Ping.
> >>>>
> >>>> I think error recovery should be kept inside error logic level
> >>>> itself, if try to recover upper level error would bring trouble.
> >>>>
> >>>> In our example, there are 2 logic levels:
> >>>> pcpu level (as container), and dev level (subfield used for sys)
> >>>
> >>> So you need to untangle free_pcpu from doing both. Meaning one does
> >>> the SysFS and the other deals with free-ing the structure and
> >>> removing itself from the list.
> >>>
> >>
> >> free_cpu is very samll, just consist of the 2 parts your said:
> >> * pcpu_sys_remove() deal with sysfs
> >> * list_del/kfree(pcpu) deal with pcpu
> >>
> >>>
> >>>> dev->release should only recover error occurred at dev/sys level,
> >>>> and the pcpu error should be recovered at pcpu level.
> >>>>
> >>>> If dev->release try to recover its container pcpu level error, like
> >>>> list_del/kfree(pcpu), it would make confusing. i.e., considering
> >>>> pcpu_sys_create(), 2 error cases: device_register fail, and
> >>>> device_create_file fail --> how can the caller decide kfree(pcpu)
> >>>> or not?
> >>>
> >>> Then you should free it manually. But you can do this by a wrapper
> >>> function:
> >>>
> >>> __pcpu_release(..) {
> >>> ..
> >>> /* Does the removing itself from the list and kfree the pcpu */ }
> >>> pcpu_release(..) {
> >>> struct pcpcu *p= container_of(..)
> >>> __pcpu_release(p);
> >>> }
> >>>
> >>> dev->release = &pcpu_release;
> >>>
> >>
> >> Too weird way. If we want to release dev itself it's good to use
> >> dev->release, but for pcpu I doubt it. (consider the example I gave
> >> --> why we create issue (it maybe solved in weird method I agree),
> >> just for using dev->release?)
> >>
> >> In kernel many dev->release keep NULL.
> >> An example of using dev->release is cpu/mcheck/mce.c -->
> >> mce_device_release(), it *just* deal dev itself.
> >
> > OK? I am not sure what are we arguing here anymore?
> > I think using 'kfree(pcpu)' on the error paths (as long as it is
> > done before device_register) is OK. I think that seperating
> > the SysFS deletion from the pcpu deletion should be done to
> > avoid races. Perhaps the SysFS deletion function should also
> > remove the pcpu from the list.
>
> How about static array pcpu[NR_CPUS]?
> It seems solve all issues we argued :)
Ugh. That could mean a structure of more than 4K of items.
Lets stick with making it dynamic.