| Message ID | 4DC1883F.7050301@chelsio.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote: > On 05/03/2011 04:34 PM, Ben Hutchings wrote: > > On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote: > >> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is > >> enough knowledge to pick an appropriate slot. So I'd remove the > >> > >> if (loc == RX_CLS_LOC_UNSPEC) > >> > >> block above, let the driver pick a slot, and then pass the selected location > >> back for ethtool to report. > > > > But first we have to specify this in the ethtool API. So please propose > > a patch to ethtool.h. > > In the past we discussed that being able to specify the first available slot or > the last available would be useful, so something like the below? > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 4194a20..909ef79 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -442,7 +442,8 @@ struct ethtool_flow_ext { > * includes the %FLOW_EXT flag. > * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC > * if packets should be discarded > - * @location: Index of filter in hardware table > + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for > + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available [...] I think that's reasonable. We should also explicitly state that location determines priority, i.e. if a packet matches two filters then the one with the lower location wins. Ben.
On 05/04/2011 10:24 AM, Ben Hutchings wrote: > On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote: >> On 05/03/2011 04:34 PM, Ben Hutchings wrote: >>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote: >>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is >>>> enough knowledge to pick an appropriate slot. So I'd remove the >>>> >>>> if (loc == RX_CLS_LOC_UNSPEC) >>>> >>>> block above, let the driver pick a slot, and then pass the selected location >>>> back for ethtool to report. >>> But first we have to specify this in the ethtool API. So please propose >>> a patch to ethtool.h. >> In the past we discussed that being able to specify the first available slot or >> the last available would be useful, so something like the below? >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index 4194a20..909ef79 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -442,7 +442,8 @@ struct ethtool_flow_ext { >> * includes the %FLOW_EXT flag. >> * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC >> * if packets should be discarded >> - * @location: Index of filter in hardware table >> + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for >> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available > [...] > > I think that's reasonable. We should also explicitly state that > location determines priority, i.e. if a packet matches two filters then > the one with the lower location wins. Easy and true for a TCAM. For hashing would you use the location to decide how to order filters that fall in the same bucket? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/4/2011 10:33 AM, Dimitris Michailidis wrote: > On 05/04/2011 10:24 AM, Ben Hutchings wrote: >> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote: >>> On 05/03/2011 04:34 PM, Ben Hutchings wrote: >>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote: >>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is >>>>> enough knowledge to pick an appropriate slot. So I'd remove the >>>>> >>>>> if (loc == RX_CLS_LOC_UNSPEC) >>>>> >>>>> block above, let the driver pick a slot, and then pass the selected location >>>>> back for ethtool to report. >>>> But first we have to specify this in the ethtool API. So please propose >>>> a patch to ethtool.h. >>> In the past we discussed that being able to specify the first available slot or >>> the last available would be useful, so something like the below? >>> >>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >>> index 4194a20..909ef79 100644 >>> --- a/include/linux/ethtool.h >>> +++ b/include/linux/ethtool.h >>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext { >>> * includes the %FLOW_EXT flag. >>> * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC >>> * if packets should be discarded >>> - * @location: Index of filter in hardware table >>> + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for >>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available >> [...] >> >> I think that's reasonable. We should also explicitly state that >> location determines priority, i.e. if a packet matches two filters then >> the one with the lower location wins. > > Easy and true for a TCAM. For hashing would you use the location to decide how > to order filters that fall in the same bucket? > The problem is none of this is backwards compatible. The niu driver has supported the network flow classifier rules since 2.6.30. Adding this would cause all rule setups for niu to fail because these locations would have to exist outside of the current rule locations. This is why I was suggesting that the best approach would be to update the kernel to add a separate ioctl for letting the driver setup the location. We can just attempt to make that call and when we get the EOPNOTSUPP errno we know the device driver doesn't support it and can then let the rule manager take over. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-05-04 at 10:41 -0700, Alexander Duyck wrote: > On 5/4/2011 10:33 AM, Dimitris Michailidis wrote: > > On 05/04/2011 10:24 AM, Ben Hutchings wrote: > >> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote: > >>> On 05/03/2011 04:34 PM, Ben Hutchings wrote: > >>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote: > >>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is > >>>>> enough knowledge to pick an appropriate slot. So I'd remove the > >>>>> > >>>>> if (loc == RX_CLS_LOC_UNSPEC) > >>>>> > >>>>> block above, let the driver pick a slot, and then pass the selected location > >>>>> back for ethtool to report. > >>>> But first we have to specify this in the ethtool API. So please propose > >>>> a patch to ethtool.h. > >>> In the past we discussed that being able to specify the first available slot or > >>> the last available would be useful, so something like the below? > >>> > >>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > >>> index 4194a20..909ef79 100644 > >>> --- a/include/linux/ethtool.h > >>> +++ b/include/linux/ethtool.h > >>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext { > >>> * includes the %FLOW_EXT flag. > >>> * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC > >>> * if packets should be discarded > >>> - * @location: Index of filter in hardware table > >>> + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for > >>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available > >> [...] > >> > >> I think that's reasonable. We should also explicitly state that > >> location determines priority, i.e. if a packet matches two filters then > >> the one with the lower location wins. > > > > Easy and true for a TCAM. For hashing would you use the location to decide how > > to order filters that fall in the same bucket? > > > > The problem is none of this is backwards compatible. The niu driver has > supported the network flow classifier rules since 2.6.30. Adding this > would cause all rule setups for niu to fail because these locations > would have to exist outside of the current rule locations. Those rule setups would have to be using some unofficial patched ethtool. I don't think that should be a major concern for us. However... > This is why I was suggesting that the best approach would be to update > the kernel to add a separate ioctl for letting the driver setup the > location. > > We can just attempt to make that call and when we get the > EOPNOTSUPP errno we know the device driver doesn't support it and can > then let the rule manager take over. How about having ETHTOOL_GRXCLSRLCNT set a flag in the 'data' field to indicate that the driver can assign locations? (We would have to specify that for compatibility with older kernels the application must initialise this filed to 0.) rmgr_init() would then check for this flag. I hope someone is actually going to test this on niu, as it sounds like that will be the only driver using a TCAM... David? Ben.
On 05/04/2011 10:41 AM, Alexander Duyck wrote: > On 5/4/2011 10:33 AM, Dimitris Michailidis wrote: >> On 05/04/2011 10:24 AM, Ben Hutchings wrote: >>> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote: >>>> On 05/03/2011 04:34 PM, Ben Hutchings wrote: >>>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote: >>>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where >>>>>> there is >>>>>> enough knowledge to pick an appropriate slot. So I'd remove the >>>>>> >>>>>> if (loc == RX_CLS_LOC_UNSPEC) >>>>>> >>>>>> block above, let the driver pick a slot, and then pass the >>>>>> selected location >>>>>> back for ethtool to report. >>>>> But first we have to specify this in the ethtool API. So please >>>>> propose >>>>> a patch to ethtool.h. >>>> In the past we discussed that being able to specify the first >>>> available slot or >>>> the last available would be useful, so something like the below? >>>> >>>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >>>> index 4194a20..909ef79 100644 >>>> --- a/include/linux/ethtool.h >>>> +++ b/include/linux/ethtool.h >>>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext { >>>> * includes the %FLOW_EXT flag. >>>> * @ring_cookie: RX ring/queue index to deliver to, or >>>> %RX_CLS_FLOW_DISC >>>> * if packets should be discarded >>>> - * @location: Index of filter in hardware table >>>> + * @location: Index of filter in hardware table, or >>>> %RX_CLS_FLOW_FIRST_LOC for >>>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last >>>> available >>> [...] >>> >>> I think that's reasonable. We should also explicitly state that >>> location determines priority, i.e. if a packet matches two filters then >>> the one with the lower location wins. >> >> Easy and true for a TCAM. For hashing would you use the location to >> decide how >> to order filters that fall in the same bucket? >> > > The problem is none of this is backwards compatible. The niu driver has > supported the network flow classifier rules since 2.6.30. Adding this > would cause all rule setups for niu to fail because these locations > would have to exist outside of the current rule locations. Looking at niu it already has a problem with its handling of location because it does u16 idx; idx = nfc->fs.location; i.e., it disregards the upper 16 bits of location. With the current code niu would return -EINVAL for the two new constants, which seems correct. > > This is why I was suggesting that the best approach would be to update > the kernel to add a separate ioctl for letting the driver setup the > location. We can just attempt to make that call and when we get the > EOPNOTSUPP errno we know the device driver doesn't support it and can > then let the rule manager take over. The problem with this is the location is dependent on the type of filter being added. I.e., the ioctl would need to get all the information the existing ioctl carries making the new ioctl a small superset of the current one. Additionally, if the driver only allocates a location in a separate ioctl how does it know that the app is actually going to use it? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/4/2011 11:05 AM, Ben Hutchings wrote: > On Wed, 2011-05-04 at 10:41 -0700, Alexander Duyck wrote: >> On 5/4/2011 10:33 AM, Dimitris Michailidis wrote: >>> On 05/04/2011 10:24 AM, Ben Hutchings wrote: >>>> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote: >>>>> On 05/03/2011 04:34 PM, Ben Hutchings wrote: >>>>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote: >>>>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is >>>>>>> enough knowledge to pick an appropriate slot. So I'd remove the >>>>>>> >>>>>>> if (loc == RX_CLS_LOC_UNSPEC) >>>>>>> >>>>>>> block above, let the driver pick a slot, and then pass the selected location >>>>>>> back for ethtool to report. >>>>>> But first we have to specify this in the ethtool API. So please propose >>>>>> a patch to ethtool.h. >>>>> In the past we discussed that being able to specify the first available slot or >>>>> the last available would be useful, so something like the below? >>>>> >>>>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >>>>> index 4194a20..909ef79 100644 >>>>> --- a/include/linux/ethtool.h >>>>> +++ b/include/linux/ethtool.h >>>>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext { >>>>> * includes the %FLOW_EXT flag. >>>>> * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC >>>>> * if packets should be discarded >>>>> - * @location: Index of filter in hardware table >>>>> + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for >>>>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available >>>> [...] >>>> >>>> I think that's reasonable. We should also explicitly state that >>>> location determines priority, i.e. if a packet matches two filters then >>>> the one with the lower location wins. >>> >>> Easy and true for a TCAM. For hashing would you use the location to decide how >>> to order filters that fall in the same bucket? >>> >> >> The problem is none of this is backwards compatible. The niu driver has >> supported the network flow classifier rules since 2.6.30. Adding this >> would cause all rule setups for niu to fail because these locations >> would have to exist outside of the current rule locations. > > Those rule setups would have to be using some unofficial patched > ethtool. I don't think that should be a major concern for us. > However... No, what I am saying is that if we were to add those locations to the ETHTOOL_SRXCLSRLINS then niu would not work anymore as it would treat them as invalid locations. This existing setup will at least work with the existing niu from what I can tell. If we were to try adding rules with locations outside of actual allowable rules then we would likely receive an indication that we provided an invalid argument. >> This is why I was suggesting that the best approach would be to update >> the kernel to add a separate ioctl for letting the driver setup the >> location. >> >> We can just attempt to make that call and when we get the >> EOPNOTSUPP errno we know the device driver doesn't support it and can >> then let the rule manager take over. > > How about having ETHTOOL_GRXCLSRLCNT set a flag in the 'data' field to > indicate that the driver can assign locations? (We would have to > specify that for compatibility with older kernels the application must > initialise this filed to 0.) > > rmgr_init() would then check for this flag. > > I hope someone is actually going to test this on niu, as it sounds like > that will be the only driver using a TCAM... David? > > Ben. > Honestly what I would prefer to see is a seperate call added such as an ETHTOOL_GRSCLSRLLOC that we could pass the flow specifier to and perhaps include first/last location call in that and then let the driver return where it wants to drop the rule. That way we can avoid having to create an overly complicated rule manager that can handle all the bizarre rule ordering options that I am sure all the different network devices support. The only reason I am not implementing this now is because there aren't any drivers in place that would currently use it. I figure once cxgb has a means in place of supporting flow classifier rules then Dimitris can add the necessary code to ethtool and the kernel to allow the driver to specify rule locations. I would prefer to avoid adding features based on speculation of what will be needed and would like to be able to actually see how the features will be used. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/4/2011 11:18 AM, Dimitris Michailidis wrote: > On 05/04/2011 10:41 AM, Alexander Duyck wrote: >> On 5/4/2011 10:33 AM, Dimitris Michailidis wrote: >>> On 05/04/2011 10:24 AM, Ben Hutchings wrote: >>>> On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote: >>>>> On 05/03/2011 04:34 PM, Ben Hutchings wrote: >>>>>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote: >>>>>>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where >>>>>>> there is >>>>>>> enough knowledge to pick an appropriate slot. So I'd remove the >>>>>>> >>>>>>> if (loc == RX_CLS_LOC_UNSPEC) >>>>>>> >>>>>>> block above, let the driver pick a slot, and then pass the >>>>>>> selected location >>>>>>> back for ethtool to report. >>>>>> But first we have to specify this in the ethtool API. So please >>>>>> propose >>>>>> a patch to ethtool.h. >>>>> In the past we discussed that being able to specify the first >>>>> available slot or >>>>> the last available would be useful, so something like the below? >>>>> >>>>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >>>>> index 4194a20..909ef79 100644 >>>>> --- a/include/linux/ethtool.h >>>>> +++ b/include/linux/ethtool.h >>>>> @@ -442,7 +442,8 @@ struct ethtool_flow_ext { >>>>> * includes the %FLOW_EXT flag. >>>>> * @ring_cookie: RX ring/queue index to deliver to, or >>>>> %RX_CLS_FLOW_DISC >>>>> * if packets should be discarded >>>>> - * @location: Index of filter in hardware table >>>>> + * @location: Index of filter in hardware table, or >>>>> %RX_CLS_FLOW_FIRST_LOC for >>>>> + * first available index, or %RX_CLS_FLOW_LAST_LOC for last >>>>> available >>>> [...] >>>> >>>> I think that's reasonable. We should also explicitly state that >>>> location determines priority, i.e. if a packet matches two filters then >>>> the one with the lower location wins. >>> >>> Easy and true for a TCAM. For hashing would you use the location to >>> decide how >>> to order filters that fall in the same bucket? >>> >> >> The problem is none of this is backwards compatible. The niu driver has >> supported the network flow classifier rules since 2.6.30. Adding this >> would cause all rule setups for niu to fail because these locations >> would have to exist outside of the current rule locations. > > Looking at niu it already has a problem with its handling of location because > it does > > u16 idx; > > idx = nfc->fs.location; > > i.e., it disregards the upper 16 bits of location. With the current code niu > would return -EINVAL for the two new constants, which seems correct. Actually it looks like this is going to trigger multiple bugs. Specifically since the upper 16 bits are being discarded it is possible to specify a value such as 0x10001 hex and it will misread that as 0x0001. This is something that will probably need to be fixed in niu. >> >> This is why I was suggesting that the best approach would be to update >> the kernel to add a separate ioctl for letting the driver setup the >> location. We can just attempt to make that call and when we get the >> EOPNOTSUPP errno we know the device driver doesn't support it and can >> then let the rule manager take over. > > The problem with this is the location is dependent on the type of filter being > added. I.e., the ioctl would need to get all the information the existing > ioctl carries making the new ioctl a small superset of the current one. > Additionally, if the driver only allocates a location in a separate ioctl how > does it know that the app is actually going to use it? It doesn't know that the application is actually going to use it. What should happen is that the location should be verified by the driver when it is used in the rule insertion call. After all it is fully possible for the user to specify a location out of range since the insert call does no validation in ethtool if the user specified the location. That responsibility now lies with the driver. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-05-04 at 11:21 -0700, Alexander Duyck wrote: [...] > Honestly what I would prefer to see is a seperate call added such as an > ETHTOOL_GRSCLSRLLOC that we could pass the flow specifier to and perhaps > include first/last location call in that and then let the driver return > where it wants to drop the rule. This must not be done as a separate operation because it's racy (in fact that's an inherent problem with the rule manager). In the sfc driver (and probably others in future) filters could be inserted for RFS at any time. > That way we can avoid having to create > an overly complicated rule manager that can handle all the bizarre rule > ordering options that I am sure all the different network devices support. Right, the rule manager can't implement that. > The only reason I am not implementing this now is because there aren't > any drivers in place that would currently use it. I figure once cxgb > has a means in place of supporting flow classifier rules then Dimitris > can add the necessary code to ethtool and the kernel to allow the driver > to specify rule locations. I would prefer to avoid adding features > based on speculation of what will be needed and would like to be able to > actually see how the features will be used. If you are going to implement the same interface in ixgbe as in niu (modulo bugs), then I have no objection to going ahead with this and then adding the option for driver-assigned locations later. Please can you confirm that the location specified for ETHTOOL_SRXCLSRLINS will indeed be used as a priority in case of overlapping filters? Ben.
On 05/04/2011 11:35 AM, Alexander Duyck wrote: > On 5/4/2011 11:18 AM, Dimitris Michailidis wrote: >> On 05/04/2011 10:41 AM, Alexander Duyck wrote: >>> This is why I was suggesting that the best approach would be to update >>> the kernel to add a separate ioctl for letting the driver setup the >>> location. We can just attempt to make that call and when we get the >>> EOPNOTSUPP errno we know the device driver doesn't support it and can >>> then let the rule manager take over. >> >> The problem with this is the location is dependent on the type of >> filter being >> added. I.e., the ioctl would need to get all the information the >> existing >> ioctl carries making the new ioctl a small superset of the current one. >> Additionally, if the driver only allocates a location in a separate >> ioctl how >> does it know that the app is actually going to use it? > > It doesn't know that the application is actually going to use it. What > should happen is that the location should be verified by the driver when > it is used in the rule insertion call. After all it is fully possible > for the user to specify a location out of range since the insert call > does no validation in ethtool if the user specified the location. That > responsibility now lies with the driver. That's not the problem I was pointing out. Of course the driver will verify the location it is given. The problem is if you have a separate ioctl call that only reserves a location (and it needs to reserve otherwise several of these calls can get the same value), if you don't use it you leave the driver with orphan reservations. Imagine you hit ^C between the two ioctls in ethtool. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/04/2011 11:05 AM, Ben Hutchings wrote: > How about having ETHTOOL_GRXCLSRLCNT set a flag in the 'data' field to > indicate that the driver can assign locations? (We would have to > specify that for compatibility with older kernels the application must > initialise this filed to 0.) > > rmgr_init() would then check for this flag. I think this is a good suggestion if we want to support location selection by either the driver or ethtool. I also think ethtool's assumption that it is the only allocator and can allocate race-free is fundamentally flawed (take two parallel ethtools). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/4/2011 11:45 AM, Ben Hutchings wrote: > On Wed, 2011-05-04 at 11:21 -0700, Alexander Duyck wrote: > [...] >> Honestly what I would prefer to see is a seperate call added such as an >> ETHTOOL_GRSCLSRLLOC that we could pass the flow specifier to and perhaps >> include first/last location call in that and then let the driver return >> where it wants to drop the rule. > > This must not be done as a separate operation because it's racy (in fact > that's an inherent problem with the rule manager). In the sfc driver > (and probably others in future) filters could be inserted for RFS at any > time. > >> That way we can avoid having to create >> an overly complicated rule manager that can handle all the bizarre rule >> ordering options that I am sure all the different network devices support. > > Right, the rule manager can't implement that. > >> The only reason I am not implementing this now is because there aren't >> any drivers in place that would currently use it. I figure once cxgb >> has a means in place of supporting flow classifier rules then Dimitris >> can add the necessary code to ethtool and the kernel to allow the driver >> to specify rule locations. I would prefer to avoid adding features >> based on speculation of what will be needed and would like to be able to >> actually see how the features will be used. > > If you are going to implement the same interface in ixgbe as in niu > (modulo bugs), then I have no objection to going ahead with this and > then adding the option for driver-assigned locations later. > > Please can you confirm that the location specified for > ETHTOOL_SRXCLSRLINS will indeed be used as a priority in case of > overlapping filters? > > Ben. > The ixgbe approach should be nearly identical in terms of how the priorities are based on the location of the filters. The original version from Santwona had the rule manager breaking the rules up into 7 sections so that rules that specified fewer fields would be near the end of the list. I'm pretty sure that was all due to priorities from what I could see in the niu driver since the filters that covered wider ranges were being made lower priority to be matched last. In terms of overloading the get count call, that probably would be the best route in terms of changing rule manager behavior. The only thing I am having a hard time seeing is how the rule manager would be able to distinguish between low priority and high priority filter rules, or is this something that new keywords would be added to the parser for? I just put out version 6 of the patches. Essentially I have reduced the size of the rule manager to being used only on insertion without any rule location specified. The one thing to keep in mind with this rule manager is that the rule at table size - 1 is always going to be the lowest priority rule. So if it was reserved for unspecified rules it would be easy to use something like that to achieve an "auto-select" location that the driver could then reassign as rules were added to it. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-05-04 at 14:07 -0700, Alexander Duyck wrote: > On 5/4/2011 11:45 AM, Ben Hutchings wrote: [...] > > Please can you confirm that the location specified for > > ETHTOOL_SRXCLSRLINS will indeed be used as a priority in case of > > overlapping filters? > > > > Ben. > > > > The ixgbe approach should be nearly identical in terms of how the > priorities are based on the location of the filters. OK, good. > The original > version from Santwona had the rule manager breaking the rules up into 7 > sections so that rules that specified fewer fields would be near the end > of the list. I'm pretty sure that was all due to priorities from what I > could see in the niu driver since the filters that covered wider ranges > were being made lower priority to be matched last. That would make sense. > In terms of overloading the get count call, that probably would be the > best route in terms of changing rule manager behavior. The only thing I > am having a hard time seeing is how the rule manager would be able to > distinguish between low priority and high priority filter rules, or is > this something that new keywords would be added to the parser for? Right, there would have to be keywords to specify that. > I just put out version 6 of the patches. Essentially I have reduced the > size of the rule manager to being used only on insertion without any > rule location specified. The one thing to keep in mind with this rule > manager is that the rule at table size - 1 is always going to be the > lowest priority rule. So if it was reserved for unspecified rules it > would be easy to use something like that to achieve an "auto-select" > location that the driver could then reassign as rules were added to it. I don't think any location value within the physical table size should select this special behaviour. The special location values for auto-select (with whatever priority) should be distinct from all the physical location values. I still need to review your patches but it sounds like they will be ready to apply. Ben.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 4194a20..909ef79 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -442,7 +442,8 @@ struct ethtool_flow_ext { * includes the %FLOW_EXT flag. * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC * if packets should be discarded - * @location: Index of filter in hardware table + * @location: Index of filter in hardware table, or %RX_CLS_FLOW_FIRST_LOC for + * first available index, or %RX_CLS_FLOW_LAST_LOC for last available */ struct ethtool_rx_flow_spec { __u32 flow_type; @@ -1142,6 +1143,10 @@ struct ethtool_ops { #define RX_CLS_FLOW_DISC 0xffffffffffffffffULL +/* special values for ethtool_rx_flow_spec.location */ +#define RX_CLS_FLOW_FIRST_LOC 0xffffffffU; +#define RX_CLS_FLOW_LAST_LOC 0xfffffffeU; + /* Reset flags */ /* The reset() operation must clear the flags for the components which * were actually reset. On successful return, the flags indicate the