> On March 28, 2014, 9:36 p.m., Chi Zhang wrote: > > src/linux/routing.hpp, line 159 > > <https://reviews.apache.org/r/19702/diff/2/?file=537899#file537899line159> > > > > We do need this, but only because libnl doesn't have a way to remove > > one action from the multiple actions on a filter, so we work around the > > issue to do 'replace with all the other remaining actions'. But really we > > wouldn't want this if libnl was a little bit better. > > > > That's why I wish we don't have to do this for append. > > Jie Yu wrote: > As far as I know, there is no simple way to let you atomically add an > action to a filter. You'll still have to do "get+update". That's the reason I > don't want to introduce an interface "append" to make the users feel that the > "append" is atomic, but in fact it is not. > > Chi Zhang wrote: > Cong should weigh in if I am making a mistake here. I think it's atomic > inside the kernel: whatever we do to prepare the 'package' in the user space > won't do any changes to the kernel until we call `rtnl_cls_change`, when the > package gets delivered down to kernel space to do a swap, so we get whatever > atomicity the kernel provides, which I'd assume is good enough. > > I am a little bit against the new Mirror and ::add combo for two reasons: > > - At the end of the day, people are going to map classifier into _cls and > actions into _act and this Mirror abstraction is quite different from the > common understanding of an action in this area. I agree we should make users > of this interface care as little as possible, but it's going to be people who > know networking who'd look at this code, and straying away from the core > concept won't really help them understand better; people who don't know would > learn core concepts first and find it mismatch with our abstraction here. > > - We only have one replace/update in ::cleanup, but we have a _lot_ more > add/append for arp/icmp in the isolator. In ::cleanup where > replaceFilterAction is used, you can see we need to do loops to prepare the > 'vector package'. While we have to in this case, we don't for the append > cases. It feels superfluous to me to do this every time for a reason we don't > have. (We'd actually have to query kernel to get the information, not because > we need it, but because the API needs it, every time we do an append.) > > > > Jie Yu wrote: > > I think it's atomic inside the kernel: whatever we do to prepare the > 'package' in > > the user space won't do any changes to the kernel until we call > `rtnl_cls_change`, > > when the package gets delivered down to kernel space to do a swap, so > we get > > whatever atomicity the kernel provides, which I'd assume is good enough. > > The 'rtnl_cls_change' is atomic as it will just send a RTM_NEWTFILTER > netlink message with flag NLM_F_REPLACE. But we don't have a way to > atomically add a single action to a filter. > > To do an append, you need to get the current list of actions, prepare the > new actions in the user space, and push the changed filter using > rtnl_cls_change mentioned above. You don't have atomicity here since two > netlink messages are involved. > > You can always write a helper function to combine a "get" (atomic) and an > "update" (atomic) in this API to mimic an "append" (not atomic) if you want. > > > In ::cleanup where replaceFilterAction is used, you can see we need to > do loops to > > prepare the 'vector package'. > > That's exactly why I want to make sure only one action is associated with > a filter so that you don't need to iterate over all actions in a given filter. > > > > Chi Zhang wrote: > yeah I was suspecting we weren't talking about the same atomicity issue. > I meant to say that traffic wouldn't be interrupted. > > You were talking about two users updating at the same time. This issue is > definitely here, unless we do active control in the interface. Can we ignore > this issue for now in general until we have that use case? > > Maybe could you give an example how to use ::add for the 2 scenarios? > > also ::add returns false if the given classifer couldn't be found, then > how do we add the first classifier with just ::add? > > Cong Wang wrote: > Jie, what's the problem with getting a filter with a list action and > appending a new action then pushing it into kernel? IOW, who cares about the > new action until we push it? Since we use actions attached to a filter, > modifying an action _is_ modifying the filter itself too, in other word, > essentially we are just replacing a filter here.
Cong, I don't quite follow your comments. I guess there are two problems here: 1) To represent mirror (to a set of links), should we use a single Mirror action, or we use a set of Mirror actions? 2) Problems with the add/update interfaces. For 1), I still think enforcing exact 1 action per filter is a nice semantics to the user, and easier to understand the reason about. For 2), in my case, adding filter uses flag: NLM_F_CREATE | NLM_F_EXCL, meaning that if there exist a matching filter, return immediately without touching the filter. Updating filter uses flag: NLM_F_REPLACE such that if the matching filter does not exist, return immediately. To add an action, you can do is: if (/* filter not exist */) { /* add filter */ } else { /* get the filter info */ /* update the action in user level */ /* update filter */ } You can create a helper function that wraps the above logic. What do you think? - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19702/#review38947 ----------------------------------------------------------- On March 29, 2014, 5:59 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19702/ > ----------------------------------------------------------- > > (Updated March 29, 2014, 5:59 a.m.) > > > Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod > Kone, and Cong Wang. > > > Repository: mesos-git > > > Description > ------- > > UPDATE: > > 1) adjusted a few interfaces per review comments. > 2) added impl. (including tests) for managing links. > > I'll be adding impl. for managing filters soon (currently, they return > Error("Unimplemented").) > > > ------ > > Hey guys, I send this review in order to get an idea about the interface > design. > > Feel free to jump in to express your thoughts, suggestions, concerns, etc. > > Thanks! > > > Diffs > ----- > > configure.ac 5404dc2 > src/Makefile.am 47d03b3 > src/linux/routing.hpp PRE-CREATION > src/linux/routing.cpp PRE-CREATION > src/tests/routing_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/19702/diff/ > > > Testing > ------- > > make check > > sudo make check > > > Thanks, > > Jie Yu > >