Avi Kivity wrote:
> NeilBrown wrote:
>> To achieve this, the "for_each" macros are now somewhat more complex.
>> For example, rq_for_each_segment is:
>>
>> #define bio_for_each_segment_offset(bv, bio, _i, offs, _size) \
>> for (_i.i = 0, _i.offset = (bio)->bi_offset + offs, \
>> _i.size = min_t(int, _size, (bio)->bi_size - offs); \
>> _i.i < (bio)->bi_vcnt && _i.size > 0; \
>> _i.i++) \
>> if (bv = *bio_iovec_idx((bio), _i.i), \
>> bv.bv_offset += _i.offset, \
>> bv.bv_len <= _i.offset \
>> ? (_i.offset -= bv.bv_len, 0) \
>> : (bv.bv_len -= _i.offset, \
>> _i.offset = 0, \
>> bv.bv_len < _i.size \
>> ? (_i.size -= bv.bv_len, 1) \
>> : (bv.bv_len = _i.size, \
>> _i.size = 0, \
>> bv.bv_len > 0)))
>>
>> #define bio_for_each_segment(bv, bio, __i) \
>> bio_for_each_segment_offset(bv, bio, __i, 0, (bio)->bi_size)
>>
>> It does some with some explanatory text in a comment, but it is still
>> a bit daunting. Any suggestions on making this more approachable
>> would be very welcome.
>>
>>
>
> Well, I hesitate to state the obvious, but how about:
>
> #define bio_for_each_segment_offset(bv, bio, _i, offs, _size) \
> for (bio_iterator_init(&_i, ...); bio_iterator_cont(&_i, ...);
> bio_iterator_advance(&_i, ...)) \
> if (bio_iterator_want_segment(&_i, ...))
>
> While this doesn't remove the complexity, at least it's readable.
Violently seconded.
--
tejun
Tejun> Avi Kivity wrote:
>> NeilBrown wrote:
>>> To achieve this, the "for_each" macros are now somewhat more complex.
>>> For example, rq_for_each_segment is:
>>>
>>> #define bio_for_each_segment_offset(bv, bio, _i, offs, _size) \
>>> for (_i.i = 0, _i.offset = (bio)->bi_offset + offs, \
>>> _i.size = min_t(int, _size, (bio)->bi_size - offs); \
>>> _i.i < (bio)->bi_vcnt && _i.size > 0; \
>>> _i.i++) \
>>> if (bv = *bio_iovec_idx((bio), _i.i), \
>>> bv.bv_offset += _i.offset, \
>>> bv.bv_len <= _i.offset \
>>> ? (_i.offset -= bv.bv_len, 0) \
>>> : (bv.bv_len -= _i.offset, \
>>> _i.offset = 0, \
>>> bv.bv_len < _i.size \
>>> ? (_i.size -= bv.bv_len, 1) \
>>> : (bv.bv_len = _i.size, \
>>> _i.size = 0, \
>>> bv.bv_len > 0)))
>>>
>>> #define bio_for_each_segment(bv, bio, __i) \
>>> bio_for_each_segment_offset(bv, bio, __i, 0, (bio)->bi_size)
>>>
>>> It does some with some explanatory text in a comment, but it is still
>>> a bit daunting. Any suggestions on making this more approachable
>>> would be very welcome.
>>>
>>>
>>
>> Well, I hesitate to state the obvious, but how about:
>>
>> #define bio_for_each_segment_offset(bv, bio, _i, offs, _size) \
>> for (bio_iterator_init(&_i, ...); bio_iterator_cont(&_i, ...);
>> bio_iterator_advance(&_i, ...)) \
>> if (bio_iterator_want_segment(&_i, ...))
>>
>> While this doesn't remove the complexity, at least it's readable.
Tejun> Violently seconded.
How about it be made into a real function instead? I was reading
through the patch, but got timed out yesterday, so take this with a
grain of salt.
I thought I saw a couple of macros defined to use this macro yet
again. Which I figured might be a problem is the passed in variables
get munged.
In any case, why does something so complicated need to be a macro, why
not a function instead?
John
On Wed, Aug 01, 2007 at 11:52:35AM -0400, John Stoffel wrote:
>
> Tejun> Avi Kivity wrote:
> >> NeilBrown wrote:
> >>> To achieve this, the "for_each" macros are now somewhat more complex.
> >>> For example, rq_for_each_segment is:
> >>>
> >>> #define bio_for_each_segment_offset(bv, bio, _i, offs, _size) \
> >>> for (_i.i = 0, _i.offset = (bio)->bi_offset + offs, \
> >>> _i.size = min_t(int, _size, (bio)->bi_size - offs); \
> >>> _i.i < (bio)->bi_vcnt && _i.size > 0; \
> >>> _i.i++) \
> >>> if (bv = *bio_iovec_idx((bio), _i.i), \
> >>> bv.bv_offset += _i.offset, \
> >>> bv.bv_len <= _i.offset \
> >>> ? (_i.offset -= bv.bv_len, 0) \
> >>> : (bv.bv_len -= _i.offset, \
> >>> _i.offset = 0, \
> >>> bv.bv_len < _i.size \
> >>> ? (_i.size -= bv.bv_len, 1) \
> >>> : (bv.bv_len = _i.size, \
> >>> _i.size = 0, \
> >>> bv.bv_len > 0)))
> >>>
> >>> #define bio_for_each_segment(bv, bio, __i) \
> >>> bio_for_each_segment_offset(bv, bio, __i, 0, (bio)->bi_size)
> >>>
> >>> It does some with some explanatory text in a comment, but it is still
> >>> a bit daunting. Any suggestions on making this more approachable
> >>> would be very welcome.
> >>>
> >>>
> >>
> >> Well, I hesitate to state the obvious, but how about:
> >>
> >> #define bio_for_each_segment_offset(bv, bio, _i, offs, _size) \
> >> for (bio_iterator_init(&_i, ...); bio_iterator_cont(&_i, ...);
> >> bio_iterator_advance(&_i, ...)) \
> >> if (bio_iterator_want_segment(&_i, ...))
> >>
> >> While this doesn't remove the complexity, at least it's readable.
>
> Tejun> Violently seconded.
>
> How about it be made into a real function instead? I was reading
> through the patch, but got timed out yesterday, so take this with a
> grain of salt.
>
> I thought I saw a couple of macros defined to use this macro yet
> again. Which I figured might be a problem is the passed in variables
> get munged.
>
> In any case, why does something so complicated need to be a macro, why
> not a function instead?
I agree and actually wrote about the same opinion in one of the
replies. It might even be benefitial performance-wise due to smaller
cache foot print.
Thanks.
--
tejun