| Message ID | 20221007193124.20322-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | argo: Remove reachable ASSERT_UNREACHABLE | expand |
On 07/10/2022 20:31, Jason Andryuk wrote: > I observed this ASSERT_UNREACHABLE in partner_rings_remove consistently > trip. It was in OpenXT with the viptables patch applied. > > dom10 shuts down. > dom7 is REJECTED sending to dom10. > dom7 shuts down and this ASSERT trips for dom10. > > The argo_send_info has a domid, but there is no refcount taken on > the domain. Therefore it's not appropriate to ASSERT that the domain > can be looked up via domid. Replace with a debug message. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> We're into the 4.17 release process now. A bugfix like this obviously should be considered, but will need approval from the release manager. CC Henry. ~Andrew > --- > xen/common/argo.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/common/argo.c b/xen/common/argo.c > index 748b8714d6..973e1e9956 100644 > --- a/xen/common/argo.c > +++ b/xen/common/argo.c > @@ -1298,7 +1298,8 @@ partner_rings_remove(struct domain *src_d) > ASSERT_UNREACHABLE(); > } > else > - ASSERT_UNREACHABLE(); > + argo_dprintk("%pd has entry for stale partner domid %d\n", > + src_d, send_info->id.domain_id); > > if ( dst_d ) > rcu_unlock_domain(dst_d);
Hi Andrew and Jason, > -----Original Message----- > From: Andrew Cooper <Andrew.Cooper3@citrix.com> > Subject: Re: [PATCH] argo: Remove reachable ASSERT_UNREACHABLE > > On 07/10/2022 20:31, Jason Andryuk wrote: > > I observed this ASSERT_UNREACHABLE in partner_rings_remove > consistently > > trip. It was in OpenXT with the viptables patch applied. > > > > dom10 shuts down. > > dom7 is REJECTED sending to dom10. > > dom7 shuts down and this ASSERT trips for dom10. > > > > The argo_send_info has a domid, but there is no refcount taken on > > the domain. Therefore it's not appropriate to ASSERT that the domain > > can be looked up via domid. Replace with a debug message. > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > We're into the 4.17 release process now. A bugfix like this obviously > should be considered, but will need approval from the release manager. > CC Henry. Andrew: Thanks for the information! Jason: Would you mind adding a "Fixes:" tag following the rule described in [1]? Thanks very much! With this tag and proper review/ack from maintainers: Release-acked-by: Henry Wang <Henry.Wang@arm.com> [1] https://xenbits.xen.org/docs/unstable/process/sending-patches.html#fixes Kind regards, Henry > > ~Andrew > > > --- > > xen/common/argo.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/xen/common/argo.c b/xen/common/argo.c > > index 748b8714d6..973e1e9956 100644 > > --- a/xen/common/argo.c > > +++ b/xen/common/argo.c > > @@ -1298,7 +1298,8 @@ partner_rings_remove(struct domain *src_d) > > ASSERT_UNREACHABLE(); > > } > > else > > - ASSERT_UNREACHABLE(); > > + argo_dprintk("%pd has entry for stale partner domid %d\n", > > + src_d, send_info->id.domain_id); > > > > if ( dst_d ) > > rcu_unlock_domain(dst_d);
On Fri, Oct 7, 2022 at 9:12 PM Henry Wang <Henry.Wang@arm.com> wrote: > > Hi Andrew and Jason, > > > -----Original Message----- > > From: Andrew Cooper <Andrew.Cooper3@citrix.com> > > Subject: Re: [PATCH] argo: Remove reachable ASSERT_UNREACHABLE > > > > On 07/10/2022 20:31, Jason Andryuk wrote: > > > I observed this ASSERT_UNREACHABLE in partner_rings_remove > > consistently > > > trip. It was in OpenXT with the viptables patch applied. > > > > > > dom10 shuts down. > > > dom7 is REJECTED sending to dom10. > > > dom7 shuts down and this ASSERT trips for dom10. dom7 used a wildcard ring, and dom10 connected to it with a (driver level) stream socket. > > > The argo_send_info has a domid, but there is no refcount taken on > > > the domain. Therefore it's not appropriate to ASSERT that the domain > > > can be looked up via domid. Replace with a debug message. > > > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > > > We're into the 4.17 release process now. A bugfix like this obviously > > should be considered, but will need approval from the release manager. > > CC Henry. Thanks, Andrew. > Andrew: Thanks for the information! > > Jason: Would you mind adding a "Fixes:" tag following the rule described > in [1]? Thanks very much! With this tag and proper review/ack from > maintainers: Of course. It would be: Fixes: 82a817307c5b "argo: init, destroy and soft-reset, with enable command line opt" > Release-acked-by: Henry Wang <Henry.Wang@arm.com> Thanks, Henry. We'll see what feedback Christopher provides. Regards, Jason
Hi Christopher, > -----Original Message----- > From: Jason Andryuk <jandryuk@gmail.com> > Subject: Re: [PATCH] argo: Remove reachable ASSERT_UNREACHABLE > > On Fri, Oct 7, 2022 at 9:12 PM Henry Wang <Henry.Wang@arm.com> wrote: > > > > Hi Andrew and Jason, > > > > > -----Original Message----- > > > From: Andrew Cooper <Andrew.Cooper3@citrix.com> > > > Subject: Re: [PATCH] argo: Remove reachable ASSERT_UNREACHABLE > > > > > > On 07/10/2022 20:31, Jason Andryuk wrote: > > > > I observed this ASSERT_UNREACHABLE in partner_rings_remove > > > consistently > > > > trip. It was in OpenXT with the viptables patch applied. > > > > > > > > dom10 shuts down. > > > > dom7 is REJECTED sending to dom10. > > > > dom7 shuts down and this ASSERT trips for dom10. > > dom7 used a wildcard ring, and dom10 connected to it with a (driver > level) stream socket. > > > > > The argo_send_info has a domid, but there is no refcount taken on > > > > the domain. Therefore it's not appropriate to ASSERT that the domain > > > > can be looked up via domid. Replace with a debug message. > > > > > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > > > > > We're into the 4.17 release process now. A bugfix like this obviously > > > should be considered, but will need approval from the release manager. > > > CC Henry. > > Thanks, Andrew. > > > Andrew: Thanks for the information! > > > > Jason: Would you mind adding a "Fixes:" tag following the rule described > > in [1]? Thanks very much! With this tag and proper review/ack from > > maintainers: > > Of course. It would be: > Fixes: 82a817307c5b "argo: init, destroy and soft-reset, with enable > command line opt" > > > Release-acked-by: Henry Wang <Henry.Wang@arm.com> > > Thanks, Henry. We'll see what feedback Christopher provides. Since we are in the process of 4.17 release and the release is supposed to happen after 2 weeks, may I have your feedback here for the patch? Thank you very much for your time. Kind regards, Henry > > Regards, > Jason
On Tue, Oct 11, 2022 at 5:18 AM Jason Andryuk <jandryuk@gmail.com> wrote: > On Fri, Oct 7, 2022 at 9:12 PM Henry Wang <Henry.Wang@arm.com> wrote: > > > > Hi Andrew and Jason, > > > > > -----Original Message----- > > > From: Andrew Cooper <Andrew.Cooper3@citrix.com> > > > Subject: Re: [PATCH] argo: Remove reachable ASSERT_UNREACHABLE > > > > > > On 07/10/2022 20:31, Jason Andryuk wrote: > > > > I observed this ASSERT_UNREACHABLE in partner_rings_remove > > > consistently > > > > trip. It was in OpenXT with the viptables patch applied. > > > > > > > > dom10 shuts down. > > > > dom7 is REJECTED sending to dom10. > > > > dom7 shuts down and this ASSERT trips for dom10. > > dom7 used a wildcard ring, and dom10 connected to it with a (driver > level) stream socket. > > > > > The argo_send_info has a domid, but there is no refcount taken on > > > > the domain. Therefore it's not appropriate to ASSERT that the domain > > > > can be looked up via domid. Replace with a debug message. > I follow this - thanks for the explanation. > > > > > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com> > > > > > > We're into the 4.17 release process now. A bugfix like this obviously > > > should be considered, but will need approval from the release manager. > > > CC Henry. > > Thanks, Andrew. > > > Andrew: Thanks for the information! > > > > Jason: Would you mind adding a "Fixes:" tag following the rule described > > in [1]? Thanks very much! With this tag and proper review/ack from > > maintainers: > > Of course. It would be: > Fixes: 82a817307c5b "argo: init, destroy and soft-reset, with enable > command line opt" > > > Release-acked-by: Henry Wang <Henry.Wang@arm.com> > > Thanks, Henry. We'll see what feedback Christopher provides. > I agree - include for 4.17. thanks Christopher > > Regards, > Jason >
On 07/10/2022 20:31, Jason Andryuk wrote: > I observed this ASSERT_UNREACHABLE in partner_rings_remove consistently > trip. It was in OpenXT with the viptables patch applied. > > dom10 shuts down. > dom7 is REJECTED sending to dom10. > dom7 shuts down and this ASSERT trips for dom10. > > The argo_send_info has a domid, but there is no refcount taken on > the domain. Therefore it's not appropriate to ASSERT that the domain > can be looked up via domid. Replace with a debug message. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- > xen/common/argo.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/common/argo.c b/xen/common/argo.c > index 748b8714d6..973e1e9956 100644 > --- a/xen/common/argo.c > +++ b/xen/common/argo.c > @@ -1298,7 +1298,8 @@ partner_rings_remove(struct domain *src_d) > ASSERT_UNREACHABLE(); > } > else > - ASSERT_UNREACHABLE(); > + argo_dprintk("%pd has entry for stale partner domid %d\n", > + src_d, send_info->id.domain_id); I was just about to commit this, but it ought to be "%pd has entry for stale partner d%u\n" so the two domains are rendered alike in the message. Can fix on your behalf if you agree. ~Andrew
On Fri, Oct 14, 2022 at 9:22 AM Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 07/10/2022 20:31, Jason Andryuk wrote: > > I observed this ASSERT_UNREACHABLE in partner_rings_remove consistently > > trip. It was in OpenXT with the viptables patch applied. > > > > dom10 shuts down. > > dom7 is REJECTED sending to dom10. > > dom7 shuts down and this ASSERT trips for dom10. > > > > The argo_send_info has a domid, but there is no refcount taken on > > the domain. Therefore it's not appropriate to ASSERT that the domain > > can be looked up via domid. Replace with a debug message. > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > --- > > xen/common/argo.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/xen/common/argo.c b/xen/common/argo.c > > index 748b8714d6..973e1e9956 100644 > > --- a/xen/common/argo.c > > +++ b/xen/common/argo.c > > @@ -1298,7 +1298,8 @@ partner_rings_remove(struct domain *src_d) > > ASSERT_UNREACHABLE(); > > } > > else > > - ASSERT_UNREACHABLE(); > > + argo_dprintk("%pd has entry for stale partner domid %d\n", > > + src_d, send_info->id.domain_id); > > I was just about to commit this, but it ought to be > > "%pd has entry for stale partner d%u\n" > > so the two domains are rendered alike in the message. > > Can fix on your behalf if you agree. Yes, that sounds better. Thank you. -Jason
diff --git a/xen/common/argo.c b/xen/common/argo.c index 748b8714d6..973e1e9956 100644 --- a/xen/common/argo.c +++ b/xen/common/argo.c @@ -1298,7 +1298,8 @@ partner_rings_remove(struct domain *src_d) ASSERT_UNREACHABLE(); } else - ASSERT_UNREACHABLE(); + argo_dprintk("%pd has entry for stale partner domid %d\n", + src_d, send_info->id.domain_id); if ( dst_d ) rcu_unlock_domain(dst_d);
I observed this ASSERT_UNREACHABLE in partner_rings_remove consistently trip. It was in OpenXT with the viptables patch applied. dom10 shuts down. dom7 is REJECTED sending to dom10. dom7 shuts down and this ASSERT trips for dom10. The argo_send_info has a domid, but there is no refcount taken on the domain. Therefore it's not appropriate to ASSERT that the domain can be looked up via domid. Replace with a debug message. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- xen/common/argo.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)