Date:	Mon, 27 Jun 2011 10:43:53 -0400
From:	Sebastian.Poehn@...den.com
To:	joe@...ches.com
Cc:	"netdev" <netdev@...r.kernel.org>
Subject: Re: [PATCH] gianfar v5: implement nfc

Thanks for your feedback Joe!

> I'm a bit confused by this:

/*
 * What we already have:
 * A temporary copy of the filer table which will later replace the current one.
 * In the filer table we always have one mask entry followed by a value (of action).
 * Further we have the mask table which is a list of all mask in the filer table.
 * To every entry we have the start and the end of the validity and a block index
 * (same block indexes means dependency). We want to sort every dependent block!
 */
static void gfar_sort_mask_table(struct gfar_mask_entry *mask_table,
        struct filer_table *temp_table, u32 and_index)
{
    /* Pointer to compare function (_asc or _desc) */
    int (*gfar_comp)(const void *, const void *);

    u32 i, size = 0, start = 0, prev = 1;
    u32 old_first, old_last, new_first, new_last;

    gfar_comp = &gfar_comp_desc;

    for (i = 0; i < and_index; i++) {
        /* As long, as the block is the same (dependency) we only increase
         * the size */
        if (prev != mask_table[i].block) {
            /* The +1 is because  start is a mask and the important
             * FF80 information is located in the next value element */
            old_first = mask_table[start].start + 1;
            /* -1 because we are already at the start of the next dependent block */
            old_last = mask_table[i - 1].end;
            
            /* Here the mask table is sorted! So the entries in the current
             * block change may change their location */
            sort(mask_table + start, size,
                    sizeof(struct gfar_mask_entry),
                    gfar_comp, &gfar_swap);

            /* Toggle order for every block. This makes the
             * thing more efficient! */
            if (gfar_comp == gfar_comp_desc)
                gfar_comp = &gfar_comp_asc;
            else
                gfar_comp = &gfar_comp_desc;
            /* The same as above but the elements are others because of the sorting */
            
            new_first = mask_table[start].start + 1;
            new_last = mask_table[i - 1].end;

            gfar_swap_bits(&temp_table->fe[new_first],
                    &temp_table->fe[old_first],
                    &temp_table->fe[new_last],
                    &temp_table->fe[old_last],
                    RQFCR_QUEUE | RQFCR_CLE |
                        RQFCR_RJE | RQFCR_AND
                    );

            start = i;
            size = 0;
        }
        size++;
        prev = mask_table[i].block;
    }

}

> Doesn't this just swap the same bits around twice?

Due to the masks are sorted numerically it is possible that a mask from any location changes its locations to ANY. Lets think of this example and the usage of the sequential swapping:

A has a header     <- old_first               new position after sorting: 2
B                  <- new_last                new position after sorting: 4
C                                             new position after sorting: 3
D has a trailer    <- old_last and new_first  new position after sorting: 1

First swap first:
A has a trailer
B
C
D has a header

Second swap:
A has a trailer
B has a header
C
D

And now after the sorted order is applied:
D
A has a trailer
C
B has a header

Thats not correct!

The concurrent swapper does:
A
B has a trailer
C
D has a header

And:
D has a header
A
C
B has a trailer

That's all right!








DISCLAIMER:

Privileged and/or Confidential information may be contained in this
message. If you are not the addressee of this message, you may not
copy, use or deliver this message to anyone. In such event, you
should destroy the message and kindly notify the sender by reply
e-mail. It is understood that opinions or conclusions that do not
relate to the official business of the company are neither given
nor endorsed by the company.

Thank You.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists