On Tue, Sep 09, 2008 at 08:48:39PM -0700, Darren Reed wrote:
> Location: mac.c 774-876
> Type: T
> Priority: 3
> Comment: Underlying the calls to mac_perim_enter/exit is a new
> variation on using mutex's with kcondvar_t's to implement locking.
> Whilst similar to the macros in <sys/condvar_impl.h>, it differs in
> that it allows recursion on write locking if the owning thread is
> reacquiring the same lock. What I'd like the project to consider
> is that rather than embedding this new locking style into these
> functions, break it out so that this design can be reused later as
> it seems generally useful.
>
I'll defer to thiru on this one.
> Location: mac_flow.c 1777-1795 (example)
> Type: T
> Priority: 2
> Comment: the organisation of the flow_desc_t structure should be
> revised so that instead of having two individual checks for IPv4
> and IPv6, there is a single 32bit mask and equality check that
> covers the IP version and dsfield information. This should work
> equally well for Ipv4 (with space left over for protocol matching)
> and Ipv6, where you may also be able to do protocol matching,
> so long as flow id's aren't being used. So in an appropriately
> designed structure, rather than having:
>
> 1780 switch (l3info->l3_version) {
> 1781 case IPV4_VERSION: {
> 1782 ipha_t *ipha = (ipha_t *)l3info->l3_start;
> 1783
> 1784 return ((ipha->ipha_type_of_service &
> 1785 fd->fd_dsfield_mask) == fd->fd_dsfield);
> 1786 }
> 1787 case IPV6_VERSION: {
> 1788 ip6_t *ip6h = (ip6_t *)l3info->l3_start;
> 1789
> 1790 return ((IPV6_FLOW_TCLASS(ip6h->ip6_vcf) &
> 1791 fd->fd_dsfield_mask) == fd->fd_dsfield);
> 1792 }
> 1793 default:
> 1794 return (B_FALSE);
> 1795 }
>
> You would have:
> return ((((uint32_t *)ipha) & fd->fd_l3hdrmask) == fd->fd_l3hdrs);
>
the current flow_desc_t design allows the user to specify a dsfield
without also specifying ip version. whereas your design makes the
ip version mandatory (the fd_l3hdrmask above is ip version dependent
because the tos field in ipv4 and traffic class in ipv6 exist in different
offsets)
also, I think that saving two comparisons is not worth sacrificing
readability. most fields in the flow_desc_t now have 1-1 correspondence
with user-settable attributes. we deliberately chose to make this structure
independent of header format details.
> Location: mac_flow.c 2004-2005
> Type: T
> Priority: 3
> Comment: there should be no need for the ?: here, the assignment of
> something to fe_match can be done inside the above if().
>
ACCEPT.
> Location: mac_flow.c 2089-2093
> Type: T
> Priority: 3
> Comment: If fd_ipversion isn't 4, then what else can it be but 6?
> We've already ASSERT'd that we have an IP version (2058, 2060) that
> is valid..
>
ACCEPT.
will change v6 comparison into an assert.
eric
