| Message ID | 20200916181854.75563-4-jeff.kubascik@dornerworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multicore support for ARINC653 scheduler | expand |
On 16.09.2020 20:18, Jeff Kubascik wrote: > --- a/xen/common/sched/arinc653.c > +++ b/xen/common/sched/arinc653.c > @@ -119,10 +119,9 @@ static int dom_handle_cmp(const xen_domain_handle_t h1, > return memcmp(h1, h2, sizeof(xen_domain_handle_t)); > } > > -static struct sched_unit *find_unit( > - const struct scheduler *ops, > - xen_domain_handle_t handle, > - int unit_id) > +static struct sched_unit *find_unit(const struct scheduler *ops, > + xen_domain_handle_t handle, > + int unit_id) > { Just fyi, afaict we consider both variants legitimate style as far as Xen as a whole is concerned; I'm unaware of scheduler code specific restrictions (but I'll be happy to be corrected if I'm wrong with this). Instead what I'm wondering by merely seeing this piece of code is whether unit_id really can go negative. If not (as would be the common case with IDs), it would want converting to unsigned int, which may be more important than the purely typographical adjustment done here. Jan
On Thu, 2020-09-17 at 10:09 +0200, Jan Beulich wrote: > On 16.09.2020 20:18, Jeff Kubascik wrote: > > --- a/xen/common/sched/arinc653.c > > +++ b/xen/common/sched/arinc653.c > > @@ -119,10 +119,9 @@ static int dom_handle_cmp(const > > xen_domain_handle_t h1, > > return memcmp(h1, h2, sizeof(xen_domain_handle_t)); > > } > > > > -static struct sched_unit *find_unit( > > - const struct scheduler *ops, > > - xen_domain_handle_t handle, > > - int unit_id) > > +static struct sched_unit *find_unit(const struct scheduler *ops, > > + xen_domain_handle_t handle, > > + int unit_id) > > { > > Just fyi, afaict we consider both variants legitimate style as far > as Xen as a whole is concerned; I'm unaware of scheduler code > specific restrictions (but I'll be happy to be corrected if I'm > wrong with this). > No, you're right, there aren't any additional restrictions. And, as many other subsystems, scheduling code is not always 100% consistent. There's quite a mix of style. E.g., there are both examples of the style that this hunk above is changing and of the one that the patch is changing it to. So I also see limited need for doing it. But of course it's Josh's and Stweart's call, I guess. > Instead what I'm wondering by merely seeing this piece of code is > whether unit_id really can go negative. If not (as would be the > common case with IDs), it would want converting to unsigned int, > which may be more important than the purely typographical > adjustment done here. > Yep, it's defined as `unsigned int` in `struct sched_unit`. So this indeed would be valuable. And while you're there, this probably applies here as well: /** * The sched_entry_t structure holds a single entry of the * ARINC 653 schedule. */ typedef struct sched_entry_s { /* dom_handle holds the handle ("UUID") for the domain that this * schedule entry refers to. */ xen_domain_handle_t dom_handle; /* unit_id holds the UNIT number for the UNIT that this schedule * entry refers to. */ int unit_id; ... } Regards
On 9/17/2020 10:40 AM, Dario Faggioli wrote: >On Thu, 2020-09-17 at 10:09 +0200, Jan Beulich wrote: >> On 16.09.2020 20:18, Jeff Kubascik wrote: >>> --- a/xen/common/sched/arinc653.c >>> +++ b/xen/common/sched/arinc653.c >>> @@ -119,10 +119,9 @@ static int dom_handle_cmp(const >>> xen_domain_handle_t h1, >>> return memcmp(h1, h2, sizeof(xen_domain_handle_t)); >>> } >>> >>> -static struct sched_unit *find_unit( >>> - const struct scheduler *ops, >>> - xen_domain_handle_t handle, >>> - int unit_id) >>> +static struct sched_unit *find_unit(const struct scheduler *ops, >>> + xen_domain_handle_t handle, >>> + int unit_id) >>> { >> >> Just fyi, afaict we consider both variants legitimate style as far >> as Xen as a whole is concerned; I'm unaware of scheduler code >> specific restrictions (but I'll be happy to be corrected if I'm >> wrong with this). >> >No, you're right, there aren't any additional restrictions. And, as >many other subsystems, scheduling code is not always 100% consistent. >There's quite a mix of style. E.g., there are both examples of the >style that this hunk above is changing and of the one that the patch is >changing it to. > >So I also see limited need for doing it. But of course it's Josh's and >Stweart's call, I guess. If that's the case, then I'm thinking keeping the previous style would be preferred. I'll switch it back. >> Instead what I'm wondering by merely seeing this piece of code is >> whether unit_id really can go negative. If not (as would be the >> common case with IDs), it would want converting to unsigned int, >> which may be more important than the purely typographical >> adjustment done here. >> >Yep, it's defined as `unsigned int` in `struct sched_unit`. > >So this indeed would be valuable. And while you're there, this probably >applies here as well: > >/** > * The sched_entry_t structure holds a single entry of the > * ARINC 653 schedule. > */ >typedef struct sched_entry_s >{ > /* dom_handle holds the handle ("UUID") for the domain that this > * schedule entry refers to. */ > xen_domain_handle_t dom_handle; > /* unit_id holds the UNIT number for the UNIT that this schedule > * entry refers to. */ > int unit_id; > ... >} Agreed. I'll make this change. -Jeff
diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c index d8a23730c3..5f3a1be990 100644 --- a/xen/common/sched/arinc653.c +++ b/xen/common/sched/arinc653.c @@ -119,10 +119,9 @@ static int dom_handle_cmp(const xen_domain_handle_t h1, return memcmp(h1, h2, sizeof(xen_domain_handle_t)); } -static struct sched_unit *find_unit( - const struct scheduler *ops, - xen_domain_handle_t handle, - int unit_id) +static struct sched_unit *find_unit(const struct scheduler *ops, + xen_domain_handle_t handle, + int unit_id) { struct a653sched_unit *aunit; @@ -145,10 +144,8 @@ static void update_schedule_units(const struct scheduler *ops) SCHED_PRIV(ops)->schedule[i].unit_id); } -static int -arinc653_sched_set( - const struct scheduler *ops, - struct xen_sysctl_arinc653_schedule *schedule) +static int a653sched_set(const struct scheduler *ops, + struct xen_sysctl_arinc653_schedule *schedule) { struct a653sched_private *sched_priv = SCHED_PRIV(ops); s_time_t total_runtime = 0; @@ -212,10 +209,8 @@ arinc653_sched_set( return rc; } -static int -arinc653_sched_get( - const struct scheduler *ops, - struct xen_sysctl_arinc653_schedule *schedule) +static int a653sched_get(const struct scheduler *ops, + struct xen_sysctl_arinc653_schedule *schedule) { struct a653sched_private *sched_priv = SCHED_PRIV(ops); unsigned int i; @@ -239,8 +234,7 @@ arinc653_sched_get( return 0; } -static int -a653sched_init(struct scheduler *ops) +static int a653sched_init(struct scheduler *ops) { struct a653sched_private *prv; @@ -257,16 +251,15 @@ a653sched_init(struct scheduler *ops) return 0; } -static void -a653sched_deinit(struct scheduler *ops) +static void a653sched_deinit(struct scheduler *ops) { xfree(SCHED_PRIV(ops)); ops->sched_data = NULL; } -static void * -a653sched_alloc_udata(const struct scheduler *ops, struct sched_unit *unit, - void *dd) +static void *a653sched_alloc_udata(const struct scheduler *ops, + struct sched_unit *unit, + void *dd) { struct a653sched_private *sched_priv = SCHED_PRIV(ops); struct a653sched_unit *svc; @@ -320,8 +313,7 @@ a653sched_alloc_udata(const struct scheduler *ops, struct sched_unit *unit, return svc; } -static void -a653sched_free_udata(const struct scheduler *ops, void *priv) +static void a653sched_free_udata(const struct scheduler *ops, void *priv) { struct a653sched_private *sched_priv = SCHED_PRIV(ops); struct a653sched_unit *av = priv; @@ -341,8 +333,8 @@ a653sched_free_udata(const struct scheduler *ops, void *priv) spin_unlock_irqrestore(&sched_priv->lock, flags); } -static void -a653sched_unit_sleep(const struct scheduler *ops, struct sched_unit *unit) +static void a653sched_unit_sleep(const struct scheduler *ops, + struct sched_unit *unit) { if ( AUNIT(unit) != NULL ) AUNIT(unit)->awake = false; @@ -355,8 +347,8 @@ a653sched_unit_sleep(const struct scheduler *ops, struct sched_unit *unit) cpu_raise_softirq(sched_unit_master(unit), SCHEDULE_SOFTIRQ); } -static void -a653sched_unit_wake(const struct scheduler *ops, struct sched_unit *unit) +static void a653sched_unit_wake(const struct scheduler *ops, + struct sched_unit *unit) { if ( AUNIT(unit) != NULL ) AUNIT(unit)->awake = true; @@ -364,12 +356,9 @@ a653sched_unit_wake(const struct scheduler *ops, struct sched_unit *unit) cpu_raise_softirq(sched_unit_master(unit), SCHEDULE_SOFTIRQ); } -static void -a653sched_do_schedule( - const struct scheduler *ops, - struct sched_unit *prev, - s_time_t now, - bool tasklet_work_scheduled) +static void a653sched_do_schedule(const struct scheduler *ops, + struct sched_unit *prev, s_time_t now, + bool tasklet_work_scheduled) { struct sched_unit *new_task = NULL; static unsigned int sched_index = 0; @@ -477,9 +466,9 @@ a653sched_pick_resource(const struct scheduler *ops, return get_sched_res(cpu); } -static spinlock_t * -a653_switch_sched(struct scheduler *new_ops, unsigned int cpu, - void *pdata, void *vdata) +static spinlock_t *a653sched_switch_sched(struct scheduler *new_ops, + unsigned int cpu, void *pdata, + void *vdata) { struct sched_resource *sr = get_sched_res(cpu); const struct a653sched_unit *svc = vdata; @@ -491,9 +480,8 @@ a653_switch_sched(struct scheduler *new_ops, unsigned int cpu, return &sr->_lock; } -static int -a653sched_adjust_global(const struct scheduler *ops, - struct xen_sysctl_scheduler_op *sc) +static int a653sched_adjust_global(const struct scheduler *ops, + struct xen_sysctl_scheduler_op *sc) { struct xen_sysctl_arinc653_schedule local_sched; int rc = -EINVAL; @@ -507,11 +495,11 @@ a653sched_adjust_global(const struct scheduler *ops, break; } - rc = arinc653_sched_set(ops, &local_sched); + rc = a653sched_set(ops, &local_sched); break; case XEN_SYSCTL_SCHEDOP_getinfo: memset(&local_sched, -1, sizeof(local_sched)); - rc = arinc653_sched_get(ops, &local_sched); + rc = a653sched_get(ops, &local_sched); if ( rc ) break; @@ -547,7 +535,7 @@ static const struct scheduler sched_arinc653_def = { .pick_resource = a653sched_pick_resource, - .switch_sched = a653_switch_sched, + .switch_sched = a653sched_switch_sched, .adjust = NULL, .adjust_global = a653sched_adjust_global,
Function definitions in the arinc653 module did not follow the Xen coding style. Furthermore, a few function names used a different prefix. This change cleans up the definitions to be consistent with the Xen coding style, and has no functional changes. Signed-off-by: Jeff Kubascik <jeff.kubascik@dornerworks.com> --- xen/common/sched/arinc653.c | 68 +++++++++++++++---------------------- 1 file changed, 28 insertions(+), 40 deletions(-)