Re: [PATCH 4/5] sched/arinc653: Reorganize function definition order
On 9/17/2020 10:16 AM, Dario Faggioli wrote:
>On Thu, 2020-09-17 at 10:12 +0200, Jan Beulich wrote:
>> On 16.09.2020 20:18, Jeff Kubascik wrote:
>>> @@ -517,27 +516,35 @@ static const struct scheduler
>>> sched_arinc653_def = {
>>> .sched_id = XEN_SCHEDULER_ARINC653,
>>> .sched_data = NULL,
>>>
>>> +.global_init= NULL,
>>> .init = a653sched_init,
>>> .deinit = a653sched_deinit,
>>>
>>> -.free_udata = a653sched_free_udata,
>>> -.alloc_udata= a653sched_alloc_udata,
>>> +.alloc_pdata= NULL,
>>> +.switch_sched = a653sched_switch_sched,
>>> +.deinit_pdata = NULL,
>>> +.free_pdata = NULL,
>>>
>>> +.alloc_domdata = NULL,
>>> +.free_domdata = NULL,
>>> +
>>> +.alloc_udata= a653sched_alloc_udata,
>>> .insert_unit= NULL,
>>> .remove_unit= NULL,
>>> +.free_udata = a653sched_free_udata,
>>>
>>> .sleep = a653sched_unit_sleep,
>>> .wake = a653sched_unit_wake,
>>> .yield = NULL,
>>> .context_saved = NULL,
>>>
>>> -.do_schedule= a653sched_do_schedule,
>>> -
>>> .pick_resource = a653sched_pick_resource,
>>> +.migrate= NULL,
>>>
>>> -.switch_sched = a653sched_switch_sched,
>>> +.do_schedule= a653sched_do_schedule,
>>>
>>> .adjust = NULL,
>>> +.adjust_affinity= NULL,
>>
>> Adding all these not really needed NULL initializers looks to rather
>> move
>> this scheduler away from all the others.
>>
>Agreed, no need for more "= NULL". On the contrary, the ones that are
>there should go away.
Agreed x2, I'll remove the "= NULL" lines.
>About this:
>
>> (Oddly enough all of them
>> explicitly set .sched_data to NULL - for whatever reason.)
>>
>Yes, we decided to keep it like that, back then. I think now it would
>be ok for it to go away too.
>
>So, Jeff, feel free to zap it with this patch or series. Or I can send
>a patch to zap all of them, as you wish.
I'll remove the ".sched_data = NULL" line above, but my scope is limited to the
ARINC653 scheduler, so I won't be able to work on this.
-Jeff
Re: [PATCH 4/5] sched/arinc653: Reorganize function definition order
On 9/17/2020 10:17 AM, Andrew Cooper wrote:
> On 17/09/2020 09:12, Jan Beulich wrote:
>> On 16.09.2020 20:18, Jeff Kubascik wrote:
>>> @@ -517,27 +516,35 @@ static const struct scheduler sched_arinc653_def = {
>>> .sched_id = XEN_SCHEDULER_ARINC653,
>>> .sched_data = NULL,
>>>
>>> +.global_init= NULL,
>>> .init = a653sched_init,
>>> .deinit = a653sched_deinit,
>>>
>>> -.free_udata = a653sched_free_udata,
>>> -.alloc_udata= a653sched_alloc_udata,
>>> +.alloc_pdata= NULL,
>>> +.switch_sched = a653sched_switch_sched,
>>> +.deinit_pdata = NULL,
>>> +.free_pdata = NULL,
>>>
>>> +.alloc_domdata = NULL,
>>> +.free_domdata = NULL,
>>> +
>>> +.alloc_udata= a653sched_alloc_udata,
>>> .insert_unit= NULL,
>>> .remove_unit= NULL,
>>> +.free_udata = a653sched_free_udata,
>>>
>>> .sleep = a653sched_unit_sleep,
>>> .wake = a653sched_unit_wake,
>>> .yield = NULL,
>>> .context_saved = NULL,
>>>
>>> -.do_schedule= a653sched_do_schedule,
>>> -
>>> .pick_resource = a653sched_pick_resource,
>>> +.migrate= NULL,
>>>
>>> -.switch_sched = a653sched_switch_sched,
>>> +.do_schedule= a653sched_do_schedule,
>>>
>>> .adjust = NULL,
>>> +.adjust_affinity= NULL,
>> Adding all these not really needed NULL initializers looks to rather move
>> this scheduler away from all the others. (Oddly enough all of them
>> explicitly set .sched_data to NULL - for whatever reason.)
>
> The "= NULL" is totally redundant, because the compiler will do that for
> you.
I agree with this. This originally was intended to lay the groundwork for patch
#5, but looking at it again, was confusing and unnecessary. I'll remove the =
NULL lines.
> The last user of .sched_data was dropped by 9c95227160. Conceptually,
> it is a layering violation (it prevents different cpupools being
> properly independent), so I'd recommend just dropping the field entirely.
I'll remove .sched_data above.
-Jeff
Re: [PATCH 4/5] sched/arinc653: Reorganize function definition order
On 9/17/2020 10:17 AM, Andrew Cooper wrote:
> On 17/09/2020 09:12, Jan Beulich wrote:
>> On 16.09.2020 20:18, Jeff Kubascik wrote:
>>> @@ -517,27 +516,35 @@ static const struct scheduler sched_arinc653_def = {
>>> .sched_id = XEN_SCHEDULER_ARINC653,
>>> .sched_data = NULL,
>>>
>>> +.global_init= NULL,
>>> .init = a653sched_init,
>>> .deinit = a653sched_deinit,
>>>
>>> -.free_udata = a653sched_free_udata,
>>> -.alloc_udata= a653sched_alloc_udata,
>>> +.alloc_pdata= NULL,
>>> +.switch_sched = a653sched_switch_sched,
>>> +.deinit_pdata = NULL,
>>> +.free_pdata = NULL,
>>>
>>> +.alloc_domdata = NULL,
>>> +.free_domdata = NULL,
>>> +
>>> +.alloc_udata= a653sched_alloc_udata,
>>> .insert_unit= NULL,
>>> .remove_unit= NULL,
>>> +.free_udata = a653sched_free_udata,
>>>
>>> .sleep = a653sched_unit_sleep,
>>> .wake = a653sched_unit_wake,
>>> .yield = NULL,
>>> .context_saved = NULL,
>>>
>>> -.do_schedule= a653sched_do_schedule,
>>> -
>>> .pick_resource = a653sched_pick_resource,
>>> +.migrate= NULL,
>>>
>>> -.switch_sched = a653sched_switch_sched,
>>> +.do_schedule= a653sched_do_schedule,
>>>
>>> .adjust = NULL,
>>> +.adjust_affinity= NULL,
>> Adding all these not really needed NULL initializers looks to rather move
>> this scheduler away from all the others. (Oddly enough all of them
>> explicitly set .sched_data to NULL - for whatever reason.)
>
> The "= NULL" is totally redundant, because the compiler will do that for
> you.
I agree with this. This originally was intended to lay the groundwork for patch
#5, but looking at it again, was confusing and unnecessary. I'll remove the =
NULL lines.
> The last user of .sched_data was dropped by 9c95227160. Conceptually,
> it is a layering violation (it prevents different cpupools being
> properly independent), so I'd recommend just dropping the field entirely.
I'll remove .sched_data above.
-Jeff
Re: [PATCH 4/5] sched/arinc653: Reorganize function definition order
On 17/09/2020 09:12, Jan Beulich wrote:
> On 16.09.2020 20:18, Jeff Kubascik wrote:
>> @@ -517,27 +516,35 @@ static const struct scheduler sched_arinc653_def = {
>> .sched_id = XEN_SCHEDULER_ARINC653,
>> .sched_data = NULL,
>>
>> +.global_init= NULL,
>> .init = a653sched_init,
>> .deinit = a653sched_deinit,
>>
>> -.free_udata = a653sched_free_udata,
>> -.alloc_udata= a653sched_alloc_udata,
>> +.alloc_pdata= NULL,
>> +.switch_sched = a653sched_switch_sched,
>> +.deinit_pdata = NULL,
>> +.free_pdata = NULL,
>>
>> +.alloc_domdata = NULL,
>> +.free_domdata = NULL,
>> +
>> +.alloc_udata= a653sched_alloc_udata,
>> .insert_unit= NULL,
>> .remove_unit= NULL,
>> +.free_udata = a653sched_free_udata,
>>
>> .sleep = a653sched_unit_sleep,
>> .wake = a653sched_unit_wake,
>> .yield = NULL,
>> .context_saved = NULL,
>>
>> -.do_schedule= a653sched_do_schedule,
>> -
>> .pick_resource = a653sched_pick_resource,
>> +.migrate= NULL,
>>
>> -.switch_sched = a653sched_switch_sched,
>> +.do_schedule= a653sched_do_schedule,
>>
>> .adjust = NULL,
>> +.adjust_affinity= NULL,
> Adding all these not really needed NULL initializers looks to rather move
> this scheduler away from all the others. (Oddly enough all of them
> explicitly set .sched_data to NULL - for whatever reason.)
The "= NULL" is totally redundant, because the compiler will do that for
you.
The last user of .sched_data was dropped by 9c95227160. Conceptually,
it is a layering violation (it prevents different cpupools being
properly independent), so I'd recommend just dropping the field entirely.
~Andrew
Re: [PATCH 4/5] sched/arinc653: Reorganize function definition order
On Thu, 2020-09-17 at 10:12 +0200, Jan Beulich wrote:
> On 16.09.2020 20:18, Jeff Kubascik wrote:
> > @@ -517,27 +516,35 @@ static const struct scheduler
> > sched_arinc653_def = {
> > .sched_id = XEN_SCHEDULER_ARINC653,
> > .sched_data = NULL,
> >
> > +.global_init= NULL,
> > .init = a653sched_init,
> > .deinit = a653sched_deinit,
> >
> > -.free_udata = a653sched_free_udata,
> > -.alloc_udata= a653sched_alloc_udata,
> > +.alloc_pdata= NULL,
> > +.switch_sched = a653sched_switch_sched,
> > +.deinit_pdata = NULL,
> > +.free_pdata = NULL,
> >
> > +.alloc_domdata = NULL,
> > +.free_domdata = NULL,
> > +
> > +.alloc_udata= a653sched_alloc_udata,
> > .insert_unit= NULL,
> > .remove_unit= NULL,
> > +.free_udata = a653sched_free_udata,
> >
> > .sleep = a653sched_unit_sleep,
> > .wake = a653sched_unit_wake,
> > .yield = NULL,
> > .context_saved = NULL,
> >
> > -.do_schedule= a653sched_do_schedule,
> > -
> > .pick_resource = a653sched_pick_resource,
> > +.migrate= NULL,
> >
> > -.switch_sched = a653sched_switch_sched,
> > +.do_schedule= a653sched_do_schedule,
> >
> > .adjust = NULL,
> > +.adjust_affinity= NULL,
>
> Adding all these not really needed NULL initializers looks to rather
> move
> this scheduler away from all the others.
>
Agreed, no need for more "= NULL". On the contrary, the ones that are
there should go away.
About this:
> (Oddly enough all of them
> explicitly set .sched_data to NULL - for whatever reason.)
>
Yes, we decided to keep it like that, back then. I think now it would
be ok for it to go away too.
So, Jeff, feel free to zap it with this patch or series. Or I can send
a patch to zap all of them, as you wish.
Regards
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
---
<> (Raistlin Majere)
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 4/5] sched/arinc653: Reorganize function definition order
On 16.09.2020 20:18, Jeff Kubascik wrote:
> @@ -517,27 +516,35 @@ static const struct scheduler sched_arinc653_def = {
> .sched_id = XEN_SCHEDULER_ARINC653,
> .sched_data = NULL,
>
> +.global_init= NULL,
> .init = a653sched_init,
> .deinit = a653sched_deinit,
>
> -.free_udata = a653sched_free_udata,
> -.alloc_udata= a653sched_alloc_udata,
> +.alloc_pdata= NULL,
> +.switch_sched = a653sched_switch_sched,
> +.deinit_pdata = NULL,
> +.free_pdata = NULL,
>
> +.alloc_domdata = NULL,
> +.free_domdata = NULL,
> +
> +.alloc_udata= a653sched_alloc_udata,
> .insert_unit= NULL,
> .remove_unit= NULL,
> +.free_udata = a653sched_free_udata,
>
> .sleep = a653sched_unit_sleep,
> .wake = a653sched_unit_wake,
> .yield = NULL,
> .context_saved = NULL,
>
> -.do_schedule= a653sched_do_schedule,
> -
> .pick_resource = a653sched_pick_resource,
> +.migrate= NULL,
>
> -.switch_sched = a653sched_switch_sched,
> +.do_schedule= a653sched_do_schedule,
>
> .adjust = NULL,
> +.adjust_affinity= NULL,
Adding all these not really needed NULL initializers looks to rather move
this scheduler away from all the others. (Oddly enough all of them
explicitly set .sched_data to NULL - for whatever reason.)
Jan
