Love, Robert W wrote:
> FYI: There is a defect in the kernel's QoS layer where
> kernel filters fail to be found (matched) when removing
> or modifying them. This problem exists with our current
> Filter (FCoE), but was really brought to my attention with
> the FIP filter, which, with the defect, can never be removed
> or modified.
>
> I have a patch to fix the kernel (one of two possble
> aproaches) and I will follow up with netdev to resolve
> this issue.
>
> Robert Love wrote:
>> FIP code has been added to the kernel and we want the
>> QoS layer in the kernel to filter FIP traffic into the
>> DCB negotiated traffic class. This patch addd an extra
>> 'tc' call to fcoeplumb to filter the FIP packets.
>>
>> This patch makes many of the filter functions generic
>> to work with multiple filters (one per ethertype) instead
>> of assuming that FCoE is the only desired filter.
>>
>> This patch also fixes three defects-
>>
>> 1) incorrect argument parsing
>>
>> delete_skbedit_filter()
>> {
>> ifname=$1
>> - queue=$?
>> + queue=$2
>>
>> $? doesn't make sense here
>>
>> 2) incorrect awk regex
>>
>> The first check in the find_skbedit_filter() was searching for
>> /^filter.*parent.*protocol '$ethertype' ....
>>
>> However, there are squared brackets around the ethertype. Since
>> the regex was searching for "protocol"<space><ethtype> it was
>> not matching "protocol"<space>[<ethertype>
>>
>> This patch adds the brackets to the check.
>>
>> 3) When no qdisc was found the script would add a qdisc
>> and filter, but then immediately delete the newly added
>> qdisc. It would then re-add it and the filters again. This
>> was unnecessary, so I removed it.
>>
>> It was explained to me that this was a workaround for a
>> 'tc' defect, but with the latest version of iproute2
>> this workaround is not needed.
>> ---
>>
>> fcoeplumb.in | 104
>> +++++++++++++++++++++++++++++++++++++--------------------- 1 files
>> changed, 66 insertions(+), 38 deletions(-)
>>
>> diff --git a/fcoeplumb.in b/fcoeplumb.in
>> index a9ae9a8..edff338 100755
>> --- a/fcoeplumb.in
>> +++ b/fcoeplumb.in
>> @@ -32,10 +32,14 @@ usage()
>> #
>> QOS_DEF=3 # default user priority
>> FCOE_ETHERTYPE=35078 # Ethertype (0x8906): tc filter show is
>> base 10
>> +FIP_ETHERTYPE=35092 # Ethertype (0x8914): tc filter show
>> is base 10 FCOE_FILTER=0xfc0e # filter handle (must be
>> lower-case
>> hex) +
>> +FCOE_FILTER_KEY=12345
>> +FIP_FILTER_KEY=67890
>> +
>> qdisc_id=1:
>> qos_list=
>> -FILTER_ID=
>> cmd=
>> # automake paths
>> pref...@prefix@
>> @@ -83,18 +87,10 @@ add_multiq_qdisc()
>> tc qdisc add dev $ifname root handle $qdisc_id multiq
>> }
>>
>> -delete_qdisc()
>> -{
>> - ifname=$1
>> -
>> - [ "$DEBUG" = "yes" ] && $LOGGER \
>> - "tc qdisc del dev $ifname root"
>> - tc qdisc del dev $ifname root
>> -}
>> -
>> get_filter_id()
>> {
>> ifname=$1
>> + filter_key=$2
>>
>> retry_count=0
>> while true
>> @@ -105,13 +101,15 @@ get_filter_id()
>> retry_count=$(($retry_count-1))
>> done
>>
>> - FILTER_ID=`echo "$ifname 12345" | \
>> - awk '{ printf("0x%x%06x", substr($1,4), $2) }'`
>> + echo "`echo "$ifname $filter_key" | \
>> + awk '{ printf("0x%x%06x", substr($1,4), $2) }'`"
>> }
>>
>> find_skbedit_filter()
>> {
>> ifname=$1
>> + ethertype=$2
>> + filter_id=$3
>>
>> found=`tc filter show dev $ifname | awk '
>> BEGIN {
>> @@ -120,11 +118,11 @@ find_skbedit_filter()
>> x3 = 0
>> queue = 8
>> }
>> - /^filter.*parent.*protocol '$FCOE_ETHERTYPE'.* handle '$FILTER_ID'/
>> { + /^filter.*parent.*protocol \['$ethertype'\].* handle
>> '$filter_id'/ { if (x1 == 0 && x2 == 0 && x3 == 0)
>> x1 = 1
>> }
>> - /cmp.*u16 at 12 layer 1 mask 0xffff eq '$FCOE_ETHERTYPE'.*\)/ {
>> + /cmp.*u16 at 12 layer 1 mask 0xffff eq '$ethertype'.*\)/ {
Now since we're matching on the
protocol, I don't think we need to do the equivalent compare of the
"u16 at 12", which is also the ethertype. That was only necessary before
tc could take arbitrary ethertypes. Of course, this doesn't do any harm
as far as I can tell.
I haven't had time to go over the rest of the patch.
>> if (x1 == 1 && x2 == 0 && x3 == 0)
>> x2 = 1
>> }
>> @@ -144,22 +142,24 @@ find_skbedit_filter()
>> delete_skbedit_filter()
>> {
>> ifname=$1
>> - queue=$?
>> + queue=$2
>> + ethertype=$3
>> + filter_id=$4
>>
>> [ "$DEBUG" = "yes" ] && $LOGGER \
>> "tc filter delete dev $ifname skbedit queue_mapping $queue"
>> PARENT=`tc filter show dev $ifname | awk \
>> - '/^filter.*parent.*protocol '$FCOE_ETHERTYPE'.* handle
>> '$FILTER_ID'/ \ + '/^filter.*parent.*protocol \['$ethertype'\].*
>> handle '$filter_id'/ \ { print $3 }'`
>> PRIO=`tc filter show dev $ifname | awk \
>> - '/^filter.*parent.*protocol '$FCOE_ETHERTYPE'.* handle
>> '$FILTER_ID'/ \ + '/^filter.*parent.*protocol \['$ethertype'\].*
>> handle '$filter_id'/ \ { print $7 }'`
>> tc filter delete dev $ifname parent $PARENT \
>> - protocol $FCOE_ETHERTYPE pref $PRIO handle $FILTER_ID basic
>> match \
>> - 'cmp(u16' at 12 layer 1 mask 0xffff eq $FCOE_ETHERTYPE')' \
>> + protocol $ethertype pref $PRIO handle $filter_id basic match \
>> + 'cmp(u16' at 12 layer 1 mask 0xffff eq $ethertype')' \
>> action skbedit queue_mapping $queue
>> tc filter delete dev $ifname parent $PARENT \
>> - protocol $FCOE_ETHERTYPE pref $PRIO basic
>> + protocol $ethertype pref $PRIO basic
>> }
>>
>> add_skbedit_filter()
>> @@ -167,12 +167,14 @@ add_skbedit_filter()
>> ifname=$1
>> qdisc_id=$2
>> queue=$3
>> + ethertype=$4
>> + filter_id=$5
>>
>> [ "$DEBUG" = "yes" ] && $LOGGER \
>> "tc filter add dev $ifname skbedit queue_mapping $queue"
>> - tc filter add dev $ifname parent $qdisc_id protocol $FCOE_ETHERTYPE
>> \
>> - handle $FILTER_ID basic match 'cmp(u16' at 12 \
>> - layer 1 mask 0xffff eq $FCOE_ETHERTYPE')' \
>> + tc filter add dev $ifname parent $qdisc_id protocol $ethertype \
>> + handle $filter_id basic match 'cmp(u16' at 12 \
>> + layer 1 mask 0xffff eq $ethertype')' \
>> action skbedit queue_mapping $queue
>> }
>>
>> @@ -180,18 +182,20 @@ replace_skbedit_filter()
>> {
>> ifname=$1
>> queue=$2
>> + ethertype=$3
>> + filter_id=$4
>>
>> [ "$DEBUG" = "yes" ] && $LOGGER \
>> "tc filter replace dev $ifname skbedit queue_mapping $queue"
>> PARENT=`tc filter show dev $ifname | awk \
>> - '/^filter.*parent.*protocol '$FCOE_ETHERTYPE'.* handle
>> '$FILTER_ID'/ \ + '/^filter.*parent.*protocol \['$ethertype'\].*
>> handle '$filter_id'/ \ { print $3 }'`
>> PRIO=`tc filter show dev $ifname | \
>> - awk '/^filter.*parent.*protocol '$FCOE_ETHERTYPE'.* handle
>> '$FILTER_ID'/ \ + awk '/^filter.*parent.*protocol
>> \['$ethertype'\].*
>> handle '$filter_id'/ \ { print $7 }'`
>> tc filter replace dev $ifname parent $PARENT protocol \
>> - $FCOE_ETHERTYPE pref $PRIO handle $FILTER_ID basic match \
>> - 'cmp(u16' at 12 layer 1 mask 0xffff eq $FCOE_ETHERTYPE')' \
>> + $ethertype pref $PRIO handle $filter_id basic match \
>> + 'cmp(u16' at 12 layer 1 mask 0xffff eq $ethertype')' \
>> action skbedit queue_mapping $queue
>> }
>>
>> @@ -261,15 +265,23 @@ do
>> done
>>
>> # This must be the first to do after parsing the command arguments!
>> -# Notice that the FILTER_ID is used in find_skbedit_filter(),
>> +# Notice that the filter ID is used in find_skbedit_filter(),
>> # add_skbedit_filter(), replace_skbedit_filter().
>> -get_filter_id $ifname
>> +fcoe_filter_id=`get_filter_id $ifname $FCOE_FILTER_KEY`
>> +fip_filter_id=`get_filter_id $ifname $FIP_FILTER_KEY`
>>
>> if [ "$cmd" == "disable" ]; then
>> remove_fcoe_interface $ifname
>> - find_skbedit_filter $ifname
>> + # Remove the FCoE filters
>> + find_skbedit_filter $ifname $FCOE_ETHERTYPE $fcoe_filter_id
>> found_filter=$?
>> - [ $found_filter -le 7 ] && delete_skbedit_filter $ifname
>> $found_filter + [ $found_filter -le 7 ] && delete_skbedit_filter
>> $ifname $found_filter $FCOE_ETHERTYPE $fcoe_filter_id +
>> + # Remove the FIP filters
>> + find_skbedit_filter $ifname $FIP_ETHERTYPE $fip_filter_id
>> + found_filter=$?
>> + [ $found_filter -le 7 ] && delete_skbedit_filter $ifname
>> $found_filter $FIP_ETHERTYPE $fip_filter_id +
>> else
>> #
>> # Choose the best QOS to use for FCoE out of the listed choices.
>> @@ -317,25 +329,41 @@ else
>> found_qdisc=$?
>>
>> if [ $found_qdisc -eq 1 ]; then
>> + # Adjust the FCoE filter
>> [ "$DEBUG" = "yes" ] && $LOGGER "$ifname: Qdisc is found"
>> - find_skbedit_filter $ifname
>> + find_skbedit_filter $ifname $FCOE_ETHERTYPE $fcoe_filter_id
>> found_filter=$?
>> if [ $found_filter -gt 7 ]; then
>> - add_skbedit_filter $ifname $qdisc_id $qos_queue
>> + add_skbedit_filter $ifname $qdisc_id $qos_queue \
>> + $FCOE_ETHERTYPE $fcoe_filter_id
>> elif [ $found_filter -ne $qos_queue ]; then
>> [ "$DEBUG" = "yes" ] && $LOGGER \
>> "$ifname: Filter is found and QOS is different"
>> - replace_skbedit_filter $ifname $qos_queue
>> + replace_skbedit_filter $ifname $qos_queue
>> $FCOE_ETHERTYPE
>> $fcoe_filter_id + else
>> + [ "$DEBUG" = "yes" ] && $LOGGER \
>> + "$ifname: Filter is found and is identical"
>> + fi
>> +
>> + # Adjust the FIP filter
>> + [ "$DEBUG" = "yes" ] && $LOGGER "$ifname: Qdisc is found"
>> + find_skbedit_filter $ifname $FIP_ETHERTYPE $fip_filter_id
>> + found_filter=$?
>> + if [ $found_filter -gt 7 ]; then
>> + add_skbedit_filter $ifname $qdisc_id $qos_queue \
>> + $FIP_ETHERTYPE $fip_filter_id
>> + elif [ $found_filter -ne $qos_queue ]; then
>> + [ "$DEBUG" = "yes" ] && $LOGGER \
>> + "$ifname: Filter is found and QOS is different"
>> + replace_skbedit_filter $ifname $qos_queue $FIP_ETHERTYPE
>> $fip_filter_id else
>> [ "$DEBUG" = "yes" ] && $LOGGER \
>> "$ifname: Filter is found and is identical"
>> fi
>> else
>> add_multiq_qdisc $ifname $qdisc_id
>> - add_skbedit_filter $ifname $qdisc_id $qos_queue
>> - delete_qdisc $ifname
>> - add_multiq_qdisc $ifname $qdisc_id
>> - add_skbedit_filter $ifname $qdisc_id $qos_queue
>> + add_skbedit_filter $ifname $qdisc_id $qos_queue $FCOE_ETHERTYPE
>> $fcoe_filter_id + add_skbedit_filter $ifname $qdisc_id $qos_queue
>> $FIP_ETHERTYPE $fip_filter_id fi
>>
>> if [ "$cmd" = "enable" ]; then
>>
>> _______________________________________________
>> devel mailing list
>> [email protected]
>> http://www.open-fcoe.org/mailman/listinfo/devel
>
> _______________________________________________
> devel mailing list
> [email protected]
> http://www.open-fcoe.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
