linux-kernel - Re: [PATCH 000 of 35] Refactor block layer to improve support for stacked devices.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070801155947.GG13674@htj.dyndns.org>
Date:	Thu, 2 Aug 2007 00:59:47 +0900
From:	Tejun Heo <htejun@...il.com>
To:	John Stoffel <john@...ffel.org>
Cc:	Tejun Heo <teheo@...e.de>, Avi Kivity <avi@...o.co.il>,
	NeilBrown <neilb@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 000 of 35] Refactor block layer to improve support for stacked devices.

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/