| Message ID | 1472462072-3734-1-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 29.08.16 at 11:14, <feng.wu@intel.com> wrote: > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -243,7 +243,7 @@ static struct vcpu *vector_hashing_dest(const struct domain *d, > for ( i = 0; i <= mod; i++ ) > { > idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx) + 1; > - BUG_ON(idx >= d->max_vcpus); > + BUG_ON(idx > d->max_vcpus); > } > > dest = d->vcpu[idx - 1]; Wouldn't it be better to change the code to unsigned int idx = -1; for ( i = 0; i <= mod; i++ ) { idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx + 1); BUG_ON(idx >= d->max_vcpus); } dest = d->vcpu[idx]; or, not utilizing wrapping unsigned int idx = find_first_bit(dest_vcpu_bitmap, d->max_vcpus); for ( i = 0; i < mod; i++ ) idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx + 1); BUG_ON(idx >= d->max_vcpus); dest = d->vcpu[idx]; Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, August 29, 2016 7:51 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: xen-devel@lists.xen.org > Subject: Re: [PATCH] Fix a BUG_ON issue > > >>> On 29.08.16 at 11:14, <feng.wu@intel.com> wrote: > > --- a/xen/drivers/passthrough/io.c > > +++ b/xen/drivers/passthrough/io.c > > @@ -243,7 +243,7 @@ static struct vcpu *vector_hashing_dest(const struct > domain *d, > > for ( i = 0; i <= mod; i++ ) > > { > > idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx) + 1; > > - BUG_ON(idx >= d->max_vcpus); > > + BUG_ON(idx > d->max_vcpus); > > } > > > > dest = d->vcpu[idx - 1]; > > Wouldn't it be better to change the code to > > unsigned int idx = -1; > > for ( i = 0; i <= mod; i++ ) > { > idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx + 1); > BUG_ON(idx >= d->max_vcpus); > } > > dest = d->vcpu[idx]; Thanks for the comments, both are good to me, but I slightly prefer this one. Do I need to send another version? Thanks, Feng > > or, not utilizing wrapping > > unsigned int idx = find_first_bit(dest_vcpu_bitmap, d->max_vcpus); > > for ( i = 0; i < mod; i++ ) > idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx + 1); > > BUG_ON(idx >= d->max_vcpus); > > dest = d->vcpu[idx]; > > Jan
>>> On 30.08.16 at 01:19, <feng.wu@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Monday, August 29, 2016 7:51 PM >> >>> On 29.08.16 at 11:14, <feng.wu@intel.com> wrote: >> > --- a/xen/drivers/passthrough/io.c >> > +++ b/xen/drivers/passthrough/io.c >> > @@ -243,7 +243,7 @@ static struct vcpu *vector_hashing_dest(const struct >> domain *d, >> > for ( i = 0; i <= mod; i++ ) >> > { >> > idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx) + 1; >> > - BUG_ON(idx >= d->max_vcpus); >> > + BUG_ON(idx > d->max_vcpus); >> > } >> > >> > dest = d->vcpu[idx - 1]; >> >> Wouldn't it be better to change the code to >> >> unsigned int idx = -1; >> >> for ( i = 0; i <= mod; i++ ) >> { >> idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx + 1); >> BUG_ON(idx >= d->max_vcpus); >> } >> >> dest = d->vcpu[idx]; > > Thanks for the comments, both are good to me, but I slightly prefer this > one. Do I need to send another version? Not necessarily - can you reason a little about your preference? I particularly dislike the subtraction necessary here: dest = d->vcpu[idx - 1]; Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Tuesday, August 30, 2016 2:34 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: xen-devel@lists.xen.org > Subject: RE: [PATCH] Fix a BUG_ON issue > > >>> On 30.08.16 at 01:19, <feng.wu@intel.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Monday, August 29, 2016 7:51 PM > >> >>> On 29.08.16 at 11:14, <feng.wu@intel.com> wrote: > >> > --- a/xen/drivers/passthrough/io.c > >> > +++ b/xen/drivers/passthrough/io.c > >> > @@ -243,7 +243,7 @@ static struct vcpu *vector_hashing_dest(const > struct > >> domain *d, > >> > for ( i = 0; i <= mod; i++ ) > >> > { > >> > idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx) + 1; > >> > - BUG_ON(idx >= d->max_vcpus); > >> > + BUG_ON(idx > d->max_vcpus); > >> > } > >> > > >> > dest = d->vcpu[idx - 1]; > >> > >> Wouldn't it be better to change the code to > >> > >> unsigned int idx = -1; > >> > >> for ( i = 0; i <= mod; i++ ) > >> { > >> idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx + 1); > >> BUG_ON(idx >= d->max_vcpus); > >> } > >> > >> dest = d->vcpu[idx]; > > > > Thanks for the comments, both are good to me, but I slightly prefer this > > one. Do I need to send another version? > > Not necessarily - can you reason a little about your preference? I Oh, no particular reason, just feel it looks a little simple! :) But I am fine if you prefer the other one. Thanks, Feng > particularly dislike the subtraction necessary here: > > dest = d->vcpu[idx - 1]; > > Jan
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c index 9e6b46c..66577b6 100644 --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -243,7 +243,7 @@ static struct vcpu *vector_hashing_dest(const struct domain *d, for ( i = 0; i <= mod; i++ ) { idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx) + 1; - BUG_ON(idx >= d->max_vcpus); + BUG_ON(idx > d->max_vcpus); } dest = d->vcpu[idx - 1];
The 'idx' can equal to the max number of vCPUs, fix it. Signed-off-by: Feng Wu <feng.wu@intel.com> --- xen/drivers/passthrough/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)