Messages in this thread |  | | Date | Wed, 3 Oct 2018 21:43:39 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 2/3] x86/intel_rdt: CBM overlap should also check for overlap with CDP peer |
| |
Reinette,
On Wed, 3 Oct 2018, Reinette Chatre wrote: > On 10/3/2018 12:02 AM, Thomas Gleixner wrote: > >> +{ > >> + struct rdt_resource *r_cdp; > >> + struct rdt_domain *d_cdp; > >> + bool ret; > >> + > >> + ret = _rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive); > >> + if (ret) > >> + return ret; > > > > if (__rdtgroup_cbm_overlaps(r, d, _cbm, closid, exclusive)) > > return true; > > > >> + > >> + if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) == 0) > >> + return _rdtgroup_cbm_overlaps(r_cdp, d_cdp, _cbm, > >> + closid, exclusive); > > > > if (rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp) < 0) > > return false; > > > > return __rdtgroup_cbm_overlaps(r_cpd, d_cdp, _cbm, closid, exclusive); > > > > Makes the whole thing more obvious. > > I think a different change is needed to support the request from your > review of the first patch to propagate that unthinkable error where only > one of the CDP peers could have an rdt_domain associated with it. > > In the above that error in question from rdt_cdp_peer_get() will be lost. > > I could do the following in support of propagating that error (note that > in support of the code below __rdtgroup_cbm_overlaps() also changes to > return int instead of bool): > > int rdtgroup_cbm_overlaps(struct rdt_resource *r, struct rdt_domain *d, > u32 cbm, int closid, bool exclusive) > { > struct rdt_resource *r_cdp; > struct rdt_domain *d_cdp; > int ret; > > if (__rdtgroup_cbm_overlaps(r, d, cbm, closid, exclusive)) > return 1; > > ret = rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp); > if (ret == -ENOENT) { > return 0; > } else if (ret == -EINVAL) { > rdt_last_cmd_puts("Error finding CDP peer\n"); > return ret; > } else { > return __rdtgroup_cbm_overlaps(r_cdp, d_cdp, cbm, > closid, exclusive); > } > > return -EINVAL; > } > > With the above change in rdtgroup_cbm_overlaps() the call sites then > change to for example: > > ret = rdtgroup_cbm_overlaps(r, d, cbm_val, rdtgrp->closid, true); > if (ret < 0) { > /* last_cmd_status already populated with error */ > return -EINVAL; > } else if (ret == 1) { > rdt_last_cmd_puts("overlaps with exclusive group\n"); > return -EINVAL; > } > /* fall through when no overlap detected */ > > Would this be acceptable?
We really have to think about that whether it's worth it. Looking at the resulting code I doubt it. Then I'd rather prefer the warnon and the simpler code. But either way works for me.
Thanks,
tglx
|  |