Message ID | 306e62e8-9070-2db9-c959-858465c50c1d@suse.com (mailing list archive) |
---|---|
Headers | show |
Series | evtchn: (not so) recent XSAs follow-on | expand |
On 27.01.2021 09:13, Jan Beulich wrote: > These are grouped into a series largely because of their origin, > not so much because there are (heavy) dependencies among them. > The main change from v4 is the dropping of the two patches trying > to do away with the double event lock acquires in interdomain > channel handling. See also the individual patches. > > 1: use per-channel lock where possible > 2: convert domain event lock to an r/w one > 3: slightly defer lock acquire where possible > 4: add helper for port_is_valid() + evtchn_from_port() > 5: type adjustments > 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() Only patch 4 here has got an ack so far - may I ask for clear feedback as to at least some of these being acceptable (I can see the last one being controversial, and if this was the only one left I probably wouldn't even ping, despite thinking that it helps reduce unecessary overhead). Jan
On 21/04/2021 16:23, Jan Beulich wrote: > On 27.01.2021 09:13, Jan Beulich wrote: >> These are grouped into a series largely because of their origin, >> not so much because there are (heavy) dependencies among them. >> The main change from v4 is the dropping of the two patches trying >> to do away with the double event lock acquires in interdomain >> channel handling. See also the individual patches. >> >> 1: use per-channel lock where possible >> 2: convert domain event lock to an r/w one >> 3: slightly defer lock acquire where possible >> 4: add helper for port_is_valid() + evtchn_from_port() >> 5: type adjustments >> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() > > Only patch 4 here has got an ack so far - may I ask for clear feedback > as to at least some of these being acceptable (I can see the last one > being controversial, and if this was the only one left I probably > wouldn't even ping, despite thinking that it helps reduce unecessary > overhead). I left feedback for the series one the previous version (see [1]). It would have been nice is if it was mentionned somewhere as this is still unresolved. The locking in the event channel is already quite fragile (see recent XSAs, follow-up bugs...). Even if the pattern is used somewhere (as you suggested), I don't think it is good idea to pertain it. To be clear, I am not saying I don't care about performance. Instead I am trying to find a trade off between code maintenability and performance. So far, I didn't see any data justifying that the extra performance is worth the risk of making a code more fragile. Cheers, [1] <3c393170-09f9-6d31-c227-b599f8769e35@xen.org> > > Jan >
On 21.04.2021 17:56, Julien Grall wrote: > > > On 21/04/2021 16:23, Jan Beulich wrote: >> On 27.01.2021 09:13, Jan Beulich wrote: >>> These are grouped into a series largely because of their origin, >>> not so much because there are (heavy) dependencies among them. >>> The main change from v4 is the dropping of the two patches trying >>> to do away with the double event lock acquires in interdomain >>> channel handling. See also the individual patches. >>> >>> 1: use per-channel lock where possible >>> 2: convert domain event lock to an r/w one >>> 3: slightly defer lock acquire where possible >>> 4: add helper for port_is_valid() + evtchn_from_port() >>> 5: type adjustments >>> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() >> >> Only patch 4 here has got an ack so far - may I ask for clear feedback >> as to at least some of these being acceptable (I can see the last one >> being controversial, and if this was the only one left I probably >> wouldn't even ping, despite thinking that it helps reduce unecessary >> overhead). > > I left feedback for the series one the previous version (see [1]). It > would have been nice is if it was mentionned somewhere as this is still > unresolved. I will admit I forgot about the controversy on patch 1. I did, however, reply to your concerns. What didn't happen is the feedback from others that you did ask for. And of course there are 4 more patches here (one of them having an ack, yes) which could do with feedback. I'm certainly willing, where possible, to further re-order the series such that controversial changes are at its end. Jan
On Thu, Apr 22, 2021 at 10:53:05AM +0200, Jan Beulich wrote: > On 21.04.2021 17:56, Julien Grall wrote: > > > > > > On 21/04/2021 16:23, Jan Beulich wrote: > >> On 27.01.2021 09:13, Jan Beulich wrote: > >>> These are grouped into a series largely because of their origin, > >>> not so much because there are (heavy) dependencies among them. > >>> The main change from v4 is the dropping of the two patches trying > >>> to do away with the double event lock acquires in interdomain > >>> channel handling. See also the individual patches. > >>> > >>> 1: use per-channel lock where possible > >>> 2: convert domain event lock to an r/w one > >>> 3: slightly defer lock acquire where possible > >>> 4: add helper for port_is_valid() + evtchn_from_port() > >>> 5: type adjustments > >>> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() > >> > >> Only patch 4 here has got an ack so far - may I ask for clear feedback > >> as to at least some of these being acceptable (I can see the last one > >> being controversial, and if this was the only one left I probably > >> wouldn't even ping, despite thinking that it helps reduce unecessary > >> overhead). > > > > I left feedback for the series one the previous version (see [1]). It > > would have been nice is if it was mentionned somewhere as this is still > > unresolved. > > I will admit I forgot about the controversy on patch 1. I did, however, > reply to your concerns. What didn't happen is the feedback from others > that you did ask for. > > And of course there are 4 more patches here (one of them having an ack, > yes) which could do with feedback. I'm certainly willing, where possible, > to further re-order the series such that controversial changes are at its > end. I think it would easier to figure out whether the changes are correct if we had some kind of documentation about what/how the per-domain event_lock and the per-event locks are supposed to be used. I don't seem to be able to find any comments regarding how they are to be used. Regarding the changes itself in patch 1 (which I think has caused part of the controversy here), I'm unsure they are worth it because the functions modified all seem to be non-performance critical: evtchn_status, domain_dump_evtchn_info, flask_get_peer_sid. So I would say that unless we have clear rules written down for what the per-domain event_lock protects, I would be hesitant to change any of the logic, specially for critical paths. Thanks, Roger.
On 14.05.2021 17:29, Roger Pau Monné wrote: > On Thu, Apr 22, 2021 at 10:53:05AM +0200, Jan Beulich wrote: >> On 21.04.2021 17:56, Julien Grall wrote: >>> >>> >>> On 21/04/2021 16:23, Jan Beulich wrote: >>>> On 27.01.2021 09:13, Jan Beulich wrote: >>>>> These are grouped into a series largely because of their origin, >>>>> not so much because there are (heavy) dependencies among them. >>>>> The main change from v4 is the dropping of the two patches trying >>>>> to do away with the double event lock acquires in interdomain >>>>> channel handling. See also the individual patches. >>>>> >>>>> 1: use per-channel lock where possible >>>>> 2: convert domain event lock to an r/w one >>>>> 3: slightly defer lock acquire where possible >>>>> 4: add helper for port_is_valid() + evtchn_from_port() >>>>> 5: type adjustments >>>>> 6: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() >>>> >>>> Only patch 4 here has got an ack so far - may I ask for clear feedback >>>> as to at least some of these being acceptable (I can see the last one >>>> being controversial, and if this was the only one left I probably >>>> wouldn't even ping, despite thinking that it helps reduce unecessary >>>> overhead). >>> >>> I left feedback for the series one the previous version (see [1]). It >>> would have been nice is if it was mentionned somewhere as this is still >>> unresolved. >> >> I will admit I forgot about the controversy on patch 1. I did, however, >> reply to your concerns. What didn't happen is the feedback from others >> that you did ask for. >> >> And of course there are 4 more patches here (one of them having an ack, >> yes) which could do with feedback. I'm certainly willing, where possible, >> to further re-order the series such that controversial changes are at its >> end. > > I think it would easier to figure out whether the changes are correct > if we had some kind of documentation about what/how the per-domain > event_lock and the per-event locks are supposed to be used. I don't > seem to be able to find any comments regarding how they are to be > used. I think especially in pass-through code there are a number of cases where the per-domain lock really is being abused, simply for being available without much further thought. I'm not convinced documenting such abuse is going to help the situation. Yet of course I can see that having documentation would make review easier ... > Regarding the changes itself in patch 1 (which I think has caused part > of the controversy here), I'm unsure they are worth it because the > functions modified all seem to be non-performance critical: > evtchn_status, domain_dump_evtchn_info, flask_get_peer_sid. > > So I would say that unless we have clear rules written down for what > the per-domain event_lock protects, I would be hesitant to change any > of the logic, specially for critical paths. Okay, I'll drop patch 1 and also patch 6 for being overly controversial. Some of the other patches still look worthwhile to me, though. I'll also consider moving the spin->r/w lock conversion last in the series. Jan