| Message ID | fb83896f3b399c7ace3292f38506812bc4616f6d.1560272437.git.tsirakisn@ainfosec.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] argo: warn sendv() caller when ring is full | expand |
On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis <niko.tsirakis@gmail.com> wrote: > > In its current state, if the destination ring is full, sendv() > will requeue the message and return the rc of pending_requeue(), > which will return 0 on success. This prevents the caller from > distinguishing the difference between a successful write and a > message that needs to be resent at a later time. > > Instead, capture the -EAGAIN value returned from ringbuf_insert() > and *only* overwrite it if the rc of pending_requeue() is non-zero. > This allows the caller to make intelligent decisions on -EAGAIN and > still be alerted if the pending message fails to requeue. > > Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com> Thanks for the correct identification of the problem and the patch. Reviewed-by: Christopher Clark <christopher.clark6@baesystems.com> > --- > xen/common/argo.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/xen/common/argo.c b/xen/common/argo.c > index 13052b9239..2f874a570d 100644 > --- a/xen/common/argo.c > +++ b/xen/common/argo.c > @@ -2048,9 +2048,13 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr, > message_type, &len); > if ( ret == -EAGAIN ) > { > + int rc; > + > argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n"); > /* requeue to issue a notification when space is there */ > - ret = pending_requeue(dst_d, ring_info, src_id.domain_id, len); > + rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len); > + if ( rc ) > + ret = rc; > } > > spin_unlock(&ring_info->L3_lock); > -- > 2.17.1 >
On 11/06/2019 19:43, Christopher Clark wrote: > On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis > <niko.tsirakis@gmail.com> wrote: >> In its current state, if the destination ring is full, sendv() >> will requeue the message and return the rc of pending_requeue(), >> which will return 0 on success. This prevents the caller from >> distinguishing the difference between a successful write and a >> message that needs to be resent at a later time. >> >> Instead, capture the -EAGAIN value returned from ringbuf_insert() >> and *only* overwrite it if the rc of pending_requeue() is non-zero. >> This allows the caller to make intelligent decisions on -EAGAIN and >> still be alerted if the pending message fails to requeue. >> >> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com> > Thanks for the correct identification of the problem and the patch. > > Reviewed-by: Christopher Clark <christopher.clark6@baesystems.com> So I was coming to commit this, but technically according to the maintainers file, ARGO is maintained by <christopher.w.clark@gmail.com> Looking at the ARGO series as committed, the patches where all From: gmail, SoB: baesystems. Which is the correct alias to use? ~Andrew
On Tue, Jun 11, 2019 at 12:16 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 11/06/2019 19:43, Christopher Clark wrote: > > On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis > > <niko.tsirakis@gmail.com> wrote: > >> In its current state, if the destination ring is full, sendv() > >> will requeue the message and return the rc of pending_requeue(), > >> which will return 0 on success. This prevents the caller from > >> distinguishing the difference between a successful write and a > >> message that needs to be resent at a later time. > >> > >> Instead, capture the -EAGAIN value returned from ringbuf_insert() > >> and *only* overwrite it if the rc of pending_requeue() is non-zero. > >> This allows the caller to make intelligent decisions on -EAGAIN and > >> still be alerted if the pending message fails to requeue. > >> > >> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com> > > Thanks for the correct identification of the problem and the patch. > > > > Reviewed-by: Christopher Clark <christopher.clark6@baesystems.com> > > So I was coming to commit this, but technically according to the > maintainers file, ARGO is maintained by <christopher.w.clark@gmail.com> > > Looking at the ARGO series as committed, the patches where all From: > gmail, SoB: baesystems. > > Which is the correct alias to use? For this purpose: Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com> thanks Christopher
On 11/06/2019 20:22, Christopher Clark wrote: > On Tue, Jun 11, 2019 at 12:16 PM Andrew Cooper > <andrew.cooper3@citrix.com> wrote: >> On 11/06/2019 19:43, Christopher Clark wrote: >>> On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis >>> <niko.tsirakis@gmail.com> wrote: >>>> In its current state, if the destination ring is full, sendv() >>>> will requeue the message and return the rc of pending_requeue(), >>>> which will return 0 on success. This prevents the caller from >>>> distinguishing the difference between a successful write and a >>>> message that needs to be resent at a later time. >>>> >>>> Instead, capture the -EAGAIN value returned from ringbuf_insert() >>>> and *only* overwrite it if the rc of pending_requeue() is non-zero. >>>> This allows the caller to make intelligent decisions on -EAGAIN and >>>> still be alerted if the pending message fails to requeue. >>>> >>>> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com> >>> Thanks for the correct identification of the problem and the patch. >>> >>> Reviewed-by: Christopher Clark <christopher.clark6@baesystems.com> >> So I was coming to commit this, but technically according to the >> maintainers file, ARGO is maintained by <christopher.w.clark@gmail.com> >> >> Looking at the ARGO series as committed, the patches where all From: >> gmail, SoB: baesystems. >> >> Which is the correct alias to use? > For this purpose: > Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com> Fixed up and pushed. Thanks. ~Andrew
diff --git a/xen/common/argo.c b/xen/common/argo.c index 13052b9239..2f874a570d 100644 --- a/xen/common/argo.c +++ b/xen/common/argo.c @@ -2048,9 +2048,13 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr, message_type, &len); if ( ret == -EAGAIN ) { + int rc; + argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n"); /* requeue to issue a notification when space is there */ - ret = pending_requeue(dst_d, ring_info, src_id.domain_id, len); + rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len); + if ( rc ) + ret = rc; } spin_unlock(&ring_info->L3_lock);
In its current state, if the destination ring is full, sendv() will requeue the message and return the rc of pending_requeue(), which will return 0 on success. This prevents the caller from distinguishing the difference between a successful write and a message that needs to be resent at a later time. Instead, capture the -EAGAIN value returned from ringbuf_insert() and *only* overwrite it if the rc of pending_requeue() is non-zero. This allows the caller to make intelligent decisions on -EAGAIN and still be alerted if the pending message fails to requeue. Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com> --- xen/common/argo.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)